Mike,

Thanks for reviewing this and sending your comments.
I'll make the changes to use mktemp.  Responses to
your other comments are in-line.

Mike Gerdts wrote:
> /usr/src/lib/brand/native/zone/image_install.ksh

> 
>  290         typeset line=$(grep "files_compressed_method" $ident)
>  301         typeset line=$(grep "files_archived_method" $ident)
> 
> Perhaps change the pattern to do a more exact match (e.g.
> "^files_archived_method=") to avoid confusion when someone creates a
> flar with a really odd content_description.

OK.

>  259                 if (system(cmd) != 0) {
>  260                         printf("%s\n", cmd);
>  261                         exit 1;
>  262                 }
> 
>  276         sort -r $fstmpfile | nawk -v zonepath=$zonepath '{
>  277                 cmd="/usr/sbin/umount " zonepath "/root" $1
>  278                 if (system(cmd) != 0) {
>  279                         printf("%s\n", cmd);
>  280                 }
>  281         }'
> 
> In lines 260 and 279 it looks like a cryptic error message goes to
> stdout.  Since stderr from mount is redirected to $logfile, it seems
> as though this cryptic error message will have no context.

I'll fix the logging.

> 
> /usr/src/lib/brand/native/zone/p2v.ksh
> 
>   85                 msg=$(gettext "Shutting down zone $ZONENAME...")
> 
> Perhaps I misunderstand, but doesn't this imply a translation would
> need to exist for every possible $ZONENAME?

I don't think so, since this would be left as a variable
in the translation, but I'll change it to use the same
formatting as the other msgs with substitution.

>  334         for i in 0 1 2 3 4 5 6 7 8 9
>  335         do
>  336                 [[ -r $ZONEROOT/etc/svc/volatile/repository_door
> ]] && break
>  337                 sleep 3
>  338         done
>  339
>  340         if [[ $i -eq 9 ]]; then
>  341                 error "$e_nosmf"
>  342                 return 1
>  343         fi
> 
> If repository_door came available after the last 3 second sleep it may
> return with an unnecessary error.  Perhaps 340 should check for a
> readable repository_door again.

I'll change that.

>  427 #
>  428 # Remove well-known pkgs that do not work inside a zone.
>  429 #
>  430 rm_pkgs()
>  431 {
>  432         PKG_LIST='
>  433                 VRTSvxfs
>  434                 VRTSvxvm'
> 
> Is there also something that should be done with packages with
> SUNW_PKG_ALLZONES=true but the package is not installed in the global
> zone?

No, update-on-attach deals with that case.

>  596 # Before booting the zone we may need to create a few mnt points, just in
>  597 # case they don't exist for some reason.
>  598 #
>  599 # Whenever we reach into the zone while running in the global zone we
>  600 # need to validate that none of the interim directories are symlinks
>  601 # that could cause us to inadvertently modify the global zone.
>  602 verbose "$v_mkdirs"
>  603 if [[ ! -f $ZONEROOT/tmp && ! -d $ZONEROOT/tmp ]]; then
>  604         /usr/bin/mkdir -p $ZONEROOT/tmp || exit 1
>  605         /usr/bin/chmod 1777 $ZONEROOT/tmp || exit 1
>  606 fi
> 
> This makes the mount point for /tmp world writable, when what you
> really want is to have these permissions on the root of the file
> system mounted at /tmp.

Yes, when you mount a file system, the top level directory assumes
the mode of the mount point.

>  In the event that tmpfs fails to mount at
> /tmp for some reason, the administrator will have no early clues that
> users are filling up / when they should really be filling up swap. :/

That will always be the case, independent of if you're installing
an image or just booting the standalone zone.

>  619 if [[ ! -f $ZONEROOT/dev && ! -d $ZONEROOT/dev ]]; then
>  620         /usr/bin/mkdir -p $ZONEROOT/dev || exit 1
>  621         /usr/bin/chmod 755 $ZONEROOT/proc || exit 1
> 
> Line 621: s/proc/dev/
> 
> Or better yet (no path to encourage use of ksh built-in if available)
> 
> mkdir -m 755 -p $ZONEROOT/dev

I'll change this.

>  698         #
>  699         # We're sys-unconfiging the zone.  This will halt the zone, 
> however
>  700         # there are problems with sys-unconfig and it usually
> hangs when the
>  701         # zone is booted to milestone=none.  This is why we
> previously halted
>  702         # the zone.  We now boot to milestone=single-user.  Again, the
>  703         # sys-unconfig can hang if the zone is still in the process of
>  704         # booting when we try to run sys-unconfig.  Wait until the boot 
> is
>  705         # done, which we do by checking for sulogin, or waiting 30 
> seconds,
>  706         # whichever comes first.
>  707         #
> 
> Wouldn't it be more correct to wait for
> svc:/milestone/single-user:default to be online?

I'll change that.

>  720         for i in 0 1 2 3 4 5 6 7 8 9
>  721         do
>  722                 pgrep -z $ZONENAME sulogin >/dev/null 2>&1 && break
>  723                 sleep 3
>  724         done
>  725
>  726         if [[ $i -eq 9 ]]; then
>  727                 verbose "$e_nosmf"
>  728         fi
> 
> Wasted final sleep.  726 should pgrep again.

I'll change that.

Thanks again for reviewing this,
Jerry

_______________________________________________
zones-discuss mailing list
zones-discuss@opensolaris.org

Reply via email to