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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org