[OpenIndiana-discuss] Fwd: 2nd round - Bug 1204 zoneadm cannot create clone of zone from snapshot

[Jim Klimov]

Ok, thank you for the review. Replies also inline below :)

On 2013-11-15 00:20, Cedric Blancher wrote:
> The first thing I noticed is: Illumos has a POSIX shell but its not
> used as such. Further comments are inline within the patch diff:

I was also confused by the different constructs being used in the
original file, but left them as such. I believe the performance
differences of equivalent syntax blocks are so minor in comparison
with cloning, etc. (HDD IO) that this is more a matter of beauty
of the code than its functional characteristics ;)

>
> --- /usr/lib/brand/ipkg/clone.orig      2011-09-12 15:01:44.000000000 +0400
> +++ /usr/lib/brand/ipkg/clone   2013-11-13 05:31:27.756698164 +0400
> @@ -45,9 +45,16 @@
>   ROOT="rpool/ROOT"
>
>   # Other brand clone options are invalid for this brand.
> -while getopts "R:z:" opt; do
> +while getopts "R:s:z:" opt; do
>          case $opt in
>                  R)      ZONEPATH="$OPTARG" ;;
> +               s)      case "$OPTARG" in
> +                       *@*)    SNAPNAME="`echo "$OPTARG" | sed 's/^[^@]*@//'`"
>
> You can use sed 's/^[^@]*@//' <<<"$OPTARG" instead of the pipe.
>
> On a 2nd thought the shells ${var/string/repl} operator could replace
> this completely if you could explain to me what you wish to do here.

New patch also added this:
               REQUESTED_DS="`echo "$OPTARG" | sed 's/\([^@]*\)@.*$/\1/`"

The idea in both cases is to, basically, either chop off the characters
before (and including) the "@" separator, or to leave them in place,
thus splitting the parameter into dataset and snapname. I agree that
there are many ways to skin a cat, but in my practice this one is more
portable and even if performance suffers, in this rarely executed code
path it matters not... not much... no?

> +                               echo "WARNING: Ignoring dataset, using
> only snapshot name"
>
> Shouldn't this go to stderr?

Good catch, probably should have, but is moot now (no longer ignored).

> +                               ;;
> +                       *)      SNAPNAME="$OPTARG" ;;
> +                       esac
> +                       ;;
>                  z)      ZONENAME="$OPTARG" ;;
>                  *)      fail_usage "";;
>          esac
> @@ -96,19 +103,21 @@
>   /usr/sbin/zfs create -o mountpoint=legacy -o zoned=on $zpds/$zpname/ROOT
>
>   # make snapshot
> -SNAPNAME=${ZONENAME}_snap
> -SNAPNUM=0
> -while [ $SNAPNUM -lt 100 ]; do
> -       /usr/sbin/zfs snapshot $ACTIVE_DS@$SNAPNAME
> -        if [ $? = 0 ]; then
> -                break
> -       fi
> -       SNAPNUM=`expr $SNAPNUM + 1`
> -       SNAPNAME="${ZONENAME}_snap$SNAPNUM"
> -done
> +if [ x"$SNAPNAME" = x ]; then
> +       SNAPNAME=${ZONENAME}_snap
> +       SNAPNUM=0
> +       while [ $SNAPNUM -lt 100 ]; do
>
> for ((SNAPNUM=0; SNAPNUM < 100;SNAPNUM++)) would be good

I only added tabs, did not change original code

> +               /usr/sbin/zfs snapshot $ACTIVE_DS@$SNAPNAME
> +               if [ $? = 0 ]; then
>
> if (( $? == 0 )); then

ditto, although this very syntax is used near the end of the file

> +                       break
> +               fi
> +               SNAPNUM=`expr $SNAPNUM + 1`
> +               SNAPNAME="${ZONENAME}_snap$SNAPNUM"
> +       done
>
> -if [ $SNAPNUM -ge 100 ]; then
> -       fail_fatal "$f_zfs_create"
> +       if [ $SNAPNUM -ge 100 ]; then
> +               fail_fatal "$f_zfs_create"
> +       fi
>   fi
>
>
> Ced
> --
> Cedric Blancher <cedric.blancher at gmail.com>
> Institute Pasteur
>
>