On 12/21/10 11:32 AM, William Schumann wrote:
Requesting code review for enhancements for service configuration
profiles in the Automated Installer.
http://cr.opensolaris.org/~wmsch/profile/
I was short on time, so tests were not reviewed.
High-level comment that impacts a number of different files: why place
the cgi in /var; wouldn't it be better to have in
/usr/lib/installadm/cgi, for example? /var/ai should be for
configuration, not executables.
High-level comment #2: seems like a lot of places here where with()
should be used for handling files.
High-level nit: the comments here are inconsistently styled, in many
cases with no space following the hash character, which detracts from
readability and the overall impression. Would be helpful to clean those up.
common_profile.py
General comment in this file is that I see camel-case in some names, and
in others you're using underscore-delimited, all lowercase multi-word
names. Is there some rule here accounting for the seeming inconsistency?
General comment #2 is I'd prefer not to have the extensive amount of
inclusion code here present until we decide to go ahead with any of that
support. This seems to affect locate_profile.py as well.
40, 43: why /var/ai/sc_profile in one place, /var/ai/profile in another?
Can we collapse?
45, 46, others: I'm skeptical of having our own log rotation going on
here. logadm is the general facility in Solaris for administrators to
control log rotation and I'd prefer to see that used since it provides a
way for them to express their preferred policy, which doesn't seem to be
supported here. Also, I'm somewhat surprised that this isn't using
InstallLogger, but perhaps there's a reason that's not suitable somehow?
58: nit, but normalizeIPv4() seems like a better name.
136: as above, normalizeMAC() seems like a better name for this method
815: what Python wouldn't support IEEE754? Is this something we really
need to concern ourselves with?
create_profile.py
214: Is continuing here the right thing to do when we're creating
multiple profiles? Does the user have a straightforward way to clean up
in the face of a partially completed "transaction"? Other failures in
this loop (such as at 261, 266) will cause the transaction to be
aborted, so I'm concerned that inconsistent semantics here will be
challenging to support.
274: I'm generally not in favor of checks like this, as they become
debris to be cleaned up when we try to use more fine-grained
authorizations. This applies to the other sub-commands, as well, and I
won't repeat.
delete_profile.py
93: Assuming I understand this right, providing a name that doesn't
exist will abort the transaction at that point, but other failures later
on (111, 119) will continue. As with create, I think consistent error
semantics would be a good idea when handling multiple objects.
locate_profile.py
285: I'm doubtful this validation is the right thing to do. It seems to
pre-suppose that the server will have the same DTD the client would have
in the boot service, but that seems to impose a requirement on the
server that it be updated to the latest possible version of the DTD,
which is generally undesirable to require from an operational point of
view. Or am I misunderstanding how this would work? This presumably
could impact the validate_profile_string() function in
common_profile.py, too.
set-profile.py
173: I'm confused by this comment and the following code. The comment
claims there should be only one response, but goes on to handle any
number; seems this part of the comment should just be deleted since it
will easily become out of date?
185: seems like we should have some way to avoid the prompt for
scripting purposes?
ai_get_manifest.py
47, 52, 53: Please use /var/run; /tmp is not protected against
interference by unprivileged processes.
55-60: please remove until security work is actually implemented
auto-installer, 41: We should be using /var/run here, as well
Dave
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
