Dave,
I implemented all of your recommendations, except the ones we discussed on
caiman-discuss.
Resubmitting for review at http://cr.opensolaris.org/~wmsch/profile2/
Decided to retain the multiple profile specifications on the command line.
Cleaned up the handling inconsistencies you mentioned.
Due to the changes in the AI webserver, I withdrew my modifications in ai_get_manifest.py, the locate_profile.py script, Makefile
and other CGI-related code. Changes to support multiple profiles will be submitted for review once they are folded in with the AI
webserver modifications.
Moved criteria changes to set_criteria.py.
Reduced redundancy per Sue's recommendations.
Removed all profile-specific logging, files, handlers, etc. The only logging will occur with the profile locator CGI code to the
Apache error log.
Thank you,
William
On 01/12/11 09:51 PM, Dave Miner wrote:
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
