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

[Cedric Blancher]

On 15 November 2013 00:37, Jim Klimov <jimklimov at cos.ru> wrote:
> 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 ;)

OK

I would prefer the for ((; ; )) syntax over the x=0;while ... ; x=x+1
; done syntax because its IMHO more readable for people coming from C.
The current script is like an obfuscation contest, and a pain to read.
IMHO there is no need to inflict that on people.

>
>
>>
>> --- /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?

Well, for ksh there is .sh.match which exactly does that. But, in any
case, for any way to skin the felis silvestris catus, a comment would
be good.

Oh, and you want .+, not .* because * matches zero or more characters
while + matches one or more characters.

>
>
>> +                               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).

OK

Ced
-- 
Cedric Blancher <cedric.blancher at gmail.com>
Institute Pasteur