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

Ben Pfaff blp at nicira.com
Fri Feb 18 18:15:19 UTC 2011

On Thu, Feb 17, 2011 at 05:52:07PM -0800, Andrew Evans wrote:
> Commit d66880ee attempted to revert the original XenServer-shipped versions of
> scripts replaced by OVS during an RPM upgrade, but the logic was incorrect. It
> assumed that %postun of the package being replaced was run before the %post of
> the new version replacing it. The reverse is actually true.
> Make upgrade and erase cases both work correctly by simply checking whether any
> of the files ever replaced in any OVS version are dangling symlinks, and if so,
> attempt to copy the saved XenServer original back to its rightful place. In the
> upgrade case, if the newly-installed version of OVS lacks any of the scripts in
> the previous version, those will be reverted. In the erase case, none of the
> OVS replacements will exist, so they will all be dangling symlinks and will all
> be reverted.
> Furthermore, replace any dangling symlink from /usr/sbin/xen-bugtool to the
> now-nonexistent OVS replacement (caused by commit 92dbd5c9).
> Lastly, prevent accidental reversion of files replaced by OVS with XenServer
> originals during rpm -U (also caused by commit 92dbd5c9).
> Bug #4696.

This looks good to me, with a few comments.

In references to commit numbers, please also give the subject of the
commit (the first line of the log message).

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

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

It sure is a shame that rpm doesn't have anything equivalent to dpkg's
"diversions" mechanism.  Just look how much time and trouble it could
have saved us over the years.

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

Thanks for cleaning up and fixing all of this.

More information about the dev mailing list