> Niall, I looked at everything.
You da man! Thanks :D
I'll respond to specific issues below, anything that I don't respond to is
taken as accepted and agreed with.
I primarily focused
> on boot.py, though.
> Anything I didn't comment on looks ok to me.
>
>
> NITS:
>
> - You've got some Sun Microsystems copyrights in some
> of your headers.
Oops. What can I say? It's hard to let go! Will fix.
>
> Questions:
>
> - Are you removing the grub_setup.py checkpoint? If
> so, are you planning on
> removing the Grub specific DOC definitions from
> distro_spec.py and the
> test for
> grub_setup.py?
Good catch, I forgot to remove these in the rush to get this out for review.
WIll remove
>
> - do you have coverage numbers for your unittests?
Yes:
test_boot_spec.py: 99%
test_boot.py: 76%
Getting higher coverage on test_boot.py is very difficult because it wants to
do lots of low level damaging stuff to the system :)
>
> 115-116: not really sure what this comment is
> saying. Can you clarify?
Will do. Basically, we combine the fake target physical target section above
for SPARC or X86
with a logical target section that is populated on the fly from the active BE:
Fake physical, real logical
> boot.py:
> -------
>
>
> 90: where did the value of 5 come from?
It's a non scientific, conservative estimate of how long the checkpoint might
take.
In common use it should only take a second or so typically.
>
> 129: can you move this to __init__ ?
Yes.
> 152: can this move to __init__? You already have it
> there, but it's
> initalized as None. Could it initialize as list()
> instead?
Yes and yes.
>
> 207: trailing ']' ?
It is there to so that the code can be more or less just uncommented (with the
existing ']:' replaced by a ',' at line 204. Do you want me to ajdust it?
>
> 245-258: can you use shutil.copy2() instead? It
> will handle copying all of
> the file's metadata (uid, gid, permissions, etc.)
> along with copying the
> file.
> It's equivalent to cp -p
>
I could use copy2, however I would still have to go through the motions since
it can not be
presumed that the permissions and ownership of 'src' match those required for
'dest' as
determined by commit_boot_config() so I need to explicitly set them to match
the uid, gid
and mode values specified in the result returned by commit_boot_config()
>
> 385-387: can we even get to this point since we trap
> on arch back in
> execute()?
No. I imported this over somewhat blindly from ict.py but it's not really
necessary here as
you point out. I'll remove it.
>
> 648-651: this is getting ugly. Can you back this
> out into a normal
> for-loop walker?
Sure
>
>
> 674: can this move to __init__ ?
Yes. Will do.
>
> 700-703: NIT: I ended up using 'slc' to designate
> disk slices. Can you
> change slyce to slc to keep things in sync? Not a
> big deal to me either
> way.
>
Will do.
> 360, 718, 894, 1030: You read the first line
> /etc/release a lot. Could you
> store that string in __init__?
It can't be determined during __init__ because it requires the target section
of the DOC to be parse first. I could however modify the code to do an
unconditional
one time check of /etc/release during parse_target_specific_doc
and store the result for later references. Would that
be an acceptable improvement?
Also, there's an
> easier way to get the first
> line:
>
> import linecache
> self.etc_release = linecache.getline("/etc/release",
> 1)
>
> linecache will also never ever raise an exception.
> If the file or line is
> issing it'll return an empty string
Will use that. Thanks.
>
> 798: *waves arms frantically* :D
>
:-)
>
> 839-843: with all the usage of /etc/release,
> shouldn't this move up into
> __init__ ?
Likewise, it can't be done in __init__ since it relies on data parsed from the
DOC. But I can
attempt to improve it in the same way mentioned above.
>
> 990-1001: It hurts me deep down that you used
> enumerate instead of
> zip() :D
zip schmip!
Thanks so much for the full review!! I'll push out a revision once this has had
a chance
to have a couple more eyeballs over it.
Niall
>
>
> -Drew
>
>
>
>
>
>
> On 4/20/11 7:51 PM, Niall Power wrote:
>
--
This message posted from opensolaris.org
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
