Re: [LTP] [PATCH] Fix some bashisms

Jiří Paleček
2009-07-07T01:12:47Z

On Tue, 07 Jul 2009 01:33:29 +0200, Garrett Cooper <yanegomi@gmail.com>
wrote:

> On Mon, Jul 6, 2009 at 3:47 PM, Jiri Palecek<jpalecek@web.de> wrote:
>> On Monday 06 July 2009 19:45:08 Garrett Cooper wrote:
>>> Jiri,
>>>     This patch looks ok for the most part, but here are some comments:
>>>
>>> 1. if [[ -z "$vnet0" -z "$vnet1" ]] => if [ -z "$vnet0" ] || [ -z
>>> "$vnet1" ]
>>>
>>> It's better read as the following, IMO:
>>>
>>> [ -z "$vnet0" -o -z "$vnet1" ]
>>
>> I think this is another "I like this more than that, because this is
>> cool and that sucks" debate. If other think test's "-o" should be used
>> in this case, I would make the change. After all, this can be done by
>> hand directly on the patch...
>
> Why call [(1) twice?

Does it (measurably) matter? Do the shell scripts contribute a big part to
the time the tests take?

>>> 2. All numeric test(1) comparisons could and should be switched from:
>>>
>>> command
>>> if [ $? = 0 ]; then
>>>
>>> to:
>>>
>>> if command; then
>>>
>>> for brevity.
>>
>> Why should them be changed? This is only a cosmetic change, which would
>> make the patch unnecessarily larger (when it's already a little too
>> large IMHO). If it could be in the former form till now, I think it can
>> stay there a few months/years.
>>
>> IMHO this usage is still much better than
>>
>>  command; RC=$?
>>  if [ $RC = 0 ] ...
>>  ... RC is not read later ...
>>
>> or (incorrect)
>>
>>  command || RC=$?
>>  if [ $RC = 0 ] ...
>
> Yes, I agree with what you said about the above usage, but if you're
> not using $? for other than just the [ $? -ne 0 ], then I wouldn't
> even bother doing that, because if command; then saves a fork-exec,
> and an additional line in the source. As long as it's readable and
> doesn't expand multiple lines with a trailing \, you're fine.

Are you talking only about efficiency and bash scripts? :-)

Anyway, if you want to save the fork-exec, you can just use a shell with a
test builtin (ksh, bash, dash ... and I believe many others do).

If you are concerned about readability, I suggest considering another
alternative:

command || {
do_something_on_error
}

command && {
do_something_when_ok
}

Of coures, usable only when there's no "else" branch.

Still, I think the original form is correct and quite straightforward. I
would probably not use the extra test myself, but I don't feel like it
needs changing. Just looking at the patch, I see places where the check is
done really suspiciously:

tst_resm TINFO "Test #1: changing mtu size of eth0:1 device."

- ip link set eth0:1 mtu 300 &>$LTPTMP/tst_ip.err
+ ip link set eth0:1 mtu 300 >$LTPTMP/tst_ip.err 2>&1
if [ $RC -ne 0 ]
then
tst_res TFAIL $LTPTMP/tst_ip.err \

I think this needs much more attention than "calling [ twice" or "extra
line in the script".

>>> 3. About &>
>>>
>>>    Redirecting Standard Output and Standard Error
>>>        Bash allows both the standard output (file descriptor 1) and
>>> the standard error output (file descriptor 2) to be redirected to the
>>> file whose name is the expansion of word with this construct.
>>>
>>>        There are two formats for redirecting standard output and
>>> standard error:
>>>
>>>               &>word
>>>        and
>>>               >&word
>>>
>>> The output redirection above captures all output (and most of the time
>>> you're capturing stdout, but losing stderr to the operating console,
>>> which can be undesirable). >& is more portable IIRC (it works with
>>> many ash variants, like FreeBSD's sh -- for sure -- and Solaris's sh
>>> -- IIRC), which makes it more ideal than &>.
>>
>> Still, it isn't POSIX, it doesn't work with dash and actually causes
>> pain. See:
>>
>>  cmd &> fn
>>
>> POSIX: run cmd in background, write empty file fn
>> BASH: run cmd (foreground), send its (normal & error) output to file fn
>>
>>  cmd >& fn
>>
>> POSIX: syntax error
>> BASH: same as above
>>
>> But I don't see the point you are making here - do you have a system,
>> where the POSIX way
>>
>>  cmd > fn 2>&1
>>
>> doesn't work? Or did you see something else in the patch?
>
> This changes the behavior of the patch though. That's the point ;)...

How does it change the semantics? It redirects standard output to fn, and
standard error to standard output (which is redirected to fn). Is there
anything else it should do?

Regards
Jiri Palecek

------------------------------------------------------------------------------
Enter the BlackBerry Developer Challenge
This is your chance to win up to $100,000 in prizes! For a limited time,
vendors submitting new applications to BlackBerry App World(TM) will have
the opportunity to enter the BlackBerry Developer Challenge. See full prize
details at: http://p.sf.net/sfu/blackberry