Dermot,
Here's my review. I looked at everything except for the tests. The
Makefile and packaging stuff look fine.
usr/src/lib/install_target/controller.py
----------------------------------------
NIT: use list() instead of []
NIT: store platform.processor() in __init__
51, 53, 55, 58: why are these not Size()'d?
226: NIT - why do we have an __init__ and initialize method? Can
these be combined? Especially since you do this code block:
# Ensure initialize() has already been called.
if self._desired_root is None:
raise BadDiskError("No selected disks to reset!")
more than once.
271: join line to 270
294: join line to 293 (if it fits)
357, 408: s/disk_or_disks/disks
460: s/,//
477: indent is 8 spaces?
536: change to : if not partitions and not slices:
539: check for x86 instead of !sparc
544-545: comment? I'm pretty sure I fixed that bug a couple of weeks
ago....
548, 552: align indent
566, 594: change to: if not slice_list
554-556 (and all copies of these three line) It looks like, to me, you
could move this assignment outsiide of all conditional statements and
only have to do it once.
if not partition and not slices:
stuff
else:
if x86:
stuff
else:
different stuff
new_slice.tag = V_ROOT
new_slice.in_vdev = in_vdev
new_slice.in_zpool = in_zpool
624-625: combine into one if statement
633-634: make as part of an else: clause for the for loop
for disk in discovered_disks:
if self.is_big_enough(disk)
return disk
else:
raise BadDiskError("...")
738, 742: extra parens
843, 933, 943, 951, 954: commented lines?
1018: check for x86 instead of !sparc
1075-1077: why not simply use get_first_child and specify the name
attribute?
slice_zero = disk.get_first_child(class_type=Slice, name="0"):
if slice_zero is None:
return False
else:
if self._vdev is not None:
.....
1087/1095: Can we make this a property and drop the "get" part of the
name? You can also cache it (if possible, not sure if it is) so you
don't make the call to get_children more than you have to.
@property
def discovered_disks(self):
if self._discovered_disks is None:
if self._discovered_root is None:
return None
self._discovered_disks = \
self.discovered_root.get_children(class_type=Disk)
return self._discovered_disks
I ask this because this you make the call to self.discovered_disks *a
lot* and maybe we could speed it up?
1133, 1145, 1156, 1163: extra parens
1169: can't this class inherit from SimpleXmlHandlerBase instead?
That way you don't have to handle all the abstract methods.
1246-1270: This exact same code exists in Karen's text-installer code
so I'll paste what I told her:
Whoa. A os.popen() call! :) Can we update this to use
solaris_install.Popen() instead?
memory_size = 0
p = Popen.check_call(["/usr/sbin/prtconf"], stdout=Popen.STORE,
stderr=Popen.STORE, logger=LOGGER)
for line in p.stdout.splitlines():
if "Memory size:" in line:
memory_size = int(line.split()[2])
break
and get rid of all this try/execpt and os.popen() stuff?
-Drew
On 4/27/11 9:42 AM, Dermot McCluskey wrote:
Hi all,
I am looking for reviewers for the TargetController class.
I don't believe a walk-though is needed, but if anyone thinks
that might be beneficial, then Evan or I should be able to provide
one.
The webrev is at:
http://cr.opensolaris.org/~dermot/controller/
I would like to get comments by COB Thu May 5th, if possible.
If you plan on reviewing, please let me know.
Thanks,
- Dermot
_______________________________________________
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
