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>
