Hi John,
Thanks for reviewing! Responses are inline. An updated webrev will be
made available after Sue responds to the man page questions.
- Keith
On 05/ 4/11 04:41 PM, John Fischer wrote:
Sue, Keith, Jesse,
Y'all have been busy making changes which look really good.
Here are my comments on the webrev.
Thanks,
John
================================================================================
usr/src/cmd/ai-webserver/AI_database.py
================================================================================
Looks good.
I'll try to remember to use the 'as' syntax for exceptions.
================================================================================
usr/src/cmd/ai-webserver/cgi_get_manifest.py
================================================================================
1. Should the following be changed to use the 'as' syntax:
320 except OSError, err:
418 except IOError, err:
436 except lxml.etree.XMLSyntaxError, err:
460 except lxml.etree.XMLSyntaxError, err:
'as' syntax is preferred as it is less ambiguous when catching multiple
exception classes ("except (A, B) as C" vs "except (A, B), C" vs "except
A, C" vs "except A as C"). The cases I changed were the ones I saw while
merging various things; the ones I missed weren't omitted for any
particular reason, but I'll go ahead and update now that you've
mentioned them.
================================================================================
usr/src/cmd/ai-webserver/common_profile.py
================================================================================
1. The comments should be consistent:
34 # profiles stored here internally
35 INTERNAL_PROFILE_DIRECTORY = '/var/ai/profile'
36 # MIME attachment name for manifest
37 AI_MANIFEST_ATTACHMENT_NAME = 'manifest.xml'
38 WEBSERVD_UID = 80 # user ID of webserver daemon
39 WEBSERVD_GID = 80 # group ID of webserver daemon
Please either more the comment for lines 38 & 39 to be above.
2. These values should not be hard coded:
38 WEBSERVD_UID = 80 # user ID of webserver daemon
39 WEBSERVD_GID = 80 # group ID of webserver daemon
Instead the values should be retreived from the system. This
should be
done like it is for the webserverd UID in publish_manifest.py:
641 webserver_id = pwd.getpwnam('webservd').pw_uid
642 # change to user/group webserver/root (uid/gid 0)
643 os.chown(manifest_path, webserver_id, 0)
I agree with the above statements and will make the changes as they're
relatively straightforward, though note that those aren't actually from
ISIM.
================================================================================
usr/src/cmd/ai-webserver/create_profile.py
================================================================================
Looks good.
================================================================================
usr/src/cmd/ai-webserver/delete_manifest.py
================================================================================
Looks good.
================================================================================
usr/src/cmd/ai-webserver/delete_profile.py
================================================================================
Looks good.
================================================================================
usr/src/cmd/ai-webserver/export.py
================================================================================
Looks good.
================================================================================
usr/src/cmd/ai-webserver/export_profile.py
================================================================================
I was wondering how to do multiple values for options. Cool.
1. Not sure about the usage statement showing ... for the -p options to
imply that the -p can be repeated.
"export\t\t-p|--profile <profile_name> ... -n|--service
<svcname>"
Should the options be grouped by using '[' and ']'?
set_criteria.py usage
statement style might work better:
47 return _('set-criteria\t-m|--manifest <manifest/script name>\n'
48 '\t\t-p|--profile <profile_name> ...\n'
49 '\t\t-n|--service <svcname>\n'
50 '\t\t-c|--criteria <criteria=value|range> ... |\n'
51 '\t\t-C|--criteria-file <criteria.xml> |\n'
52 '\t\t-a|--append-criteria <criteria=value|range> ... ')
Looks like we had a mismerge; I believe this file should actually be
gone... it became export.py.
================================================================================
usr/src/cmd/ai-webserver/publish_manifest.py
================================================================================
1. Please either move line 169 down to line 174 or move lines 174 &
175 up
to line 168. Unless there is a good reason to have them separate.
169 is actually what will raise the KeyError. I'll move 168 to before
the 'try' block though.
167 try:
168 service = AIService(options.service_name)
169 image_path = service.image.path
170 except KeyError as err:
171 raise SystemExit(_("Data for service %s is corrupt.
Missing "
172 "property: %s\n") %
(options.service_name, err))
173
174 service_dir = service.config_dir
175 dbname = service.database_path
2. Are the following not within a try/except block because by these
points we
know the service_name is valid?
Hopefully the lack of try/except for these make more sense now given the
prior comment. Though actually, the try/except from above is of minimal
value.
622 service = AIService(files.service_name)
623 manifest_path = os.path.join(service.manifest_dir,
files.manifest_name)
1541 service = AIService(data.service_name)
1578 service = AIService(data.service_name)
================================================================================
usr/src/cmd/ai-webserver/set_criteria.py
================================================================================
Looks good.
================================================================================
usr/src/cmd/ai-webserver/test/test_set_criteria.py
================================================================================
Looks good.
================================================================================
usr/src/cmd/ai-webserver/validate_profile.py
================================================================================
1. The following should probably be moved before the try/except block:
140 if verbose:
141 print >> sys.stderr, (_("Validating static profile %s...") %
142 profile_name)
Done
2. Should the blank line be indented at line 177?
175 if verbose:
176 print raw_profile # dump unparsed profile for perusal
177 print >> sys.stderr
178 print >> sys.stderr, errmsg # print error message
No, it's correct. I'll add a blank line of code to separate the print of
raw_profile to stdout from the messaging sent to stderr.
================================================================================
usr/src/cmd/auto-install/svc/manifest-locator
================================================================================
Looks good.
================================================================================
usr/src/cmd/auto-install/version
================================================================================
Looks good.
================================================================================
usr/src/cmd/installadm/Makefile
================================================================================
Looks good.
================================================================================
usr/src/cmd/installadm/ai_smf_service.py
================================================================================
Looks good.
================================================================================
usr/src/cmd/installadm/aimdns.py
================================================================================
Looks good.
================================================================================
usr/src/cmd/installadm/aimdns_mod.py
================================================================================
Looks good.
================================================================================
usr/src/cmd/installadm/aimdnsd.py
================================================================================
Looks good.
================================================================================
usr/src/cmd/installadm/create_client.py
================================================================================
Looks good.
================================================================================
usr/src/cmd/installadm/create_service.py
================================================================================
Looks good.
================================================================================
usr/src/cmd/installadm/delete_client.py
================================================================================
Looks good.
================================================================================
usr/src/cmd/installadm/delete_service.py
================================================================================
Looks good.
================================================================================
usr/src/cmd/installadm/grub.py
================================================================================
0. Should this be more generalized so that other components can use the
grub menu generation?
Nope. That's what pybootmgmt will be doing.
1. Should the following be present in the push or should it be a comment
instead:
143 print "\nXXX Dev note: install pxegrub in %s (per
6960345)\n\n" % \
144 os.path.join(image_path, "boot/grub")
Or perhaps a logging.debug() statement?
This will be removed prior to the push. Actually, I think I can remove
it now.
2. Should the default and timeout be retrieved from some OS environment
variable? Probably not but thought I would ask.
149 menu.write("default=0\n")
150 menu.write("timeout=30\n")
Possibly set as constants, or parameters to setup_grub, but it's a minor
issue, and I expect a lot of this to change when installadm moves to
adopt pybootmgmt later.
================================================================================
usr/src/cmd/installadm/image.py
================================================================================
1. Should we simply raise an exception if the file exists so as to
not cause
an issue rather then removing the destination?
173 dest = os.path.join(com.WEBSERVER_DOCROOT,
174 self.image_area.lstrip("/"))
175 try:
176 os.symlink(self.image_area, dest)
177 except OSError as err:
178 if err.errno == errno.EEXIST:
179 os.remove(dest)
180 os.symlink(self.image_area, dest)
181 else:
182 raise
No, we should remove the old symlink. WEBSERVER_DOCROOT is a private
directory; if EEXIST comes up, it means a prior service
creation/deletion/modification failed. The best approach is to just fix
it up, so that's what's done.
Yes, I am paranoid? What was that? /me looks around.
================================================================================
usr/src/cmd/installadm/installadm-common.sh
================================================================================
Looks fine.
================================================================================
usr/src/cmd/installadm/installadm.py
================================================================================
1. Is it safe to assume that the service is an alias in this case?
132 except InvalidServiceError as err:
133 raise SystemExit(_("This alias may not be enabled until
all "
134 " invalid manifests and profiles have
been"
135 " corrected or removed."))
Or can a service end up in this invalid state?
Changed alias to service to 'future proof' this.
================================================================================
usr/src/cmd/installadm/installadm_common.py
================================================================================
1. Please use the 'as' syntax:
795 except CalledProcessError, e:
815 except CalledProcessError, e:
852 except CalledProcessError, e:
Done.
================================================================================
usr/src/cmd/installadm/list.py
================================================================================
1. s/of/off/
234 # strip of the leading '01' and reinsert ':'s
Fixed.
2. My assumption is that the err contains the service name for the
following:
337 try:
338 config.verify_key_properties(akey, serv)
339 except config.ServiceCfgError as err:
340 print >> sys.stderr, err
341 continue
Correct.
3. Should aliasofwidth be reset after line 365?
363 aliasofwidth = max(len(aliasof), aliasofwidth)
364 else:
365 info['aliasof'] = '-'
Initially aliasofwidth is 0. This it should now be 1, right? Not
a big
deal because of line 387 takes care of this case.
Good point though. I went ahead and updated aliasofwidth to start at 1.
================================================================================
usr/src/cmd/installadm/properties.py (deleted)
================================================================================
Makes sense as all functionality moved elsewhere.
================================================================================
usr/src/cmd/installadm/rename_service.py
================================================================================
Looks good.
================================================================================
usr/src/cmd/installadm/service.py
================================================================================
1. Comment does not seem to match the functionality of unmount_all:
220 # If this installadm server doesn't understand this
221 # service, do not mount it.
Updated.
2. Something does not parse in my brain with the following comment:
344 # _check_version should ONLY by AIService.create during
service
345 # creation. Bypassing the version check when the service
has already
346 # been created may lead to indeterminate results.
Does it make more sense phrased as:
# _check_version should ONLY be used by AIService.create during
service
# creation. Bypassing the version check when the service has
already
# been created may lead to indeterminate results.
3. Should there be a set of version constants or a dictionary for:
265 if alias is not None and service.image.version < 3:
271 compatibility_port = (service.image.version < 1)
751 if newbasesvc.image.version < 3:
854 if self.image.version < 2:
977 if self.image.version < 3:
like:
353 elif version < self.EARLIEST_VERSION:
355 elif version > self.LATEST_VERSION:
I suppose would be worth defining, in image.py, constants for the
versions like so:
PORT_VERSION = 1.0
SC_PROFILE_VERSION = 2.0
SYSCONF_VERSION = 3.0
4. Is it time to fix the imports:
1108 # XXX fix imports later
1109 from osol_install.auto_install.publish_manifest import \
1110 insert_SQL, place_manifest, DataFiles
Not quite. We're planning on doing that just prior to push (we should
have mentioned that in the initial code review email).
Moving them now just creates noise in the webrevs.
================================================================================
usr/src/cmd/installadm/service_config.py
================================================================================
5. Can is_enabled() and get_service_props() use _read_config_file()?
I believe so.
================================================================================
usr/src/cmd/installadm/set_service.py
================================================================================
1. Seems like the usage statement could be rewritten to better handle
the
repeating option:
49 'set-service\t-o|--option <prop>=<value> ... <svcname>\n'
The option actually isn't currently repeatable. I'll update the usage.
================================================================================
usr/src/cmd/installadm/setup-image.sh
================================================================================
Looks good.
================================================================================
usr/src/cmd/installadm/setup-service.sh
================================================================================
Looks good.
================================================================================
usr/src/cmd/installadm/setup-sparc.sh
================================================================================
Looks good.
================================================================================
usr/src/cmd/installadm/setup-tftp-links.sh (deleted)
================================================================================
Makes sense.
================================================================================
usr/src/cmd/installadm/svc-install-server
================================================================================
1. Should install/debug be a variable to be consistent with line 61?
61 SMF_PORT=$($SVCPROP -p $PORT $SMF_FMRI 2>/dev/null)
62 export PYLOG_LEVEL=$($SVCPROP -p "install/debug" $SMF_FMRI
2>/dev/null)
Sure
2. Should mount-all be called before the aimdnsd is started?
144 $PYTHON $SVC_MODULE mount-all || exit $SMF_EXIT_ERR_FATAL
This would match the refresh section of the service.
Done
3. ECHO is not defined within the scope of the file. Is there any
reason
why the builtin echo can not be used?
205 ${ECHO} "Usage: $0 { start | stop | refresh }"
Dunno. I didn't write that, but I'll fix it.
================================================================================
usr/src/cmd/installadm/test/test_aimdns.py
================================================================================
Looks good.
================================================================================
usr/src/cmd/installadm/test/test_create_service.py
================================================================================
Looks good.
================================================================================
usr/src/cmd/installadm/test/test_rename_service.py
================================================================================
1. Comment does not seem to match function name:
49 def test_parse_three_args(self):
50 '''Ensure one args caught'''
51 myargs = ["mysvc", "newsvc", "threesacrowd"]
52 self.assertRaises(SystemExit,
rename_service.parse_options, myargs)
Fixed.
================================================================================
usr/src/cmd/installadm/test/test_service_config.py
================================================================================
Looks good.
================================================================================
usr/src/cmd/slim-install/svc/net-assembly
================================================================================
Looks good.
================================================================================
usr/src/lib/install_common/__init__.py
================================================================================
Looks good.
================================================================================
usr/src/lib/netif/Makefile
================================================================================
Looks good.
================================================================================
usr/src/lib/netif/_netif.c
================================================================================
0. The netif.so interface was intended to be short lived which Python
proper
incorporated the if_* interfaces. Thus originally the
getifaddrs() was not
included within this library but in libaimdns.so. So if netif.so is
sticking around and getifaddrs() is added to it then please remove
getifaddrs() from libaimdns.so.
I shuffled this around at the start of the project while we were getting
our heads wrapped around things, and the changes were unneeded. I'll be
reverting all the netif changes, as they don't appear to be needed by
any of what we've done with ISIM.
1. Update the copyright:
22 * Copyright (c) 2010, Oracle and/or its affiliates. All rights
reserved.
2. Should netif_methods be changed to _netif_methods to keep the naming
consistent?
344 static PyMethodDef netif_methods[] = {
377 module = Py_InitModule("_netif", netif_methods);
================================================================================
usr/src/lib/netif/_netif.h
================================================================================
1. Does the copyright need to be updated?
22 * Copyright (c) 2010, Oracle and/or its affiliates. All rights
reserved.
================================================================================
usr/src/lib/netif/mapfile-vers
================================================================================
1. Update the copyright:
21 # Copyright (c) 2010, Oracle and/or its affiliates. All rights
reserved.
================================================================================
usr/src/lib/netif/netif.py
================================================================================
Looks good.
================================================================================
usr/src/lib/netif/test/test_netif.py
================================================================================
1. Update the copyright:
22 # Copyright (c) 2010, Oracle and/or its affiliates. All rights
reserved.
================================================================================
usr/src/man/installadm.1m.txt
================================================================================
I'll let Sue respond to man page questions.
1. Comma needed before '...'
22 -o|--option <prop>=<value>... <svcname>
2. Long options should be added for all subcommands. Both in the
subcommand
summary list at the beginning and in the description section for each
subcommand.
3. Comma needed before '...'
54 -f <profile_file> ...
56 [-c <criteria=value|range> ... |
4. Should aliasing be added to the list of tasks:
122 The installadm utility can be used to accomplish the
123 following tasks:
124 - Set up installation services
125 - Set up installation images
126 - Set up or delete clients
127 - Add or delete manifests
128 - Specify or modify criteria for a manifest
129 - Add or delete system configuration profiles
130 - Validate profiles
131 - Add or modify criteria for a manifest or profile
132 - Export profiles
133 - Enable or disable installation services
134 - List installation services
135 - List clients for an installation service
136 - List manifests for an installation service
137 - List profiles for an installation service
5. Probably should be dhcp_ip_count:
219 -c|--ip-count <dhcp_ip_cont>
6. Service name should either be listed first or last to match the
summary
section and be consistent within the description section for the list
command.
321 installadm list [-n <svcname>] [-c] [-m] [-p]
322
323 Lists all enabled installation services on a server.
324
325 -c
326 Lists the clients of the installation services on a
327 local server.
328
329 -m
330 Lists the manifests associated with the installation
331 services on a local server.
332
333 -n <svcname>
334 Lists information about the specific installation
335 service on a local server. Or:
336 - if the -c option is specified, lists the client
337 information associated with the installation
338 service.
339 - if the -m option is specified, lists the
340 manifests associated with the installation
341 service.
342 - if the -p option is specified, lists the
343 profiles associated with the installation
344 service.
345
346 -p
347 Lists the profiles associated with the installation
348 services on a local server.
Other commands have similar issues within the man page for example
add-manifest. Alphabetical order for options doesn't seem correct
but since that appears to be what is done here that would be ok too.
7. Required should be added for all commands that require a service
name:
500 -n <svcname>
501 Specifies the name of the installation service this
502 manifest or profile is associated with.
This is inconsistent throughout the man page.
7. Are commas needed after ...
563 -P <profile_file> ... |
564 -n <svcname> -p <profile_name> ...
================================================================================
usr/src/pkg/manifests/install-installadm.mf
================================================================================
Looks good.
================================================================================
usr/src/pkg/manifests/system-install-auto-install-auto-install-common.mf
================================================================================
Looks good.
================================================================================
On 05/ 2/11 03:36 PM, Sue Sohn wrote:
We are requesting a code review of the ISIM project (minus the DHCP
related
modifications which will be sent out for review separately).
Webrev location:
http://cr.opensolaris.org/~kemitche/webrev.ISIM.1/
Please provide your comments by May 16.
The code can be divided into the following sections:
1) ai-webserver subdir
usr/src/cmd/ai-webserver/AI_database.py
usr/src/cmd/ai-webserver/cgi_get_manifest.py
usr/src/cmd/ai-webserver/common_profile.py
usr/src/cmd/ai-webserver/create_profile.py
usr/src/cmd/ai-webserver/delete_manifest.py
usr/src/cmd/ai-webserver/delete_profile.py
usr/src/cmd/ai-webserver/export.py
usr/src/cmd/ai-webserver/export_profile.py
usr/src/cmd/ai-webserver/publish_manifest.py
usr/src/cmd/ai-webserver/set_criteria.py
usr/src/cmd/ai-webserver/test/test_set_criteria.py
usr/src/cmd/ai-webserver/validate_profile.py
2) installadm subdir - commands plus service_config.py:
usr/src/cmd/installadm/create_client.py
usr/src/cmd/installadm/create_service.py
usr/src/cmd/installadm/delete_client.py
usr/src/cmd/installadm/delete_service.py
usr/src/cmd/installadm/installadm.py
usr/src/cmd/installadm/list.py
usr/src/cmd/installadm/rename_service.py
usr/src/cmd/installadm/service_config.py
usr/src/cmd/installadm/set_service.py
3) Rest of installadm subdir, including service.py and image.py
usr/src/cmd/installadm/ai_smf_service.py
usr/src/cmd/installadm/aimdns.py
usr/src/cmd/installadm/aimdns_mod.py
usr/src/cmd/installadm/aimdnsd.py
usr/src/cmd/installadm/grub.py
usr/src/cmd/installadm/image.py
usr/src/cmd/installadm/installadm-common.sh
usr/src/cmd/installadm/installadm_common.py
usr/src/cmd/installadm/properties.py
usr/src/cmd/installadm/service.py
usr/src/cmd/installadm/setup-image.sh
usr/src/cmd/installadm/setup-service.sh
usr/src/cmd/installadm/setup-sparc.sh
usr/src/cmd/installadm/setup-tftp-links.sh
usr/src/cmd/installadm/svc-install-server
4) Other (Makefiles, packaging changes, lib changes, man page)
usr/src/cmd/auto-install/svc/manifest-locator
usr/src/cmd/auto-install/version
usr/src/cmd/installadm/Makefile
usr/src/cmd/installadm/test/test_aimdns.py
usr/src/cmd/installadm/test/test_create_service.py
usr/src/cmd/installadm/test/test_rename_service.py
usr/src/cmd/installadm/test/test_service_config.py
usr/src/cmd/slim-install/svc/net-assembly
usr/src/lib/install_common/__init__.py
usr/src/lib/netif/Makefile
usr/src/lib/netif/_netif.c
usr/src/lib/netif/_netif.h
usr/src/lib/netif/mapfile-vers
usr/src/lib/netif/netif.py
usr/src/lib/netif/test/test_netif.py
usr/src/man/installadm.1m.txt
usr/src/pkg/manifests/install-installadm.mf
usr/src/pkg/manifests/system-install-auto-install-auto-install-common.mf
Thanks,
Sue, Keith, Jesse, and Harold
_______________________________________________
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
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
