Jack,
Few. Lots of good stuff. Keep it up. You're almost there.
See below for comments.
Thanks,
John
================================================================================
usr/src/Makefile.master
================================================================================
1. There needs to be parenthesis around ROOTPYTHONVENDORSOLINSTALLMANINPUT
below:
118 ROOTPYTHONVENDORSOLINSTALLMANINPUTTEST= \
119
$ROOTPYTHONVENDORSOLINSTALLMANINPUT/test
================================================================================
usr/src/Targetdirs
================================================================================
1. There seems to be an indentation issue at:
86 /usr/lib/python2.6/vendor-packages/solaris_install/target/libadm \
87 /usr/lib/python2.6/vendor-packages/solaris_install/target/libbe \
88
/usr/lib/python2.6/vendor-packages/solaris_install/target/libdiskmgt \
89 /usr/lib/python2.6/vendor-packages/solaris_install/target/libefi \
90
/usr/lib/python2.6/vendor-packages/solaris_install/target/libnvpair \
91 /usr/lib/python2.6/vendor-packages/solaris_install/target/shadow \
================================================================================
usr/src/cmd/aimanifest/aimanifest.py
================================================================================
1. If we are alphabetizing imports then according to /usr/bin/sort the
following needs to be switched:
39 from solaris_install.manifest_input.mim_errors import MimError,
MimDTDInvalid
40 from solaris_install.manifest_input.mim import ManifestInput
2. Please remove the parenthesis like you have for other sections:
90 if (AimLog.enabled):
3. Parenthesis is not needed around the len(args) clauses below:
187 if command in cmds_w_value_arg and (len(args) != 3):
189 if ((command in cmds_wo_value_arg and (len(args) != 2)) or
190 (command in cmds_w_no_args and (len(args) != 1))):
4. Parenthesis are not needed:
212 if (command == "set") or (command == "add"):
================================================================================
usr/src/cmd/auto-install/ai_get_manifest.py
================================================================================
Looks fine.
================================================================================
usr/src/cmd/auto-install/auto_install.py
================================================================================
Looks fine.
================================================================================
usr/src/cmd/auto-install/checkpoints/dmm/dmm.py
================================================================================
1. If we are alphabetizing imports then the following needs to be reordered:
37 import os
38 import linecache
39 import pwd
40 import platform
2. The second + is not needed in the following:
113 errmsg = (MSG_HEADER + "Scriptfile must be
specified or " +
114 "stored in data object cache.")
though does not hurt anything.
3. Parenthesis not necessary in:
347 if ((not first_line.startswith("#!")) or
348 (not (KSH93_SHEBANG in first_line or
349 PYTHON_SHEBANG in first_line))):
which could be changed to:
if not first_line.startswith("#!") or
not (KSH93_SHEBANG in first_line or
PYTHON_SHEBANG in first_line):
4. Outer parenthesis can be removed in:
371 perms_ok = ((mode & own_rx) == own_rx)
373 perms_ok = ((mode & grp_rx) == grp_rx)
375 perms_ok = ((mode & oth_rx) == oth_rx)
5. Parenthesis can be removed in:
432 if ((tree.docinfo is not None) and
433 (tree.docinfo.system_url is not None)):
================================================================================
usr/src/cmd/auto-install/checkpoints/dmm/dmm_errors.py
================================================================================
Looks good.
================================================================================
usr/src/cmd/auto-install/checkpoints/dmm/test/dmm_build_test.py
================================================================================
1. Should the /tmp/dmm_buildtest_script be removed in the teardown method?
================================================================================
usr/src/cmd/auto-install/checkpoints/dmm/test/dmm_env_test.py
================================================================================
1. Should the /tmp/dmm_env_test.ksh script be removed in the teardown
method?
================================================================================
usr/src/cmd/auto-install/config/get_manifest
================================================================================
1. Correct the copyright information:
23 # Copyright 2009 Sun Microsystems, Inc. All rights reserved.
24 # Use is subject to license terms.
2. It seems that there should be a check to see if $manifestdir/default.xml
was partially created on error and removed if it was. This should
happen
in the if code block at lines 72-74.
================================================================================
usr/src/cmd/auto-install/svc/manifest-locator
================================================================================
Looks fine.
================================================================================
usr/src/cmd/rbac/user_attr.system%2Finstall%2Fauto-install
================================================================================
Looked at the Cdiffs file and it looks fine. The frames diff link does not
work. Very strange.
================================================================================
usr/src/lib/install_common/__init__.py
================================================================================
Looks fine.
================================================================================
usr/src/lib/install_manifest_input/__init__.py
================================================================================
1. Can remove the parenthesis in:
250 if (not incremental) or self.no_starting_data:
================================================================================
usr/src/lib/install_manifest_input/mim.py
================================================================================
1. Can remove the parenthesis in:
1028 if (idx > 0) and (brkt_active_lvl == 0):
2. Changing the names for left_eq, _none and right_eq might make the
following
more readable:
1078 left_eq, _none, right_eq = branch.partition("=")
Perhaps to something like:
attribute_name, equal, attribute_value = branch.partition("=")
3. Similar comment about:
1085 left_of_open_bkt, _none, right_of_open_bkt = \
1086 left_eq.partition("[")
1089 left_at, _at, right_at =
right_of_open_bkt.partition("@")
1108 preequal, _none, postequal = branch.partition("=")
Though no idea what naming might work for these.
================================================================================
usr/src/lib/install_manifest_input/mim_errors.py
================================================================================
Looks fine.
================================================================================
usr/src/lib/install_manifest_input/mim_utils.py
================================================================================
1. curr does not need to be initialized before the loop as it is initialized
in the loop and only used within the loop.
77 curr = None # Set to something not found in a string.
2. I am not 100% sure what is being searched for in
search_and_get_context().
What does the path look like that is being searched? Perhaps adding
that to the function comment would be helpful. It sure seems that there
should be a more efficient way to do the search.
================================================================================
usr/src/lib/install_manifest_input/process_dtd.py
================================================================================
1. The parenthesis can be removed in the following:
140 elif (line[idx] == splitchar) and (parend_level == 0):
2. Rewrite:
208 if (len(arg) != 1) or (not arg in "+*?1"):
As:
if len(arg) != 1 or arg not in "+*?1":
3. The parenthesis can be removed in the following:
636 if ((item.type == SchemaData.DDObj.T_ELEM) and
637 (find_this == item[0])):
656 elif ((item.type == SchemaData.DDObj.T_SEQ) or
657 (item.type == SchemaData.DDObj.T_OR)):
686 if ((item.qty == "?") and
687 (pt_list.type == SchemaData.DDObj.T_OR)):
688 search_state.treat_as_0_1_qty = pt_list
691 elif ((item.type == SchemaData.DDObj.T_SEQ) or
692 (item.type == SchemaData.DDObj.T_OR)):
4. The parenthesis within each "or" clause can be removed in the following:
717 if ((pt_list ==
search_state.first_list_gathered_from) and
718 (((pt_list.type == SchemaData.DDObj.T_SEQ) and
719 (item.qty != "?")) or
720 ((pt_list.type == SchemaData.DDObj.T_OR) and
721 (idx == last_pt_list_idx)))):
================================================================================
usr/src/lib/install_manifest_input/test/test_manifest_input_overlay.py
================================================================================
1. Should the created files be removed in a tearDown method for each test?
================================================================================
usr/src/lib/install_manifest_input/test/test_manifest_input_pathing.py
================================================================================
1. Should the created files be removed in a tearDown method for each test?
================================================================================
usr/src/lib/install_manifest_input/test/test_manifest_input_set_get_add.py
================================================================================
1. Should the created files be removed in a tearDown method for each test?
================================================================================
usr/src/lib/install_manifest_input/test/test_manifest_input_validate_commit.py
================================================================================
1. Should the created files be removed in a tearDown method for each test?
================================================================================
usr/src/lib/install_manifest_input/test/test_process_dtd.py
================================================================================
1. Should the created files be removed some place within the test?
================================================================================
usr/src/pkg/manifests/system-install-auto-install.mf
================================================================================
Looks fine.
================================================================================
On 03/29/11 08:46 PM, Jack Schwartz wrote:
Hi everyone.
Here is round 2 of the Derived Manifests code review.
It is a product of integrating all code review comments, from both
server and client sides, including lots of cleanup and some bug fixes.
Thanks to Drew, Matt, Dermot, Karen and Dave for reviewing the client
side. Thanks to John for reviewing the server side.
All reviewers please double check that the code changes address what
you mentioned in your emails. Please send any comments by Friday 4/1 COB.
Client side:
Delta: http://cr.opensolaris.org/~schwartz/110329.1/webrev.cli.1.2.diff/
(No webrev vs AI-CUD gate is available.)
Server side:
Delta: http://cr.opensolaris.org/~schwartz/110329.1/webrev.srv.1.3.diff/
Vs slim_source: http://cr.opensolaris.org/~schwartz/110329.1/webrev.srv.3/
Note: Server side unit tests still need to be reviewed. Please call
up the files with their "new" link from the server side delta webrev.
Notes:
1) I have tried to apply general comments to all files, even those not
directly mentioned in the comments. (For example, to use
os.path.exists() instead of os.access().)
2) Testing is ongoing. I have unit tested pretty thoroughly on X86.
Most code is generic, but I will be re-testing the SPARC-specific bits
ASAP. QE is also testing.
3) I plan on filing one last code review of bugfixes plus any other
changes resulting from this code review, in mid April a few days
before project completion.
Thanks,
Jack
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
