Hi Darren -
Thanks for the feedback. I've replied inline...
Ginnie
On 02/14/11 11:09 AM, Darren Kenny wrote:
Hi Ginnie,
I took a look at the ICT webrev, focusing on the non-test files.
In general, looking good, but I have some comments/suggestions for you
below...
General:
- Getting the BE
I know I sent you this originally, but on further thought the searching for
the BE in could probably be 'slightly' faster if we stop when we've found
the first (and only occurrence) by adding the max_count parameter to
get_descendants():
self.boot_env = self.target.get_descendants(class_type=BE, max_count=1)[0]
I'm also wondering if there would be some value to this part of the code
being shared between all checkpoints, not just in your ICT code, although
that's probably the main consumer of it...
OK. I'll change it and we can see what others say about sharing it across
all checkpoints.
- Use of subprocess.check_call()
It probably makes sense for us to use the new solaris_install.Popen that
Keith has provided rather than check_call() for a couple of reasons but the
main one being how things are logged.
Alternatively, maybe we also need a solaris_install.check_call()?
I'm adding this in.
- Other shared code
There is some shared code in the ICTs, especially in the __init__() and
start of the execute() methods - the progress estimation code is also common
at the moment - and in the case of short ICTs it might be shared too.
As such it might be useful to have a shared common class for ICTs, that did
the following:
- Define an __init__ that set up the doc, target and BE references.
BTW, you should be able to get a reference to the DOC in the __init__()
since the engine creates it only once, well before this is called.
ok. I'll fix this.
- Define a parse_doc() method that gets the above information from the DOC
and fills it in for you, and this would be called at the start of the
execute() method.
This would also help with the repeated code as mentioned above for
fetching the BE.
Thanks for the suggestion. I will do so.
apply_sysconfig.py:
- lines 96-99:
96 os.environ[Common.SVCCFG_DTD] = os.path.join(self.target_dir,
97 Common.SVC_BUNDLE)
98 os.environ[Common.SVCCFG_REPOSITORY] =
os.path.join(self.target_dir,
99 Common.SVC_REPO)
I think that these lines should probably be done inside the dry-run since
they are potentially changing the environment for subsequent checkpoints
(although I do realise that it's unlikely they will use it, but you never
know).
ok. I'll move them. I debated about whether or not they should be under
dry_run, so this is helpful feedback.
- lines 101-102:
101 self.sc_profile_dst = os.path.join(self.target_dir,
102 Common.PROFILE_DEST)
I don't think that sc_profile_dst needs to be an instance attribute, so you
could probably omit the 'self.' here.
ok.
boot_archive.py:
- lines 90-94:
90 if not dry_run:
91 # Run bootadm
92 cmd = [Common.BOOTADM, 'update-archive', '-R ',
93 self.target_dir, '2>&1']
94 subprocess.check_call(cmd)
(I know we mentioned this before in separate emails, but just want to ensure
it's recorded here too)
The space after the '-R' is a bug, and will fail.
I don't think we should use '2>&1' since it is useful to get the error
messages logged, which would happen if solaris_install.Popen is used.
yes. I've already taken care of this.
cleanup_live_cd.py:
- Several locations:
Its a nit really but there is a space between the string and .join call,
which I feel is misleading:
" " .join(cmd)
I think it would be clearer to have no space after the string.
ok. I'll remove.
- lines 155-157:
155 cmd = '(Common.CD %s&& find . -type f -print | \
156 Common.CPIO -pmu %s> /dev/null 2>& 1)&& Common.RM
-rf
%s' \
157 % (savedir, self.target_dir, savedir)
I think that the command here is very broken, since the values Common.XX are
not interpreted but remain as they are in the command, giving you something
like:
(Common.CD /a/save&& find . -type f -print | \
Common.CPIO -pmu /a> /dev/null 2>& 1)&& Common.RM -rf /a
as opposed to the expected
(cd /a/save&& find . -type f -print | \
cpio -pmu /a> /dev/null 2>& 1)&& rm -rf /a
So it should probably read as:
cmd = '(%s %s&& find . -type f -print | \
%s -pmu %s> /dev/null 2>& 1)&& %s -rf %s' \
% (Common.CD, savedir, Common.CPIO,
self.target_dir, Common.RM, savedir)
I'll be fixing this. Thanks for the suggestion.
- lines 191-197:
191 # Restore the original IPS default and
192 # set purging the IPS download cache to "False"
193 cmd = [Common.PKG, '-R', self.target_dir, 'set-property',
194 'flush-content-cache-on-success', 'False']
195 self.logger.debug("executing: %s" % " " .join(cmd))
196 if not dry_run:
197 subprocess.check_call(cmd)
As mentioned in a separate thread, this code should probably be moved to a
separate checkpoint since it's also needed by AI, which doesn't use this
CleanupLiveCD checkpoint.
yes. It's on my list of things to do.
common.py:
- line 58:
I reckon the "RM" command should be "/usr/bin/rm" not "rm", to be more
consistent with other commands also in here.
Thanks. Drew suggested removing it altogether and using os.unlink.
Once I sort out the cpio command, I'll see if that is necessary.
create_snapshot.py:
- No comments
yeah!
device_config.py:
- line 97:
97 cmd = [Common.DEVFSADM, '-R ', self.target_dir, ' 2>&1']
The space after the -R should be removed, and the '2>&1' also if you change
to using Popen.
Yes. Already taken care of.
generate_sc_profile.py:
- lines 185-192, and 201-206:
185 with open(self.sc_profile_src, "r+") as fh:
186 for line in fh:
187 if Common.KBD_DEFAULT in line and \
188 self.keyboard_layout != Common.KBD_DEFAULT:
189 line = line.replace(Common.KBD_DEFAULT,
190 self.keyboard_layout)
191 new_sc_profile.append(line)
192 fh.close()
...
201 if self.keyboard_layout == '':
202 self.logger.debug('Keyboard layout has not been identified')
203 self.logger.debug('It will be configured to %s.'%
Common.KBD_DEFAULT)
204 else:
205 self.logger.debug('Detected [%s] keyboard layout'
206 % self.keyboard_layout)
According to the debug statement at line 201, it would appear to be possible
that the keyboard_layout could be blank '', and should default to
KBD_DEFAULT if it is.
But, the above code doesn't take this into account and will replace the
default in the profile with ''...
I think the lines 201-206 should be before the looping code or even in the
end of the _get_keyboard_layout() method, and if it sees the keyboard_layout
as being '', then set it to Common.KBD_DEFAULT.
Thanks for catching that. I'll fix it.
- lines 184-199:
If self.keyboard_layout != Common.KBD_DEFAULT in the code, then this whole
try/except is essentially the equivalent of a:
shutil.copy2(self.sc_profile_src, self.sc_profile_dst)
as such, I wonder if we could have the if statement check this condition
first, if so, then call shutil.copy2(), otherwise process it in more detail
as in the try/catch.
Ok. I'll give this a try.
initialize_smf.py:
- lines 104-115:
104 self.sys_profile_dict = {Common.GEN_LTD_NET_XML:
105 os.path.join(self.target_dir,
106 Common.GENERIC_XML),
107 Common.NS_DNS_XML:
108 os.path.join(self.target_dir,
109 Common.NAME_SVC_XML),
110 Common.INETD_XML:
111 os.path.join(self.target_dir,
112 Common.INETD_SVCS_XML),
113 os.path.basename(Common.SC_PROFILE):
114 os.path.join(self.target_dir,
115 Common.PROFILE_SITE)}
It's a nit really, but I wonder could this be better formatted to make it
more readable? e.g.:
self.sys_profile_dict = {
Common.GEN_LTD_NET_XML:
os.path.join(self.target_dir, Common.GENERIC_XML),
Common.NS_DNS_XML:
os.path.join(self.target_dir, Common.NAME_SVC_XML),
Common.INETD_XML:
os.path.join(self.target_dir, Common.INETD_SVCS_XML),
os.path.basename(Common.SC_PROFILE):
os.path.join(self.target_dir, Common.PROFILE_SITE)}
I can do this, no problem.
- line 127:
127 os.unlink(value)
Again, we've said this in another thread, but just to bring together, we
should be checking whether the file exists first before we try unlink it,
otherwise it could fail when it doesn't need to.
yes.
Thanks,
Darren.
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
