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

[Andrew Evans]

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

>> +# Deliberately break %postun in broken OVS builds that revert original
>> +# XenServer scripts during rpm -U by moving the directory where it thinks
>> +# they are saved.
>> +[ -d /usr/lib/openvswitch/xs-original ] && \
>> +    mv -f /usr/lib/openvswitch/xs-original /usr/lib/openvswitch/xs-saved 2>/dev/null
> 
> I see two possible issues here.  If /usr/lib/openvswitch/xs-original
> and /usr/lib/openvswitch/xs-saved both exist, then I think that we
> will end up with a /usr/lib/openvswitch/xs-saved/xs-original
> directory.  So I think that we should add a [ ! -e
> /usr/lib/openvswitch/xs-saved ] condition here too.
> 
> Second, the '-f' and '2>/dev/null' indicate that you expect that there
> could be errors that should be suppress.  What errors do you expect,
> and why should they be suppressed?

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

>> +    saved="/usr/lib/openvswitch/xs-saved/$(basename "$orig")"
> 
> There's no word splitting on the right side of a shell assignment, so
> you can omit the outermost double quotes here.

I thought I understood shell quoting, but this is something I didn't
know. It turns out that there *is* word splitting on the right side of a
shell assignment; i.e. `x=a b` will fail. But the shell appears to
auto-quote the expansion of the path and the shell command expansion:

$ orig="/a/b/c d e"
$ set -x
$ x=/usr/lib/$(basename "$orig")
++ basename '/a/b/c d e'
+ x='/usr/lib/c d e'

So I've removed the outermost quotes. Thanks for teaching me something
new about the shell after all this time. :)

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

> Thanks for cleaning up and fixing all of this.

Thanks for your review!

-Andrew