Alok,
2 small points below.
On 11/05/10 18:45, Alok Aggarwal wrote:
Hi Dermot,
Thanks for the review.
On Wed, 27 Oct 2010, Dermot McCluskey wrote:
Hi Alok,
I have only looked at the files you listed in section [2] of your
list.
Comments below.
[2] usr/src/cmd/distro_const/__init__.py
35:
Nit: it seems we should be sorting the imports alphabetically. eg
swap lines 38 and 35.
44:
I know there was some discussion about where is the correct place
to register DOC classes - separately in the modules where they are
defined or all together in the main app.
I thought the correct place was in the modules, not in the app? eg
the Checkpoint class would be registered in the target_spec module?
Did Darren make a statement on this?
usr/src/cmd/distro_const/configuration.py
40:
There are several similar /dev/null streams created in different
modules. Is there a convenient way to combine them all?
I can probably stick it into the __init__.py file.
67:
why don't you check if type is None, in the same way dest and validation
are tested?
Fixed.
138:
unused variable, msg
Removed.
110, 121, 128, 139:
How are there messages presented to the user? I think they need some
context. Also, it might be an idea to re-use the *_LABEL variables when
building the message. eg:
err_base = "Error reading <%s> element from manifest" %
Configuration.CONFIGURATION_LABEL
...
...
if not os.path.exists(source):
raise ParsingError("%s: '%s' does not exist: %s" % (err_base,
Configuration.SOURCE_LABEL, source)
etc
Changed to the suggested format.
130-140:
it appears to me that this method will return an error if:
1. the <validation> sub element is not present (because the
subprocess.check_call()
will fail because path has not been defined?). But according to the
DTD, <validation> is optional.
Am I correct, and if so, is this what you intended?
2. the <validation> sub-element is present, but does not have any
path (again, due to
subprocess.check_call failing, this time because path is None). Is
this also what you intended?
If I am correct, I think the 1st case is an error, but the second
might be valid?
Thanks for catching that. I've changed the try/except
such that that block gets executed only if 'path' is
not None.
That should address both of the above cases.
However, in both cases the exception raised by check_call() is not
subprocess.CalledProcessError
Check out /usr/lib/python2.6/subprocess.py, line 488.
CalledProcessError is the exception that's raised.
If, for example, path is None and you call:
133 cmd = [path, args, source]
and then:
137 subprocess.check_call(cmd, stdout=_NULL, stderr=_NULL)
this will raise something like:
AttributeError: 'NoneType' object has no attribute 'rfind'
(at least that's what I get when I test it).
so line 138 will not catch it. And, in any event, the msg returned
here ("source manifest specified
is invalid...") might be misleading, eg if the path is missing?
I've changed the message to be:
raise ParsingError("Error reading %s element from the
" + \
"source manifest. source manifest specified is " + \
"invalid: %s" % (Configuration.NAME_LABEL, source))
What I'm saying is that if <validation> is missing then the user
will still see the above message, even if the source manifest itself
is perfectly valid - it's just the validation path & co that were missing.
So, in that case, saying the source manifest is invalid might be misleading.
Anyway, both of the above points may be addressed by your next webrev,
so I'll take another look at it then.
- Dermot
148:
validation will never be None here, as you initialized it in line 102.
Removed.
usr/src/cmd/distro_const/distro_spec.py
25:
there is no python doc string for the module here, like in other modules
Added.
215:
there's no "return False" here if the tag is not "grub_mods".
I'm surprised this does not cause errors elsewhere?
Good catch, fixed. I'm surprised as well why this didn't
cause errors elsewhere, perhaps because the grub section is
the last part of the distro section. If it weren't we'd
see the error.
247:
"lines" might be a better name than "line" here, to show that this
attribute
holds multiple <line> elements?
Changed.
287:
This will return False if <title_suffix> is present but empty, or
even if the
*last* <line> is empty, eg:
<grub_entry position="1">
<title_suffix>GRUB Title</title_suffix>
<line>Option #1</line>
<line>Option #2</line>
<line></line>
</grub_entry>
would return False here. Is that what you intended?
Removed.
307:
"if position is not None:" is better here
309:
"if title_suffix is not None:" is better here
(actually, neither of the above tests are necessary as it would just
re-assign None to the attribute anyway, but I agree it's clearer to
have them there.)
311:
"if len(lines):" is better here (I think?)
Changed all of the above.
usr/src/cmd/distro_const/execution_checkpoint.py
39, 82,
comments are misleading: what "class method" do they refer to?
Removed the "method" part.
90:
as the args and kwargs elements are fairly complex in their own right,
my preference would be to have them as separate classes (and perhaps
even
make <arglist>, etc into separate classes also?). As it stands you
are storing up to four levels of sub-elements in one object, eg:
<checkpoint>
<kwargs>
<arglist>
<argitem></argitem>
</arglist>
</kwargs>
</checkpoint>
and you can end up with a list inside a dictionary, inside a
dictionary, inside an object!
Plus see comments below, where I think the logic is wrong (probably
due to being overly complex).
118:
it might make more sense to move this line to after 110, so all the
mandatory
attributes are grouped together?
Done.
121:
174:
188:
the DTD only allows for 0 or 1 <args> sub-element:
<!ELEMENT checkpoint (args?, kwargs?)>
so you should not need to save and iterate through multiple <args>
196:
in the case where you have more than one <arg> element under a <kwargs>
element, then you will end up repeatedly assigning
kwargs[Checkpoint.ARG_LABEL]
with the dictionary of <arg>s at this line. This works, but it is
surely not optimal?
More reason to split these into separate classes!
205-206:
I believe this is broken in the case where there are multiple
<arglist>s under a <kwargs>
element, as is allowed by the DTD, eg if the input to from_xml() is:
<kwargs>
<arglist name="list 1">
<argitem>item 1</argitem>
<argitem>item 2</argitem>
</arglist>
<arglist name="list 2">
<argitem>item 3</argitem>
<argitem>item 4</argitem>
</arglist>
</kwargs>
then the output from to_xml() would be something like:
<kwargs>
<arglist name="list 2">
<argitem>item 1</argitem>
<argitem>item 2</argitem>
<argitem>item 3</argitem>
<argitem>item 4</argitem>
<argitem>item 1</argitem>
<argitem>item 2</argitem>
<argitem>item 3</argitem>
<argitem>item 4</argitem>
</arglist>
</kwargs>
(I've tested this code on its own and that's what I got).
Yet more reasons to split into separate classes!
I'll look into breaking the args/kwargs into a separate
class. The code has gotten very complex as a result of
adding the arglist stuff to the kwargs.
Please take a look at the rewrite when the next revision
of this is posted.
213:
This is fine, but for consistency with your other code, you might
test if
desc and other non-mandatory attributes are None before the assignment?
Changed.
217:
"if len(args):" is better, I think?
Changed.
219:
I'd prefer "if len(kwargs.keys()):" but perhaps Drew can suggest the
most correct
expression to use?
I'll check with him.
usr/src/cmd/distro_const/manifest/boot_archive_contents_sparc.xml
add CDDL header?
usr/src/cmd/distro_const/manifest/boot_archive_contents_x86.xml
add CDDL header?
usr/src/cmd/distro_const/manifest/dc_ai_sparc.xml
2:
I'm wondering what is the correct location to give for the DTD here?
maybe the eventual install location, "/usr/share/install/dc.dtd"
would be best?
Also, if you don't do so already, it might be an idea to validate the
XML
files with "xmllint" from the Makefile, as part of the build?
35 and throughout file.
Only selective lines are indented, eg 35, should be indented; 53
should be
further indented; 56 should be further indented; 113 is over-indented.
Also, some sections have 4-space indents, others have 8-space (eg 295).
I suggest running all the XML files through "xmllint" first to correct
these and other style issues.
296 and elsewhere:
should add a comment to explain the use of DOC search syntax?
297, 301, 308, 465, 472, 476, etc:
closing tag should be merged into opening tag, ie instead an empty
element like:
<image ....></image>
change to:
<image .../>
(xmllint can help with this - except for XML inside comments)
353:
indentation is inconsistent. In fact, I don't think the attributes
should be
split across lines, even if it is allowed. Check what xmllint does
with it.
usr/src/cmd/distro_const/manifest/dc_ai_x86.xml
usr/src/cmd/distro_const/manifest/dc_livecd.xml
usr/src/cmd/distro_const/manifest/dc_text_sparc.xml
usr/src/cmd/distro_const/manifest/dc_text_x86.xml
usr/src/cmd/distro_const/profile/ai.xml
usr/src/cmd/distro_const/profile/generic.xml
usr/src/cmd/distro_const/profile/livecd.xml
usr/src/cmd/distro_const/profile/text.xml
Same comments as previous XML files, eg indentation, closing
elements, etc.
Drew has addressed all of these comments as part of
a separate email response.
Thanks again for the thorough review!
Alok
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
