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.
- iirc, in s10 p2v your require either the -u or -p option to be
specified. why don't you do the same here?
- 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]
- 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?
- 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?
----------
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 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.)
- does '-d -' actually work? seems like it wouldn't since pkgcreatezone
just mounted a zfs dataset on the zoneroot before invoking this script.
- 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.
- why check [[ ! -d "$ZONEROOT" ]], pkgcreatezone just mounted this for
us.
- 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.)
- 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?
- it seems weird that this script can print out $install_good, and
then still return failure after that.
On Wed, Jun 10, 2009 at 01:18:01PM -0600, Jerry Jelinek wrote:
> I need another code review for the p2v support for
> zones using IPS. This is bug:
>
> 6793 p2v support for ipkg-branded zones
>
> This was reviewed once before, a couple of months ago,
> but I didn't finish addressing the comments in time for
> 2009.06. The main differences that should be reviewed
> now are the new code in src/brand/p2v to fix the
> pkg variant. These are lines 201-228. This is
> a temporary solution until IPS supports changing
> the variant.
>
> The other changes that should be looked at are
> in src/brand/pkgcreatezone since I had to merge
> this file and update it. In particular, the
> -a option behavior will be changing with this
> integration. Also, the src/brand/image_install
> code has been updated to correspond with the native
> brand code in ON. This code depends on ON code in b114
> so I won't be putting back until I know we're syncing
> up with that build or later.
>
> The updated webrev is at:
>
> http://cr.opensolaris.org/~gjelinek/webrev.6793/
>
> Thanks,
> Jerry
> _______________________________________________
> pkg-discuss mailing list
> [email protected]
> http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
