Re: [zones-discuss] native p2v code review
> > Thanks for looking at this. I'll take a look > > at each of your recommendations. One factor > > is that we plan on backporting this to S10, so > > I want to keep things working on that release > > with a minimum of changes. > > Erm... the warnings returned by $ ksh93 -n # apply to ksh88 > (e.g. /usr/bin/ksh in Solaris <= 11/B106 and /usr/xpg4/bin/sh), too. > This features does not enforce any feature outside the POSIX shell > standard (but checks - if they are used - ksh88&&ksh93-specific > features, too). > Or short: These issues should be fixed for Solaris 10, too (as a > side-effect you get more performance for the script (even on Solaris > 10), too). I didn't see any response to my question about whether the OpenSolaris (or perhaps just ON) community had reached agreement on the advice offered by shlint. I could name several dozen personal best practices that I wish every C program in ON followed; that doesn't give me the right to just update a private version of cstyle and start enforcing those things each time someone posts a code review. Please start with getting consensus on shlint. If you're unable to get consensus with what you have, then you need to modify your proposal until you do. -- meem ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] native p2v code review
Jerry Jelinek wrote: > Thanks for looking at this. I'll take a look > at each of your recommendations. One factor > is that we plan on backporting this to S10, so > I want to keep things working on that release > with a minimum of changes. Erm... the warnings returned by $ ksh93 -n # apply to ksh88 (e.g. /usr/bin/ksh in Solaris <= 11/B106 and /usr/xpg4/bin/sh), too. This features does not enforce any feature outside the POSIX shell standard (but checks - if they are used - ksh88&&ksh93-specific features, too). Or short: These issues should be fixed for Solaris 10, too (as a side-effect you get more performance for the script (even on Solaris 10), too). Bye, Roland -- __ . . __ (o.\ \/ /.o) roland.ma...@nrubsig.org \__\/\/__/ MPEG specialist, C&&JAVA&&Sun&&Unix programmer /O /==\ O\ TEL +49 641 3992797 (;O/ \/ \O;) ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] native p2v code review
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
Re: [zones-discuss] native p2v code review
Roland, Thanks for looking at this. I'll take a look at each of your recommendations. One factor is that we plan on backporting this to S10, so I want to keep things working on that release with a minimum of changes. Thanks again, Jerry ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] native p2v code review
On Tue, Jan 20, 2009 at 6:33 PM, Jerry Jelinek wrote:
> I have a first cut at p2v for native zones.
> This is:
>
> 6667924 physical to virtual utility for native zones
> PSARC 2008/766 native zones p2v
>
> There is a webrev at:
>
> http://cr.opensolaris.org/~gjelinek/webrev.p2v/
I only had a chance to look over image_install.ksh and p2v.ksh.
/usr/src/lib/brand/native/zone/image_install.ksh
198 zonecfg -z $zonename info inherit-pkg-dir | \
199 nawk -v ipdcpiof=/var/tmp/$zonename.ipd.cpio.$$ \
200 -v ipdpaxf=/var/tmp/$zonename.ipd.pax.$$ '{
...
610 logfile="/var/tmp/$zonename.install.$$.log"
611 zone_logfile="${logdir}/$zonename.install.$$.log"
612 exec 2>"$logfile"
613 screenlog "$install_log" "$logfile"
614
615 fstmpfile=/tmp/fsinfo.$zonename.$$
...
697 # Make sure we always have a file holding the IPDs (even if empty)
698 touch /var/tmp/$zonename.ipd.cpio.$$
699 touch /var/tmp/$zonename.ipd.pax.$$
(and maybe others)
Please use mktemp(1) to create safe temporary files and directories.
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.
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.
/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?
262 }' $ZONEROOT/etc/vfstab >/tmp/vfstab.$$
Please use mktemp(1) or at least write the file to a directory that is
not world-writable.
322 echo $k >> \
323
/var/tmp/$ZONENAME.$$.smf
mktemp(1)
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.
345 # Get a list of the svcs that exist in the zone.
346 /usr/sbin/zlogin -S $ZONENAME /usr/bin/svcs -aH | \
347 /usr/bin/nawk '{print $3}' >/var/tmp/$ZONENAME.$$.instsmf
mktemp(1)
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?
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. 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. :/
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
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,
Re: [zones-discuss] native p2v code review
> "p2v.ksh" triggers lots of warnings when scanned via $ shlint p2v.ksh # Has consensus been reached in the OpenSolaris community on the advice offered by shlint? If not, it would seem that needs to be done first. -- meem ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] native p2v code review
Roland Mainz wrote: > Jerry Jelinek wrote: > > I have a first cut at p2v for native zones. > > This is: > > > > 6667924 physical to virtual utility for native zones > > PSARC 2008/766 native zones p2v > > > > There is a webrev at: > > > > http://cr.opensolaris.org/~gjelinek/webrev.p2v/ > > Quick look at http://cr.opensolaris.org/~gjelinek/webrev.p2v/on.patch > (patch code is quoted with "> "): [snip] BTW: The whole use of /usr/bin/gettext is unneccesary assuming you use ksh93. ksh93 has builtin l10n support via the new $"..."-style literals (see http://www.opensolaris.org/os/project/shell/shellstyle/#use_builtin_localisation_support), e.g. any string literal within $"..." will be automatically passed through a l10n catalog of the same name as the calling script (this is much simpler than "gettext" and has the advantage that the shell does all the catalog/message caching for you, too). The l10n messages can be extrated via $ shcomp -D # or $ ksh93 -D # (for systems where "shcomp" is not available). Bye, Roland -- __ . . __ (o.\ \/ /.o) roland.ma...@nrubsig.org \__\/\/__/ MPEG specialist, C&&JAVA&&Sun&&Unix programmer /O /==\ O\ TEL +49 641 3992797 (;O/ \/ \O;) ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] native p2v code review
Jerry Jelinek wrote: > > I have a first cut at p2v for native zones. > This is: > > 6667924 physical to virtual utility for native zones > PSARC 2008/766 native zones p2v > > There is a webrev at: > > http://cr.opensolaris.org/~gjelinek/webrev.p2v/ Quick look at http://cr.opensolaris.org/~gjelinek/webrev.p2v/on.patch (patch code is quoted with "> "): > +# Check for a non-empty root. > +# > +cnt=`ls $install_root | wc -l` Plese use $(...) instead of `...` (see http://www.opensolaris.org/os/project/shell/shellstyle/#use_posix_subshell_syntax). > +if [ $cnt -ne 0 ]; then Please use arithmetric expressions to compare numbers, e.g. -- snip -- if (( cnt != 0 )) ; then -- snip -- (see http://www.opensolaris.org/os/project/shell/shellstyle/#use_posix_arithmetic_expressions) > +screenlog "$root_full" "$install_root" > +exit $int_code > +fi "p2v.ksh" triggers lots of warnings when scanned via $ shlint p2v.ksh # (or as substitute use $ ksh93 -n p2v.ksh #): -- snip -- $ shlint p2v.ksh p2v.ksh: warning: line 48: `...` obsolete, use $(...) p2v.ksh: warning: line 48: `...` obsolete, use $(...) p2v.ksh: warning: line 65: `...` obsolete, use $(...) p2v.ksh: warning: line 65: `...` obsolete, use $(...) p2v.ksh: warning: line 75: `...` obsolete, use $(...) p2v.ksh: warning: line 75: `...` obsolete, use $(...) p2v.ksh: warning: line 84: -ne within [[...]] obsolete, use ((...)) p2v.ksh: warning: line 143: `...` obsolete, use $(...) p2v.ksh: warning: line 145: -ne within [[...]] obsolete, use ((...)) p2v.ksh: warning: line 152: `...` obsolete, use $(...) p2v.ksh: warning: line 153: -ne within [[...]] obsolete, use ((...)) p2v.ksh: warning: line 158: `...` obsolete, use $(...) p2v.ksh: warning: line 159: -ne within [[...]] obsolete, use ((...)) p2v.ksh: warning: line 164: `...` obsolete, use $(...) p2v.ksh: warning: line 185: `...` obsolete, use $(...) p2v.ksh: warning: line 222: -eq within [[...]] obsolete, use ((...)) p2v.ksh: warning: line 264: -eq within [[...]] obsolete, use ((...)) p2v.ksh: warning: line 299: `...` obsolete, use $(...) p2v.ksh: warning: line 303: `...` obsolete, use $(...) p2v.ksh: warning: line 313: `...` obsolete, use $(...) p2v.ksh: warning: line 340: -eq within [[...]] obsolete, use ((...)) p2v.ksh: warning: line 349: `...` obsolete, use $(...) p2v.ksh: warning: line 371: -eq within [[...]] obsolete, use ((...)) p2v.ksh: warning: line 379: -eq within [[...]] obsolete, use ((...)) p2v.ksh: warning: line 387: `...` obsolete, use $(...) p2v.ksh: warning: line 417: `...` obsolete, use $(...) p2v.ksh: warning: line 477: `...` obsolete, use $(...) p2v.ksh: warning: line 490: `...` obsolete, use $(...) p2v.ksh: warning: line 546: -lt within [[...]] obsolete, use ((...)) p2v.ksh: warning: line 548: -gt within [[...]] obsolete, use ((...)) p2v.ksh: warning: line 639: -ne within [[...]] obsolete, use ((...)) p2v.ksh: warning: line 676: -ne within [[...]] obsolete, use ((...)) p2v.ksh: warning: line 691: -ne within [[...]] obsolete, use ((...)) p2v.ksh: warning: line 713: -ne within [[...]] obsolete, use ((...)) p2v.ksh: warning: line 726: -eq within [[...]] obsolete, use ((...)) p2v.ksh: warning: line 732: -ne within [[...]] obsolete, use ((...)) -- snip -- "image_install.ksh" has similar problems: -- snip -- image_install.ksh: warning: line 37: `...` obsolete, use $(...) image_install.ksh: warning: line 37: `...` obsolete, use $(...) image_install.ksh: warning: line 50: `...` obsolete, use $(...) image_install.ksh: warning: line 50: `...` obsolete, use $(...) image_install.ksh: warning: line 60: `...` obsolete, use $(...) image_install.ksh: warning: line 60: `...` obsolete, use $(...) image_install.ksh: warning: line 159: `...` obsolete, use $(...) image_install.ksh: warning: line 164: `...` obsolete, use $(...) image_install.ksh: warning: line 326: -ne within [[...]] obsolete, use ((...)) image_install.ksh: warning: line 336: Invariant test image_install.ksh: warning: line 347: '=' obsolete, use '==' image_install.ksh: warning: line 355: '=' obsolete, use '==' image_install.ksh: warning: line 359: '=' obsolete, use '==' image_install.ksh: warning: line 366: '=' obsolete, use '==' image_install.ksh: warning: line 385: '=' obsolete, use '==' image_install.ksh: warning: line 390: '=' obsolete, use '==' image_install.ksh: warning: line 396: '=' obsolete, use '==' image_install.ksh: warning: line 409: '=' obsolete, use '==' image_install.ksh: warning: line 414: `...` obsolete, use $(...) image_install.ksh: warning: line 414: `...` obsolete, use $(...) image_install.ksh: warning: line 418: '=' obsolete, use '==' image_install.ksh: warning: line 440: '=' obsolete, use '==' image_install.ksh: warning: line 448: -ne within [[...]] obsolete, use ((...)) image_install.ksh: warning: line 495: -lt within [[...]] obsolete, use ((...)) image_install.ksh: warning: line 576: `...` obsolete, use $(...) image_install.ksh: warning: line 576: `...` obsolete, use $(...) image_install.ksh: warning: line
[zones-discuss] native p2v code review
I have a first cut at p2v for native zones. This is: 6667924 physical to virtual utility for native zones PSARC 2008/766 native zones p2v There is a webrev at: http://cr.opensolaris.org/~gjelinek/webrev.p2v/ I already have some comments from Ed and I'll be making a few small changes, but I'd like to see if anyone else has any input. Feel free to send me any comments or questions. Thanks, Jerry ___ zones-discuss mailing list zones-discuss@opensolaris.org
