1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
|
When to submit a review document
================================
Please do fill out the review form for every significant change.
Significant change means a change in program logic, memory allocation, etc.
We do not need a review form for minimal changes like fixing typos,
fixes to man pages, source code documentation, etc.
How to use the review form
==========================
- copy the template doc/devel/review.txt to a working directory
- if you have one review for all branches affected by your code change, leave
the name "review.txt"
- if you need multiple reviews for different branches, name the review documents
"review_<branch>.txt, e.g. review_V61_BRANCH.txt, or review_maintrunk.txt.
Multiple reviews for different branches might be needed, either because the
code base is too different for a simple code merge (e.g. V60s2_BRANCH vs.
maintrunk), or different solutions have been chosen for the different
branches.
- complete the form and add it as attachment to the Bugster CR,
or if there is no CR, to the IZ
- it is possible to update attachments, if a review document is modified
How to fill out the review form
===============================
1 Please copy/paste the complete Changelog entry for the corresponding checkin
(the first checkin, if the code change is merged to multiple branches, but
only one review document is needed).
2 Conforms to specification
- For Bugfixes or RFE's from Bugster:
Check if the "Suggested Fix" section contains usefull information.
Check if the bug has been fixed according to the "Suggested Fix" section
in the CR.
- If a specification document exists (usually for bigger RFE's):
Check, if the code change has been implemented according to the
specification.
3 Check if the documentation has been changed or change requests have been made
3.1 Does the issue make necessary any changes in the user or admin guide?
If yes, have the changes been documented and communicated to the doc writer?
This is done by creating a Bugster CR in the doc subcategory.
Answer n/a if documentation is not affected.
3.2 Are the man pages and online help correct?
If necessary, have the man pages, or online help been adjusted, and are they
understandable from a user perspective?
Answer n/a if man pages are not affected.
3.3 If commandline switches are implemented or affected by the code change,
check the -help output (has to exist and be understandable from a users
view).
Answer n/a if commandline switches are not affected.
3.4 All interfaces have to be documented.
The following interfaces have to be documented - any documentation beyond
the obligated is welcome:
- GDI Interface
- Event client interface
- any library functions
- java API
- shell functions
If interfaces changed, analyze all affected situations/code pieces.
Documentation shall be done using ADOC headers, or javadoc respectively.
Answer n/a if no documentation is required.
3.5 If messages changed, check if they are correct and meaningful.
Output formats shall only be changed if unavoidable - such changes have to
be documented.
Attention: Output (e.g. from qstat) may be part of the documentation. Please
communicate changes to the doc writer (see also 3.1).
Answer n/a if no messages were changed.
3.6 Bugster Change Request
- A CR in Bugster has to be created
- for all bugs
- for feature development, improvements, ...
- for all changes to files that shall go into a patch
- Is the Synopsis correct, meaningfull and understandable?
- Has the version information been set:
- Introduced in Release
- Targeted Release,
- Commit to Fix in Build,
- Fixed in Build
- Integrated in Build
- Is the status set to "10-Fix Delivered"?
- Is the hook3 field set to the Changelog id?
- Is the hook4 field set to the automatic test path (testsuite or module
test, testsuite IZ number)?
- If there is an Issuezilla entry (there usually should be one, with only a
few exceptions, confidential data): Has the Hook5 field been set to the
Issue Number?
- Is the CR-Number referenced in Changelog?
- If documentation is affected, or messages have changed: Is this documented
in Bugster ("More Info" Tab, "Fix affects ...").
- If the code change includes a fix for memory leaks: Are the keywords
"memory" and "leak" set?
3.7 Issuezilla
- Usually there should be an IssueZilla entry for every change.
- We have no IssueZilla entries for
- GEMM
- Doc Issues
- Is the Synopsis correct, meaningfull and understandable?
- Has the version information been set (Version, Target Milestone,
comment "Fixed in ...")?
- Is the IZ-Number referenced in Changelog?
4 Check source code (diff)
4.1 Does the source code conform to our coding styleguide?
4.2 If memory is allocated, check for deallocation, access to freed memory,
boundaries, static buffer sizes ...
In java projects check if all resources are cleaned up when no longer needed.
Set n/a for changes that do not contain c, or Java code.
4.3 Is the code thread safe? Are all system calls / sge functions the code calls
thread safe? Does the doc header of the function contain info about
thread safety?
Set n/a for changes that do not contain c, or Java code.
4.4 Does the code preserve security? User privileges and file permissions are
set correctly?
Can only privileged users conduct the specified operations -
i.e. only managers can configure the system, or start/stop the daemon,
only arco_write user can write to the database for ARCo project ...
Are the file permissions set correctly?
Set n/a for changes that do not affect security, or user access.
SGE only:
4.5 Does the code break GDI communication?
It might have been necessary to modify object definitions (libs/sgeobj/*L.h
files) to reach the objective of the work package.
This can have 2 implications:
1. If such objects are sent via GDI interface, the GDI version has to be
increased (libs/gdi/version.c).
2. If spooled objects change, an upgrade procedure has to be provided,
and the release notes / upgrade documentation have to mention the change.
Set n/a for changes that do not affect GE objects.
5 Check if adequate testing has taken place
5.1 Has the code been built on all platforms (using the relevant tool for the
project - i.e. raimk for SGE)?
Set n/a for changes that do not affect our c, java code.
SGE only:
5.2 Run the changed component in a memory debugger, e.g.
dbx (check -all), purify, insure.
Optional test for complex scenarios.
Especially for library functions having no dependencies on a running system,
please write a module tests and run this module tests in the memory
debugger.
Set n/a for changes that do not affect our c code.
SGE only:
5.3 (Still) optional: Has the affected c modules been run through lint
(scripts/gelint.sh), and are they lint error free?
Set n/a for changes that do not affect our c code.
5.4 Which manual tests have been done?
Include a short description.
For ARCo project specify the database where the tests were run.
If the test doesn't require testing, e.g. for fixed man pages,
set n/a.
5.5 Does the testsuite sufficiently cover the issue?
If not, please add a test scenario yourself.
This can be a completely new testsuite check or just an addition to an
existing check.
If the test doesn't require testing, e.g. for fixed man pages,
set n/a.
5.6 Has a new testsuite test or module test been created?
If testsuite doesn't cover the issue yet, creation of a new testsuite or
module test is required.
If no test is created, a precise and comprehensive justification has to be
added to the review document and a testsuite issue has to be created.
Leave blank when 5.5 = yes.
5.7 If the testsuite does not cover the issue, and no new test has been created,
a testsuite issue has to be filed.
This issue shall contain information about test cases (usually will
reflect the manual tests),
if scripts have been written for the manual testing, they shall be
attached to the issue.
Leave blank when 5.5 or 5.6 = yes
5.8 A testsuite run should always be done before checking in code.
Even if there is no special test case for the issue, testsuite might show
problems or sideeffects.
Use the comment field if you ran only a subset of tests,
or if you had failing tests, but are sure the failures are not due to your
changes.
6 Comments
Add any comments that came up during the review.
If the change is not accepted in the first review, describe in the
Comments section why it has not been accepted.
7 Accepted
Even if the change has not (yet) been accepted, do add the review paper to
the CR. It describes an intermediate step, and if the review,
or even the code change is continued by a different engineer, it helps to
understand the status of the change, and possible challenges.
If any of the questions in the following review sections has been answered
by "no", the code change cannot be accepted:
- 2 Conforms to specification
- 3 Documentation
- 4 Source review
- 5 Tests, except Testsuite, here we might have cases where no test is done,
but these cases require a justification.
|
