Hi Drew,
Thanks for reviewing. Responses below.
On 05/ 3/11 05:02 PM, Drew Fisher wrote:
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 []
will do.
NIT: store platform.processor() in __init__
will do.
51, 53, 55, 58: why are these not Size()'d?
Because Size objects cannot be added to, or multiplied
by each other, we need to pick some unit to do the calculations
in. We could just make these values Size()s, but then we'd end
up constantly calling, eg MIN_SWAP_SIZE.get(units=Size.mb_units),
so it seems more convenient to have them as MB values.
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.
The intention was that the user can instantiate a TargetController
object whenever they want, but they cannot call the API methods,
like initialize(), select_disk(), etc, until after TD has completed and
populated "discovered". So, initialize is not called automatically
on instantiation. Perhaps the problem is that initialize is not a
great name?
271: join line to 270
Is that our preferred style? I don't mind, so I'll
change it as you prefer.
294: join line to 293 (if it fits)
It doesn't (but I will remove the unnecessary trailing "\" on 292).
357, 408: s/disk_or_disks/disks
will do.
460: s/,//
will do
477: indent is 8 spaces?
yes. will fix
536: change to : if not partitions and not slices:
will do.
539: check for x86 instead of !sparc
will do
544-545: comment? I'm pretty sure I fixed that bug a couple of weeks
ago....
I'll take your word for it. will remove comment and
unnecessary code
548, 552: align indent
will fix.
(btw, without opening a can or worms, when we have to
do continuation lines, do we have a general preference for
whether the params line up under each other, or are just
indented from the start of the command? eg
my_return_var = my_object.my_method(par1,
par2,
par3)
or
my_return_var = my_object.my_method(par1,
par2,
par3)
566, 594: change to: if not slice_list
will do.
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
yes - I think you're right. will do.
624-625: combine into one if statement
will do.
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("...")
ooh, fancy!
(will do.)
738, 742: extra parens
will remove.
843, 933, 943, 951, 954: commented lines?
yes - will delete
1018: check for x86 instead of !sparc
will fix.
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:
.....
partially agree. I think it still needs to be get_descendants(),
not get_first_child() as we could be talking about slices inside
a partition inside a Disk. But agree on adding name search to
call.
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?
good suggestion - will fix
1133, 1145, 1156, 1163: extra parens
agree - will remove
1169: can't this class inherit from SimpleXmlHandlerBase instead?
That way you don't have to handle all the abstract methods.
Agree - will do.
1246-1270: This exact same code exists in Karen's text-installer code
...that's 'cause we borrowed this code from Karen ;)
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?
yep - will do
Many thanks,
- Dermot
-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
