On 12/16/10 01:50 PM, Keith Mitchell wrote:
Hi Joe,
General: I think it'd be worth considering laying out the files
differently, rather than using
{osol/solaris}_install.auto_install.utils. "utils" is too generic in my
eyes - if a generic location is needed, dropping directly under
auto_install would be preferred.
For example, osol_install.auto_install.utils.ai_image could simply be
"osol_install.auto_install.image" (note that the "ai_" prefix is
removed, as it's implied by the "auto_install" part).
create_service.py:
42: (Alternative style option) - when importing this many items from a
module, I find it cleaner to simply import the module and reference the
functions/constants via the module, e.g. "import foo.bar.baz as baz;
baz.MY_CONSTANT; baz.my_function()"
113/117/122: Is the "is_iso" really in a position to fatally exit here?
(As much as possible, I try to view functions in isolation from their
context - as an arbitrary consumer of is_iso(), I wouldn't expect those
failures to kill my program). The first OSError will only occur if
"/usr/bin/file" has an issue - quite frankly, it's not worth worrying
about, and if a sysadmin has mucked around with that, it's probably
better if things exit uncleanly.
126-9: Nit: Use the syntax "if 'x' in 'abcxyz'" for checking for
substrings.
131/4: Parens not needed.
165: Re-wrapping this exception doesn't seem to add significant value.
285: This line should probably be run from installadm itself, rather
than individually in each subcommand.
330: Just have create_target raise an appropriate error in the first
place - no need to re-wrap the exception.
399-414: Nits: Trailing '\'s not needed (due to being inside brackets
{}), and should align all lines with the opening brace, e.g.:
line_1 = {hello : "goodbye"
line_2: "foobar"}
487-492: Another good candidate for not-try-excepting.
delete_service.py:
86-87: Use "parser.error(<msg>)" to print_help and exit in one step.
installadm.py:
61: ESCDELAY is a curses-related environment variable and shouldn't be
needed here
129-130: Use parser.error()
ai_image.py:
59: I think the default name should be different.
88: As discussed on the phone, it'd be best to avoid adding yet another
'run_cmd' style function.
160: This appears to be duplicated code from create_service.py
228: Why are root privileges required to *instantiate* an object? Let
the pkg API complain when insufficient privileges are found.
297: Don't raise a new OSError - let the old one propagate!
299-334: Might be worth investigating defining this as an 'abstract base
class': http://docs.python.org/library/abc.html
517: This should be in a 'finally' clause to ensure that CWD is restored
even if a failure occurs.
736: Looks like "self._unmount_iso_image()" should be done just once, in
a 'finally' clause
ai_smf_service.py:
165-169: By setting "props[prop] =" to something, you're modifying the
dictionary that was passed in - a caller may not be expecting that. I
think these lines could be simply removed, and line 170 changed to:
smf.AIservice(smf.AISCF(), pg_name)[prop] = str(props[prop]) # blindly
cast to a str()
On a separate note, is it possible to instantiate the smf.AIservice()
object once, prior to entering the 'for' loop, rather than instantiating
a new one for each property?
339-406: These 3 functions are identical except for the string passed to
smf.AISCF(FMRI=AI_SVC_FMRI).state and the value checked for it. Suggest
consolidating to a single function. (If you're feeling adventurous, I
can show you how to use functools.partial() to achieve this end;
otherwise, defining "MAINTENANCE", "ONLINE", etc. as constants and using
them as inputs to the function will work.
dbbase.py:
The code in this file looks familiar... was this file copied/moved from
elsewhere? If so, before pushing, can you tell 'hg' that it was moved or
copied from another file? (Did not review this since I can't tell what,
if anything, was modified in this file)
grub.py:
327: Rather than _SPACE and _TAB, could you use "\s", or "[ \t]" (a
literal space followed by \t to represent TAB)? That would make these RE
constructs more readable, or at least more consistent with other regex
statements.
339/345: Changing the 'for' loop on line 325 to "for line_num, line in
enumerate(menu_lst_content):" would give you the line number without
requiring a call to menu_lst_content.index().
mount_service.py:
127-130: To prevent errors from the Popen call from bypassing the
null_handle.close() call, use the "with" syntax:
with open("/dev/null", "w") as null_handle:
cmdout, cmderr = Popen(cmd, stdout=null_handle,
stderr=null_handle).communicate()
The null_handle will be closed automatically when leaving the 'with' block.
installadm_common.py:
142: As discussed, even though this is temporary, it can easily be
replaced with: subprocess.check_call()
targetdir.py:
usr/src/lib/install_target/zfs.py now has a ZFS dataset class that may
be leveraged in many of the ZFS functions here.
169: I think the check won't catch correctly if you the cwd is a subdir
of target path. Something like:
if os.getcwd().startswith(target_path):
would catch that case. (There's also a slim chance of symlinks making
the target path look different from the cwd; if that's a concern than
some manipulation from os.path may be in order).
199, 215: There's a lot of raising, catching, re-raising here - is it
all truly necessary, or could some of this be trimmed, in favor of
letting the Python exceptions bubble?
Test code:
Not specifically looked at, though I can take a look if you have
specific areas you'd like reviewed, questions about approaches /
coverage, etc.
On 12/ 2/10 01:08 PM, [email protected] wrote:
This is preliminary code review request for a selection of code for
the Install Service Image Management project.
The ISIM project design document can be found here:
http://hub.opensolaris.org/bin/view/Project+caiman/AI+Image+Management
The webrev can be found here:
http://cr.opensolaris.org/~joev/ISIM_2_DEC_2010/
Please provide feedback by the two weeks from today, Thursday
16.Dec.2010.
For this round of review input please focus on only the following bits:
ai_image:
---------
This code provides a single interface for managing either ISO or
pkg(5) based AI images.
usr/src/cmd/installadm/utils/ai_image.py
usr/src/cmd/installadm/utils/test/test_ai_image.py
Code coverage is over 80%
dbbase:
-------
This functionality is currently provided in installadm_common.py It
has been moved to a separate module, simplified a little and tests a
have been added.
dbbase provides dictionary style access to both system configuration
files and data in string format. I realize it might be better to
implement this as a Mutable Mapping but it had already been
implemented as derived from dict.
usr/src/cmd/installadm/utils/dbbase.py
usr/src/cmd/installadm/utils/test/test_dbbase.py
Code coverage is over 90%
host_service:
-------------
Currently only a skeleton this module will be used to provide a
centralized access point to resources available on the host server.
usr/src/cmd/installadm/utils/host_server.py
usr/src/cmd/installadm/utils/test/test_host_server.py
Test currently only run on a system with the Install SMF service is
available. I have considered having setUp install a dummy SMF service
so tests can be run on build systems. Any suggestions for running
these tests on the build system are appreciated.
Thank you!
Joe
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
Thank you Keith,
I'll be going through this next week.
As my original email described dbbase.py was pulled from
installadm_common.py. I'll contact you regarding how best to develop the
webrev with diff for this. Sorry it wasn't clearer.
Regarding file placement. Clay also had some layout ideas. Perhaps the
three of us could meet to discuss. I'll arrange that.
Joe
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
