Hi Karen -
Thanks for the review. My responses are in line...
Don't worry about duplication. I appreciate the feedback.
thanks,
ginnie
On 02/14/11 04:15 PM, Karen Tung wrote:
Hi Ginnie,
I reviewed all the files in group 2. I also reviewed other Python
checkpoint files.
My comments are below. I didn't look at comments others provided. So,
if a certain item is already mentioned by somebody else, please just
point me
to your response to them.
General:
----------
- In all the execute() function in all ICT related checkpoints, there's
the same code there for retrieving the target_dir for the install target.
I think it would be cleaner to define a common function that can be
called by all the checkpoints, which just returns the install target
directory.
Darren also suggested this. I'll follow through on this.
- In the getting of the target value, you currently do the following:
self.target = self.doc.get_descendants(name=Target.DESIRED,
class_type=Target)[0]
if not self.target:
raise RuntimeError("No desired target nodes specified")
I think you can simplify it to doing:
self.target = self.doc.get_descendants(name=Target.DESIRED,
class_type=Target,
not_found_is_err=True)[0]
Then, you don't need to worry about checking for the return value to
make sure it is not None.
ok. Thanks.
- self.target.get_descendants(class_type=BE)[0], this call also needs
the not_found_is_err=True argument, especially you don't check the
return code.
ok.
- In all the __init__() functions, different variables are assigned the
None value. However, most of those variables are only used in the
execute()
function, and nothing else. I think it is unnecessary to initialize
all those variables.
ok. I'll take a look at that.
apply_sysconfig.py
----------------------
- line 69-70: The comment didn't say anything about profiles are being
validated, I think it should be mentioned.
I'll add something.
- line 104: I wonder whether it makes sense to validate the profiles
even for dry_run, since no change is done to the system, I think that
will be a helpful feature.
Ok....just so I'm clear, you are referring to validate lines 116 - 119,
correct?
And move that outside of the dry_run.
- line 106 and 112-123: The code here works. However, it is not very
Python.
Instead of calling os.listdir() and acting on the file names, I think
it will adhere to more Python programming style by doing a os.walk
through the directory, filtering out names, and performing action on
the files.
ok. I'll take a look at it.
cleanup_live_cd.py
--------------------
- General comment about the name of this file and checkpoint. Since
everything
done here is applicable after a cpio-based install of the live media, I
wonder whether a name that reflects that is more appropriate. Maybe
something
like "cleanup_cpio_install"??
Sure. I don't mind changing, if that seems more accurate.
- line 57, and lines 199-203: Dave just pushed changes for 7010834,
so, removing of coreadm.conf file is no longer needed.
ok. Thanks for letting me know. I'll follow up on that.
- lines 124-132: Since an installer might or might not
have specified any packages or files to remove, I think we should be
more cautious here and check for return values from all the
get_descendants() call before just using the values.
ok.
- line 138: the comment in the file says to "Creates /etc/mnttab if
it doesn't already exist and chmods it to the correct values". However,
there's no check here. Furthermore, the way it is coded now, if the file
does exist, opening it with 'w' will erase all of its existing content.
So, I think it is important that the file's existence be checked.
ok.
- line 163-193: Should we consider using IPS API instead of
calling the IPS command for these IPS related commands?
Yes. I think so. Drew reminded me that I need to do so.
- line 217: os.rmdir only works if the directory to be removed is empty.
Will we have situations where the directory is not empty?
Good point. I'll be sure to check on that.
device_config.py
-------------------
- line 97: remove the '2>&1'. Using solaris_install.Popen
implementation's
stdout and stderr to handle the output.
yes. Darren and Matt let me know about this.
generate_sc_profile.py:
------------------------
- line 84-113: This function does all the work
to figure out the keyboard layout value and then, sets it in
the self.keyboard_layout variable. While the way it is currently coded
works, but I don't think it is very good programming practice, and
makes the code harder to read. People who are reading the code
might wonder who sets the self.keyboard_layout variable, and they
have to search backwards. It would be much better for the value
that's determined from the ioctl calls to be returned and
set in the self.keyboard_layout variable.
ok.
- line 113: close not needed.
ok.
- line 195-196: You can optimize the code by opening the sc_profile_dst
before entering during the previous loop (lines195-191), and just
write the lines out without having to save them.
ok. I'll look this over.
- line 192: this is not needed since you are doing a "with"
ok.
- line 197: line also not needed
yep.
- line 187-190: if self.keyboard_layout == '', will this code make the
entry invalid? If self.keyboard is ' ', then, line 188 will determine
that it doesn't match, and the KBD_DEFAULT will be replaced with ' '.
You can add a check to make sure self.keyboard_layout is not ' ' before
you do the line.replace. Another option is to move lines 201-206 up
before line 184, and check that if self.keyboard_layout == ' ',
just copy self.sc_profile_src to self.sc_profile_dst, otherwise,
do the replace.
ok. thanks.
initialize_smf.py:
------------------
- line 127: add a debugging line here showing which profile is being
worked on?
ok. sounds good.
update_dumpadm.py:
--------------------
- lines 106-123: you can optimize this by opening
the self.dumpadmfile, dumpadmfile_dest at the same time.
Then, read from self.dumpadmfile, and write those lines that doesn't
contain the DUMPADM_SAVDIR.
Allright. I'll do so.
Thanks,
--Karen
On 02/08/11 11:38, Ginnie Wray wrote:
Hi -
I've need a code review for the ICT conversions. They are relatively
short, so I thought I would group them and ask a few people each take
a group.
Because this is short, I would appreciate feedback by February 16th.
If that doesn't work, let me know and I can accomodate you.
Webrev is located at:
http://cr.opensolaris.org/~ginnie/icts/
Thanks,
ginnie
Group 1:
usr/src/Makefile.master
usr/src/Targetdirs
usr/src/lib/Makefile
usr/src/lib/Makefile.targ
usr/src/lib/install_ict/Makefile
usr/src/lib/install_ict/__init__.py
usr/src/pkg/manifests/system-library-install.mf
usr/src/lib/install_ict/common.py
usr/src/lib/install_ict/apply_sysconfig.py
usr/src/lib/install_ict/test/test_apply_sys_config.py
usr/src/lib/install_ict/boot_archive.py
usr/src/lib/install_ict/test/test_boot_archive.py
Group 2:
usr/src/lib/install_ict/cleanup_live_cd.py
usr/src/lib/install_ict/test/test_cleanup_live_cd.py
usr/src/lib/install_ict/create_snapshot.py
usr/src/lib/install_ict/test/test_create_snapshot.py
usr/src/lib/install_ict/update_dumpadm.py
usr/src/lib/install_ict/test/test_updatedumpadm.py
Group 3:
usr/src/lib/install_ict/device_config.py
usr/src/lib/install_ict/test/test_device_config.py
usr/src/lib/install_ict/generate_sc_profile.py
usr/src/lib/install_ict/test/test_gen_sc_profile.py
usr/src/lib/install_ict/initialize_smf.py
usr/src/lib/install_ict/test/test_init_smf.py
_______________________________________________
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
