On 11/ 1/10 02:32 PM, Drew Fisher wrote:
Jean,
Thanks for the code review. Comments inline.
[3] usr/src/cmd/distro_const/checkpoints/pre_pkg_img_mod.py
line 200: Should you be unsetting the new LD_LIBRARY_PATH?
Fixed.
line 303: shame on you ;-)
:)
usr/src/cmd/distro_const/checkpoints/boot_archive_configure.py
line 111: Is read the default here? Might be nice to specify for
readability.
read is the default, but I added the "r" flag to open() to be explicit.
line 150: s/createing/creating/
fixed.
line 233: I don't understand why you have path here and then join it
with subdir which seams to just yield subdir.
That code block is designed to take an entire path of directories
(/etc/foo/bar/baz) and, starting at the beginning directory, check for
existence. If it exists, skip it. If not, create the directory and
copy the metadata from the pkg_image area to the boot_archive area.
So, it starts with etc/. etc/ exists, so nothing happens. Then it
tries etc/foo. etc/foo does NOT exist, so it gets created and updated
with the proper metadata information. Then it tries etc/foo/bar, etc.
etc.
Thanks.
line 303,352,448: Do we not want to use dry_run for DC?
Right now, we have no provisions for dry_run. Since the checkpoints
require the build_data dataset to be in specific states before we can
run at all, having a dry_run flag which would arguably do nothing at
all didn't seem to be worth much to DC.
OK.
line 416: I'm not familiar with this .conf file, is there potentially
more than one daemon line?
No, there's only one line with [daemon] in it. Not sure what you'd
prefer me to do otherwise, though?
As long as there's realistically only one line with daemon in it I'm fine.
usr/src/cmd/distro_const/checkpoints/boot_archive_archive.py
Line 120: you do a chdir(directory). Where do you chdir back?
Fixed.
If I remember correctly there were a bunch of these. Did you happen to
catch all of them?
Yeah, I know I didn't write down all of them. Probably should have.
line 245,250,253,401: Just a heads up that with the transfer
restructuring we talked about these lines will change. I'll work with
you on it.
Or, I'll use copytree() again. :)
no no no she screams
lines 268-271: If we're just throwing away the nbpi that was
calculated in calculate_ba_size why bother? Would it make sense to
set self.nbpi from the code that calculates nbpi if it hasn't been
passed in so that this if goes away? This is kind of a nit though.
We're not throwing it away, though. The nbpi is used a few lines
later on the instantiated lofi object.
line 300: Although I like this message I suspect it should go, or at
least the HEY's.
:) No more Fat Albert in DC.
line 334: you chdir here, do you need to chdir back?
Fixed.
line 444: You never do anything with dry_run. Is that on purpose?
See above on dry_run.
usr/src/cmd/distro_const/checkpoints/test/test_pre_pkg_img_mod.py
What coverage did you get?
Doc strings would be nice.
line 82: Yeah, verification would be nice. This test always passes,
right?
usr/src/cmd/distro_const/checkpoints/test/test_boot_archive_configure.py
What coverage did you get?
Doc strings would be nice.
usr/src/cmd/distro_const/checkpoints/test/test_boot_archive_archive.py
What coverage did you get?
Some doc comments as to what these are actually doing would be nice.
line 249: It's commented out so this test really doesn't test anything?
Still need to update the tests with doc strings and try to finish off
test_baa.py to cover the SPARC specific stuff.
Thanks, Jean!
-Drew
That's it.
Jean
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
