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

[Eric Blake]

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.

>> 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
> 
> This is an odd behaviour. On my bash, (Ubuntu 10.04), sed '2d' and sed
> '1d'|sed '1d' removes the second line of the stream. But the on the
> HMC, sed '1d'|sed '1' removes the first two lines of the stream and sed
> '2d' removes the second line. And what I need is to remove the first
> two lines, so keeping sed '1d'|sed '1d'.

My apologies.  I mistyped the equivalence (easy to do with complicated
sed scripts).
   ...|sed '1d'|sed '1d'
is really equivalent to:
   ...|sed 1,2d

(2d strips just the second line, but 1,2d strips the range between the
first and second lines).  Sorry for my confusion.

> All the commands are tested and studied directly on the HMC/VIOS and
> IVM befores inserting on C code, so they really work :) All the issues
> you pointed were fixed in all worldwide code (not just the storage
> driver)
> 
> Will send another patch right away after reading all the comments and
> making all the properly fixes.

Looking forward to it.

> 
> Thanks for all the comments!

You're welcome.

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20100621/347e756e/attachment-0001.sig>