[ovs-dev] [PATCH] xenserver: Revert only the XenServer scripts no longer replaced by OVS.
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