Hi Karen,
On Wed, 1 Dec 2010, Karen Tung wrote:
Hi Alok and Drew,
Here are my comments from reviewing the updated code:
---------------------------------
boot_archive_archive.py:
---------------------------------
251-253, 258-260, 263-265, 419-420: You are hard coding the transfer actions
and types strings
here. Those are now defined as constants in solaris_install.transfer.info,
please use the constants.
Changed.
---------------------------------
boot_archive_configure.py:
---------------------------------
420-430: I think it is better to use the "UserattrFile" class from
pkg.cfgfiles than to manipulate
the /etc/user_attr file yourself.
Yep, I've changed the code to now use the UserattrFile class.
The following has been added:
uafile = UserattrFile(self.ba_build)
# convert root to a role
rootentry = uafile.getvalue({'username' : 'jack'})
rootattrs = rootentry['attributes']
rootattrs['type'] = ['role']
rootentry['attributes'] = rootattrs
uafile.setvalue(rootentry)
# give jack administrator profile
jackattrs = dict({'roles' : ['root'],
'profiles' : ['Software Installation']})
jackentry = dict({'username' : 'jack', 'attributes' : jackattrs})
uafile.setvalue(jackentry)
# write out the etc/user_attr file
uafile.writefile()
412: nit. The name of this function is called "configure_user_attr", but
this function does more
than configuring user_attr. It also configures the sudoers file. I think a
more general name
should be used.
This function has been renamed to configure_user_attr_and_sudoers.
---------------------------------
pkg_mg_mod.py:
---------------------------------
185-191, 222-227: Is there a reason the stderr is being ignored. It would be
useful to log stderr into the
log file too, if any.
Yes, the only message that mkisofs(8) spits out to stderr is
a message along the lines of "Warning: Creating file-system
that does not conform to ..". It seemed to us that by sending
this message to /dev/null, we wouldn't print it to the console
and thus startle the user with the warning.
Much of the useful error messaging from mkisofs is transmitted
to stdout so I don't think we're losing much in the way of
error messages.
264-267: I think it would be more efficient to do the equivalent of this in
Python than doing it
with calling the find command. In fact, this used to be in Python, when I
ported the code
to a finalizer script, I changed it to a command since I was implementing a
ksh script.
Yep, I've changed it with the following:
content_list = []
for root, dirs, files in os.walk("."):
for f in files:
if not f.endswith(".zlib") and not f.endswith(".image_info") \
and not f.endswith("boot_archive") and not \
f.endswith(".livecd-cdrom-content"):
content_list.append(os.path.join(root, f))
for d in dirs:
content_list.append(os.path.join(root, d))
with open(".livecd-cdrom-content", "w") as fh:
for entry in content_list:
fh.write(entry + "\n")
---------------------------------
pre_pkg_img_mod.py:
---------------------------------
348: is it necessary to generate the image size for the AI image? AI doesn't
use
the cpio based transfers, so, I think it is not needed.
Removed.
---------------------------------
dc_livecd.xml:
---------------------------------
line 78: should there be a comment here with a warning telling ppl to not add
anything
after the "with screen reader" grub entry, like the current Live CD manifest?
Added.
Thanks for a second round of review!
Alok
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
