2009/7/6 Jiří Paleček <[email protected]>:
> On Tue, 07 Jul 2009 01:33:29 +0200, Garrett Cooper <[email protected]>
> wrote:
>
>> On Mon, Jul 6, 2009 at 3:47 PM, Jiri Palecek<[email protected]> 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
_______________________________________________
Ltp-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list
