Hi Darren and Matt.
Here are my comments on target_selection.py:
First of all, seems like a really nice piece of work!
I am not an expert at this stuff and to become one would take me much
longer, so I did a medium-depth check given the time constraints...
210, 220: OptionS -> Options
__handle_vdev, __handle_filesystem ,__handle_zvol,
__handle_pool_options, __handle_dataset_options are all the same except
for their logging messages. I see why this was done, as some entities
such as BEs or zpools need more processing and thus must have their own
handlers. However, the simple copy-only methods could be combined into
a generic, simple method and their logging messages displayed by the
method that calls the generic method, and
that would condense the code by ~30 lines.
452-457: Cleanup needed?
466, 1488: occured -> occurred
478, 491: This may have already been discussed, and this may be better
handled as a follow-on bug, but it seems overly-simplistic to calculate
swap and dump to be 1/2 the RAM size. These days some systems have so
much RAM that they have little or no swap. And as systems with
terabytes of RAM will work their way into the world, do we need swap and
dump that are half the size of RAM?
769: idenfifying -> identifying
782, 886: idenfifiable -> identifiable
786: amd -> and
795: ecah -> each
987: uniqely once -> once
1010: devince -> device
1028: Can pull start_vdev_key assignment out of the loop.
1035: Not sure why a dict is needed here. A list should do.
uniq_vdevs = []
for vdev in zpool.children:
if isinstance(vdev, Vdev) and not vdev in uniq_vdevs:
uniq_vdevs.append(vdev)
return uniq_vdevs
I don't think you can use a list comprehension though, since the check
would need to access the list you are building, i.e. the "not vdev in
uniq_vdevs" of the below:
uniq_vdevs = [vdev for vdev in zpool.children if isinstance(vdev, Vdev)
and not vdev in uniq_vdevs]
1088: identifyable -> identifiable
1134: parition -> partition
1145: specifiction -> specification
1155: Will a maintainer in the future know what TI stands for? I would
spell it out.
1184: Can make this elif
1494: nit: change name of __create_temp_logicals() to
__create-temp_logical() since only one is being created.
1631: looks like a personal comment. "I think..."
1689: nit: s/have no children/have children/
1704: s/ to install too / to install to /
1778: somehoe -> somehow
1820: excpetion -> exception
many places: existant -> existent
Throughout the file (example: 190, 270): \ not needed with () spans
multiple lines.
Thanks,
Jack
On 04/22/11 10: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
