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:
================================================================================
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)
================================================================================
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> ... ')
================================================================================
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.
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?
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)
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
================================================================================
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?
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?
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")
================================================================================
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
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?
================================================================================
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:
================================================================================
usr/src/cmd/installadm/list.py
================================================================================
1. s/of/off/
234 # strip of the leading '01' and reinsert ':'s
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
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.
================================================================================
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.
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.
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:
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
================================================================================
usr/src/cmd/installadm/service_config.py
================================================================================
5. Can is_enabled() and get_service_props() use _read_config_file()?
================================================================================
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'
================================================================================
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)
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.
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 }"
================================================================================
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)
================================================================================
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.
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
================================================================================
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
