Re: [LTP] [PATCH] Fix some bashisms

Garrett Cooper
2009-07-07T01:02:08Z

2009/7/6 Jiří Paleček <jpalecek@web.de>:
> 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? :-)

Efficiency, and more importantly readability.

> 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 course, usable only when there's no "else" branch.

That's ugly and hard to read with large logic blocks.

> 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".

Yeah, true. Why not run everything through set -u then?

>>>> 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?

Ugh. Nevermind -- you're right ><. Keep on forgetting about that bloody syntax.
-Garrett

------------------------------------------------------------------------------
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