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? :-) 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 _______________________________________________ Ltp-list mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/ltp-list
