I looked at everything in section 3. I have no comments on
files that are not listed below.
------------------------------------------
usr/src/cmd/installadm/ai_smf_service.py
------------------------------------------
- There seemed to be a lot of duplicate code in the
maintain_instance(), disable_instance(), enable_instance(),
restore_instance() functions. Can we combine them into 1 function,
and just pass in different flags to make it do different things?
------------------------------------------
usr/src/cmd/installadm/aimdns_mod.py
------------------------------------------
- line 410: I think a "break" here to break out from the for loop in
line 407,
since you already found what you are looking for.
------------------------------------------
usr/src/cmd/installadm/aimdnsd.py
------------------------------------------
- Question: line 134, the comment says that var/run/aimdns file left over,
truncate via open it. Howver, in line 137, the file is opened with
mode 'w+'. Should it be opened with 'w' instead?
------------------------------------------
usr/src/cmd/installadm/grub.py
------------------------------------------
- General comment about this file. The boot checkpoint in
usr/src/lib/install_boot have a lot of code there manipulating the
grub menu. I wonder whether we can put the functionality implemented
here to somewhere in the boot checkpoint. That way, we have all
the grub manipulation code in 1 single place instead of scattered all over.
Futhermore, the setup_grub() function starting in line 124 looks like
something DC and all the installers also do, and there's already code
in the boot checkpoint to do this. Perhaps something from the boot
checkpoint can just be called?
- line 117-120: would it be more efficient to put this into a "else" clause
of the "if" block that starts in line 109? A line can not be both a
kernel or module line. So, if it is already a kernel line, no need to
try partitioning it.
------------------------------------------
usr/src/cmd/installadm/image.py
------------------------------------------
- line 79-95: the verify() function relies on the value of self.path, but
self.path is not initialized in the the __init__() function, and it is
not checked prior to use in this function. I think the function should
at least check self.path is defined. Furthermore, I don't see any other
functions in the InstalladmImage class set or use self.path. Is this
really the variable that should be checked? Should the "self.image_area"
be checked instead?
- line 105: define "image_version" and any other keys that is used
in the .image_info file as constants?
------------------------------------------
usr/src/cmd/installadm/installadm-common.sh
------------------------------------------
- line 58: question: after AI is moved to CUD, things will be
in solaris_install/auto_install/.... I assume you will have
to update this path after they putback?
------------------------------------------
usr/src/cmd/installadm/installadm_common.py
------------------------------------------
- Why are we defining separate logging format and stuff in this file?
Why not just use InstallLogger?
------------------------------------------
usr/src/cmd/installadm/service.py
------------------------------------------
- General question about this file: I observed that all the error
in this file is printed with "print >> sys.stderr....", while logging
is done with logging.debug(). Error messages are not logged to the logger.
Is that intentional? I also don't see where the logger is setup.
- line 290-291: just pass the logging into the Popen.check_all() call?
- line 622: why pass in logger=''?
- line 968-969: pass logger into Popen call?
Thanks,
--Karen
On 05/02/11 15:36, 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
