[ovs-dev] [PATCH] xenserver: Revert only the XenServer scripts no longer replaced by OVS.
aevans at nicira.com
Fri Feb 18 22:27:41 UTC 2011
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.)
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/ &&
>> + 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!
More information about the dev