On 04/22/11 01:25 PM, 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/
Overall looks quite good. Did not review tests, but let me know if you
need reviewers for those.
Dave
auto-install/__init__.py: start date on copyright should have stayed at 2008
ai_get_manifest.py:
73,828,829: should really be /system/volatile now
ai_manifest.xml:
22: start date on copyright should have stayed at 2008
114: The old manifest was showing the combination of these two
properties applied together. The change doesn't seem the same.
ai_sd.py, 161: s.var/run.system/volatile.
auto_install.py
150 and others [esp. determine_profile_type()]: Ethan's zones spec says
that -m is used for manifests, -p for SCI profiles. The usage here is
-p for manifests (the prior, and confusing usage/terminology). Perhaps
you can fix this now so that the QE tests converge sooner than later and
the terminology doesn't cause developers to create bugs based on a
misunderstanding of what profile means?
272: It seems surprising that we have to do this special handling, as
well as the block at 395 in perform_autoinstall(). Would it make sense
to modify the derivation and parser checkpoints to deal with the fact
they may have nothing to do and let them just run in the normal
sequence? It would seem to simplify a lot of perform_autoinstall(),
though I realize you need to do something after parsing in order to
process the proxy and auto_reboot settings, but even that could
seemingly be a checkpoint.
315: If we're always going to send debug-level to the service log as
implied at 297, is there really a reason to have -v?
437: I'm unclear how "creating Logical" is the right context in this
error message, or what that even means.
480: It would be nice if we could set the dataset for the engine to take
snapshots of during the install process. This would seemingly be
helpful in debugging issues with all of the post-TI checkpoints.
567: what's the use case for -d? Do we really need to bother?
997: s/of/if/
925ff: A lot of this seems like it might be more generally useful than
just for AI. Should we put factoring this out as a future work item?
parse_progress_msg seems like something that ought to come from
ProgressHandler.
1029: Receive
1011: what's this magic number 524288?
target_selection.py
24: nit, new file, should just be 2011
81: not sure what there is to do right here. Surprising that the
initial value isn't 0?
86: why not just use platform.processor()'s value directly?
251: In wording this error message and the many other similar ones that
occur in this module, I'd suggest a slight alteration to provide better
clarity (state the rule that's violated, rather than what was done in
the manifest), and I wouldn't use capitalization where that causes the
string to be different than the element or value used in the manifest
(this helps searching more complex manifests). So, maybe something like
"zpool '%s' must not be specified more than once", or (if you like some
caps at the beginning) "Must not specify more than one zpool named '%s'".
501: The comment here is slightly incorrect, in that the noswap/nodump
specifications allow for there to be none of either one.
547: "per root pool"
685: I would think there are other cases where found_toplevel should be
set, but there isn't any other reference within the for loop that
started at 588.
724: The "vdev redundancy" term seems a little obscure.
795: "each"
1088: "identifiable"
1360: why not do this check against discovered_disk before copying?
1671: Perhaps a better docstring
1778: Does an error or warning get raised somewhere else in this case?
usr/src/cmd/auto-install/svc/auto-installer:
s./var/run./system/volatile.g999
manifest-locator.xml, 42: If the only reason we had this dependency was
to get /tmp mounted, we should drop it, as /system/volatile is mounted
by the kernel before init even runs.
utmpx.py
110: we definitely need to check this behavior when virtual consoles are
turned on, but my guess is that the right answer is to treat that as
nobody being logged in
120: For the present implementation it would seemingly be better to have
users_on_console() take an optional argument to turn on printing and
then you can move 127-131 and delete the rest
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
