[libvirt] [PATCH 2/2 (v3)] util: fallback to ioctl(SIOCBRDELBR) if netlink RTM_DELLINK fails

[Martin Kletzander]

On Wed, Aug 26, 2015 at 03:05:25PM -0400, Laine Stump wrote:
>commit 09778e09 switched from using ioctl(SIOCBRDELBR) for bridge
>device deletion to using a netlink RTM_DELLINK message, which is the
>more modern way to delete a bridge (and also doesn't require the
>bridge to be ~IFF_UP to succeed). However, although older kernels
>(e.g. 2.6.32, in RHEL6/CentOS6) support deleting *some* link types
>with RTM_NEWLINK, they don't support deleting bridges, and there is no
>compile-time way to figure this out.
>
>This patch moves the body of the SIOCBRDELBR version of
>virNetDevBridgeDelete() into a static function, calls the new function
>from the original, and also calls the new function from the
>RTM_DELLINK version if the RTM_DELLINK message generates an EOPNOTSUPP
>error. Since RTM_DELLINK is done from the subordinate function
>virNetlinkDelLink, which is also called for other purposes (deleting a
>macvtap interface), a function pointer called "fallback" has been
>added to the arglist of virNetlinkDelLink() - if that arg != NULL, the
>provided function will be called when (and only when) RTM_DELLINK
>fails with EOPNOTSUPP.
>
>Resolves:  https://bugzilla.redhat.com/show_bug.cgi?id=1252780 (part 2)
>---
>Another idea for doing this I came up with during review of the others...
>
> src/util/virnetdevbridge.c  | 41 ++++++++++++++++++++++++++++++-----------
> src/util/virnetdevmacvlan.c |  2 +-
> src/util/virnetlink.c       | 13 +++++++++++--
> src/util/virnetlink.h       |  5 ++++-
> 4 files changed, 46 insertions(+), 15 deletions(-)
>
>diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c
>index ae38901..3b06829 100644
>--- a/src/util/virnetdevbridge.c
>+++ b/src/util/virnetdevbridge.c
>@@ -558,20 +558,15 @@ int virNetDevBridgeCreate(const char *brname)
>  *
>  * Returns 0 in case of success or an errno code in case of failure.
>  */
>-#if defined(__linux__) && defined(HAVE_LIBNL)
>-int virNetDevBridgeDelete(const char *brname)
>-{
>-    /* If netlink is available, use it, as it is successful at
>-     * deleting a bridge even if it is currently IFF_UP.
>-     */
>-    return virNetlinkDelLink(brname);
>-}
>-#elif defined(HAVE_STRUCT_IFREQ) && defined(SIOCBRDELBR)
>-int virNetDevBridgeDelete(const char *brname)
>+#if defined(HAVE_STRUCT_IFREQ) && defined(SIOCBRDELBR)
>+static int
>+virNetDevBridgeDeleteWithIoctl(const char *brname)
> {
>     int fd = -1;
>     int ret = -1;
>
>+    ignore_value(virNetDevSetOnline(brname, false));
>+
>     if ((fd = virNetDevSetupControl(NULL, NULL)) < 0)
>         return -1;
>
>@@ -587,8 +582,32 @@ int virNetDevBridgeDelete(const char *brname)
>     VIR_FORCE_CLOSE(fd);
>     return ret;
> }
>+#endif
>+
>+
>+#if defined(__linux__) && defined(HAVE_LIBNL)
>+int
>+virNetDevBridgeDelete(const char *brname)
>+{
>+    /* If netlink is available, use it, as it is successful at
>+     * deleting a bridge even if it is currently IFF_UP. fallback to
>+     * using ioctl(SIOCBRDELBR) if netlink fails with EOPNOTSUPP.
>+     */
>+    return virNetlinkDelLink(brname, virNetDevBridgeDeleteWithIoctl);

You're passing DeleteWithIoctl here, but that was defined only if
defined(HAVE_STRUCT_IFREQ) && defined(SIOCBRDELBR), does it mean that
if you have libnl on linux, you also have those two?  If not, then I
suggest adding check for HAVE_STRUCT_IFREQ && SIOCBRDELBR definitions
atop this function definition as well, just to make sure we don't run
into similar problems later on with some weird distribution/custom
builds/versions.  No need to resend, this is just a hint before
pushing.

Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150827/e9ec3940/attachment-0001.sig>