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 scriptname # 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 - ksh88ksh93-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
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
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 21 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
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 581: -a obsolete, use -e
Re: [zones-discuss] native p2v code review
On Tue, Jan 20, 2009 at 6:33 PM, Jerry Jelinek gerald.jeli...@sun.com 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, the 703 #