On 15 November 2013 00:37, Jim Klimov <jimkli...@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.blanc...@gmail.com>
Institute Pasteur
_______________________________________________
OpenIndiana-discuss mailing list
OpenIndiana-discuss@openindiana.org
http://openindiana.org/mailman/listinfo/openindiana-discuss
