Darren and Matt,
For the most part I only found nits. The target_selection checkpoint is
cool. :) I didn't look at the test code, though...
usr/src/cmd/auto-install/Makefile
---------------------------------
35-43: alphabetize
usr/src/cmd/auto-install/ai_manifest.xml
----------------------------------------
22: shouldn't the copyright date be 2008, 2011?
usr/src/cmd/auto-install/svc/Makefile
-------------------------------------
It doesn't look like this file changed. If it hasn't can you revert
it back?
usr/src/cmd/auto-install/svc/auto-installer.xml
-----------------------------------------------
It doesn't look like this file changed. If it hasn't can you revert
it back?
usr/src/cmd/auto-install/utmpx.py
---------------------------------
copyright isn't valid for a new file
usr/src/cmd/installadm/__init__.py
----------------------------------
copyright isn't valid for a new file
usr/src/lib/install_common/__init__.py
--------------------------------------
371: TODO comment?
usr/src/cmd/auto-install/auto_install.py
----------------------------------------
27-37: alphabetize imports
43-65: alphabetize imports
68: Why not inherit from object()?
101: shouldn't debug be set to False?
236-243: use with open(profile, "r") as fh: ....
457-463: Can you move this check up higher so it's caught and
executed sooner?
568: TODO comment?
581: I'm not the biggest fan on catching Exception. If you must do
it at least capture the error string and log it somewhere.
except Exception as err:
self.logger.debug("Uncaught Exception in register_parse_manifest:
%s" % str(err))
return False
(or something similar)
755: TODO?
781: s/Dump ADM/dumpadm
771-815: why an 8 space indent?
817: see line 581 above
839: instantiate a dictionary with dict() instead of {}
997: use list() instead of []
1036-1042: use with() instead
usr/src/cmd/auto-install/checkpoints/__init__.py
------------------------------------------------
copyright isn't valid for a new file
usr/src/cmd/auto-install/checkpoints/target_selection.py
--------------------------------------------------------
copyright isn't valid for a new file
General Nit: Your docstring commenting style changes throughout the
file. May want to standarize if you have time
General Nit: You use len(list) > 0 to determine if a list is populate
at all. An empty list is boolean false, so you can simply change
if len(list) > 0:
to
if list:
and
if len(list) == 0:
to
if not list:
81: TODO statement?
210, 220: s/OptionS/Options
452-457: Does this need to be updated with the latest change to the
DTD for zpools in logical?
480: the default for Zvol.action is "create". this can be deleted
481: add_zvol takes a 'use' argument. You can add it to 479 and
remove this line
493: same as 480
494: same as 481
786: s/amd/and
800-924: you're constantly checking the len(object.children) to see
if the object has any at all. Shouldn't you be use object.has_children ?
849, 850: use disk_kid.action in ["create", "use_existing"]
1068-1071: why not just do: tmp_device_map = device_map.values() ?
1083, 1113: use device.has_children instead
1116: odd 8 space indent
1214, 1273: This isn't true. Slices need to start one cylinder into the
Disk/Partition. Setting a start_sector of 1 will auto round to the
first cylinder for you, however.
1442-1446: move this above the partition walker and you can remove
the if statement at 1419. If there are no partitions in
partition_list, nothing gets done.
something sort of like this:
partitions = disk.get_children(class_type=Partition)
slices = disk.get_children(class_type=Slice)
<whole disk if-statement check>
if self.arch == "x86" and not partitions and slices:
raise SelctionError("Invalid specification of slices outside "
"of partition on this platform")
for partition in partitions:
<stuff>
<....>
1559: get_children() returns a list, so you can do:
if logicals:
<stuff>
1778: s/somehoe/somehow
Whew!
-Drew
On 4/22/11 11:25 AM, Darren Kenny wrote:
Hi,
I'd like to ask for help in reviewing the code for the moving of the Auto
Installer client to the CUD architecture.
Some light reading for the weekend ;)
As Drew pointed out, we're adding less code than we're removing - that has to be
positive, doesn't it??
I'm going to schedule a code review for 8am PDT/4pm IST on Tuesday 26th, I'll
send out details closer to the date.
The webrev is at:
http://cr.opensolaris.org/~dkenny/cud_ai-to-slim/
If at all possible, I would like to get he bulk of the feedback by COB Friday
May 29.
Thanks,
Darren.
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
