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@xxxxxxxxxx +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list