[ovs-dev] [PATCH] xenserver: Revert only the XenServer scripts no longer replaced by OVS.

[Ben Pfaff]

On Fri, Feb 18, 2011 at 02:27:41PM -0800, Andrew Evans wrote:
> On 2/18/11 10:15 AM, Ben Pfaff wrote:
> > On Thu, Feb 17, 2011 at 05:52:07PM -0800, Andrew Evans wrote:
> > In references to commit numbers, please also give the subject of the
> > commit (the first line of the log message).
> 
> Ok, now it reads like this:
> 
> Commit d66880ee (xenserver: Clean up /usr/sbin/brctl dangling symlink.)
> 
> and:
> 
> now-nonexistent OVS replacement (caused by commit 92dbd5c9 (xenserver:
> Replace customized xen-bugtool with plugin to collect qdisc info.)).

Thank you.

> I don't like that either, now that I look at it again. I think I was
> just brute-forcing through that error condition. How about this? ("Now
> with 100% less brute force!") I've tested it, and I think it handles the
> corner cases more gracefully.
> 
> if [ -d /usr/lib/openvswitch/xs-original ]; then
>     mkdir -p /usr/lib/openvswitch/xs-saved
>     mv /usr/lib/openvswitch/xs-original/* /usr/lib/openvswitch/xs-saved/ &&
>         rmdir /usr/lib/openvswitch/xs-original
> fi

Looks reasonable.  Thank you.

> >> +    # Only revert dangling symlinks.
> >> +    if [ -h "$f" ] && ! readlink -e "$f"; then
> > 
> > Won't this readlink invocation print the link name on the console?
> > Should we redirect to /dev/null?  But I think that [ ! -e "$f" ] would
> > work just as well, since "test" generally follows symlinks.
> 
> You're right, it does write to stdout in the case where the links are
> valid, and it shouldn't. This now reads:
> 
>     if [ -h "$f" -a ! -e "$f" ]; then

It's best to avoid -a and -o.  Please use && instead.

The Autoconf manual says this:

`test'
     The `test' program is the way to perform many file and string
     tests.  It is often invoked by the alternate name `[', but using
     that name in Autoconf code is asking for trouble since it is an M4
     quote character.

     The `-a', `-o', `(', and `)' operands are not present in all
     implementations, and have been marked obsolete by Posix 2008.
     This is because there are inherent ambiguities in using them.  For
     example, `test "$1" -a "$2"' looks like a binary operator to check
     whether two strings are both non-empty, but if `$1' is the literal
     `!', then some implementations of `test' treat it as a negation of
     the unary operator `-a'.

     Thus, portable uses of `test' should never have more than four
     arguments, and scripts should use shell constructs like `&&' and
     `||' instead.  If you combine `&&' and `||' in the same statement,
     keep in mind that they have equal precedence, so it is often
     better to parenthesize even when this is redundant.  For example:

          # Not portable:
          test "X$a" = "X$b" -a \
            '(' "X$c" != "X$d" -o "X$e" = "X$f" ')'

          # Portable:
          test "X$a" = "X$b" &&
            { test "X$c" != "X$d" || test "X$e" = "X$f"; }

Thanks,

Ben.