Re: [libvirt] [PATCH] phyp: Adding storage management driver
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 619 bytes)