Hi Karen,
Thanks for the review. Responses in line.
- Keith
On 05/16/11 04:24 PM, Karen Tung wrote:
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?
There's a bit of boilerplate in some cases (prints to stdout/stderr); so
I've collapsed this by having each of the above call a helper function.
I'm not comfortable changing it more at this point given the timing, and
that this code was not introduced by ISIM.
------------------------------------------
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.
Ok.
------------------------------------------
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?
I don't really know - that section isn't ISIM.
------------------------------------------
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?
I agree - the solution there will be pybootmgmt, eventually. This is
more short/medium-term, just what we need for right now, functionality.
- 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.
Sure.
------------------------------------------
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?
path is a property defined on line 135. Nothing else in this file uses
image.path, but service.py uses it extensively.
Having both path and image_area is admittedly confusing. I've replaced
all self.image_area references with self.path, to make it more
consistent. image.path now looks more like a traditional "read-only"
property.
- line 105: define "image_version" and any other keys that is used
in the .image_info file as constants?
May this be deferred? For such a change to be valuable, it'd need to be
in a place that is used consistently across DC, installadm, etc.
It's unlikely that we'd change the string literal used at any point,
since that would break compatibility with previously constructed images.
------------------------------------------
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?
I think the plan is to get all the AI/installadm pieces in, then move
everything at once. This would be one such place, yes.
------------------------------------------
usr/src/cmd/installadm/installadm_common.py
------------------------------------------
- Why are we defining separate logging format and stuff in this file?
Why not just use InstallLogger?
Short answer: Because installadm itself is not an installer.
Longer answer: There's not much value in instantiating an InstallEngine
for the sole purpose of getting the InstallLogger, particularly when the
logger here is used solely for debugging at this time, and littering
/var/tmp with a new default_log.<pid> every time someone invokes an
"installadm list" seems like poor behavior.
------------------------------------------
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.
As mentioned above, the logger is only used during debugging.
- line 290-291: just pass the logging into the Popen.check_all() call?
The subtlety here is that the script in question may print things to
stdout/stderr. If Popen handles the logging, that output has to get
stored (instead of sent to stdout); then we just end up having to do a
"print popen.stdout" afterwards, and we lose any knowledge of ordering
if the stdout and stderr data are interleaved.
Comment added to explain.
- line 622: why pass in logger=''?
Shorthand for:
logger=logging.getLogger()
(i.e., the root logger, which is what installadm uses at this time)
- line 968-969: pass logger into Popen call?
See above.
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
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
