[libvirt] [PATCH] phyp: Adding storage management driver

[Eduardo Otubo]

On 06/21/2010 05:05 PM, Eric Blake wrote:
> On 06/21/2010 12:32 PM, Eduardo Otubo wrote:
>>> Ouch.  If the result of the `` command substitution begins with 0, you
>>> have a problem with octal numbers.  Remember, $((010 + 1)) is not the
>>> same as $((10 + 1)).  Perhaps you can modify the sed commands used in
>>> your script to strip leading 0?
>>
>> Yes, I can strip the leading zero using sed, but I hardly believe that
>> would be a such a return. But better fix this now than in the client
>> screen. :)
>>
>>> And, rather than doing $(()) (or expr) to do +1 in the shell, where you
>>> have to worry about octal in the first place, why not just output the
>>> last value as-is (that is, drop "echo $((`" and "` +1 ))" from cmd),
>>> then do +1 in C code?
>>
>> This is an option, but I really like to isolate the most I can on the
>> shell side, returning the final value for the function. I've been
>> keeping this pattern over all the code, so did the same here
>
> I would much rather see it shift in the other direction - do as LITTLE
> work in shell as possible (since you have to carefully audit for
> exploits, and because it is SO expensive to fork processes), and as MUCH
> work in C as possible.  Shortcuts like 'cmd | head -n 1' =>  C processing
> are one of the few things where doing more in shell can actually be
> faster, as it can lead to much less I/O, and even stop a cmd with lots
> of output early (due to EPIPE/SIGPIPE) rather than wasting processing
> power in running cmd to completion when we only need the first line in C
> code.  But anything as complex as massaging octal strings into known
> binary is going to be orders of magnitude faster in C than in shell.

Yes, you're right on this point. I was just thinking on the easy side
for the one who writes the code. Parsing strings in C is not exactly an
easy job, mainly if you have all the facilities of a shell script on
hand. This surely will be a separate patch in the future.

>
>>> Also, '\ ' is not a portable sed escape sequence.  Did you mean to use
>>> the C string "s/\\\\ //g" for the shell line 's/\\ //g', in order to
>>> delete backslash-space sequences from the output?  (multiple instances
>>> of this pattern in your patch)
>>
>> No, I meant to use the C string 's/\\ //g' for the shell line 's/\ //g'
>> in order to delete white spaces.
>
> But that's my point.  Some versions of sed treat '\ ' as the single byte
> space, while others treat it as the two-byte sequence backslash-space.
> In short:
>    sed 's/\ //g'
> _is not portable_.  The only portable shell command lines for deleting
> spaces are:
>    sed 's/ //g'
>    sed s/\ //g
>
> That is, either quote the space using '' in shell, or quote the space
> using \ in shell, but do NOT escape the space for sed.
>
>>> Save a process:
>>>     ...|sed '1d'|sed '1d'
>>> is equivalent to:
>>>     ...|sed 2d

Ok, fixed.

The next patch is right away.

-- 
Eduardo Otubo
Software Engineer
Linux Technology Center
IBM Systems & Technology Group
Mobile: +55 19 8135 0885
eotubo at linux.vnet.ibm.com