Ed,
Thanks for reviewing this, my comments are inline.
Edward Pilatowicz wrote:
hey jerry,
my comments are below. i still haven't reviewed the p2v script itself
but i'll get to that tommorow morning.
thanks
ed
----------
src/brand/pkgcreatezone
- '=' is a valid character to have in a path. you might want to try
validating the path, and only checking for '=' if the validation
fails.
I'll change it to work that way.
- iirc, in s10 p2v your require either the -u or -p option to be
specified. why don't you do the same here?
We do. This is in image_install at line 222.
- perhaps the usage message should be multiple lines?
install -h
install [-P publisher=uri [-c certificate_file] [-k key_file]] [-s|-v]
install {-a archive|-d path} {-u|-p} [-s|-v]
I'll change it to read like that.
- after a normal zone install, this script prints:
printf "$m_complete\n\n" ${SECONDS}
but if you install from an image, you print:
printf "$m_done\n"
why the difference?
I'll change image_install to match what pkgcreatzone does.
- it seems that trusted labeled zones runs this script. presumably
trusted won't need to support p2v. that means that the usage output
for installing trusted zones now be wrong. trusted should supply a
wrapper script around this script. could you file a bug on this issue?
Will do.
----------
src/brand/image_install
- i'm a little confused. i'm looking at the following file on
osol-200906 and i'm wondering where it comes from:
/usr/lib/brand/shared/common.ksh
it looks like the nevada version, but it's missing stuff.
specifically it's missing install_image(), which you define in
this file. what happened to the common version? (it also seems
like the version in this file doesn't support as many formats
as the nevada common.ksh version.)
This is because 2009.06 is based on nv b111 but the extra code
we need was integrated into b114. In any case, the next dev build
will sync to b116 so we'll have the support we need.
- this file seems to support flar images. but flar's are not supported
on opensolaris. if this was common code then i'd understand the
references to flars. but this isn't. so what's going on here?
(this may be related to my confusion above.)
Yes, this is common code.
- does '-d -' actually work? seems like it wouldn't since pkgcreatezone
just mounted a zfs dataset on the zoneroot before invoking this script.
It does if you first set up the dataset correctly but this is definitely
an area that needs to be improved. The idea is you could zfs recv the
dataset but then you'd have to manually set up the properties to make it
usable. I'll file a bug to track that.
- why do you have install_media and install_archive set to the same
value, and then use them interchangably? why not just have one
variable? same comment goes for install_media and source_dir.
This is an artifact from the original code in nv but I'll clean it up.
- why check [[ ! -d "$ZONEROOT" ]], pkgcreatezone just mounted this for
us.
This was derived from the nv code which doesn't have pkgcreatezone.
I'll fix this.
- would it be possible to generate $ipdcpiofile and $ipdpaxfile from a
single list of directories? (that way no one would miss the other
when doing bugfixes/updates.)
Unfortunately these use different formats. This is based on the
same code we have today in nv.
- rather than manually setting up zone mounts via get_fs_info() and
mnt_fs(), wouldn't it be better to do mount -f? or ready -f? and
have the zones framework take care of this all?
This is based on the nv code. Mount -f won't work since that mounts
under /a. We'd first have to fix the installer to work in the
scratch zone. If we want to do that I'd want to fix up the original nv
p2v installer first. There is no ready -f option.
- it seems weird that this script can print out $install_good, and
then still return failure after that.
I'll move that msg down.
Thanks again for your comments,
Jerry
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
