On Tue, Aug 25, 2015 at 11:47:54PM -0400, Laine Stump wrote: The problem mentioned in te 2/2 (v3) doesn't exist here, so I think the second patch should really be changed. Anyway, would it make sense to also allow fallback to the version with SIOCIFCREATE2? ACK if the answer to the question is "No". Martin

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. (Ugly Hack Alert: because the RTM_DELLINK message is sent from a different function (virNetDevDelLink()), not directly from virNetDevBridgeDelete(), and the other caller of that function needs to *not* have special behavior on EOPNOTSUPP, virNetlinkDelLink() has an added "silentEOPNOTSUPP" argument, which is set to true when virNetlinkDelLink should be silent when encountering EOPNOTSUPP, and instead return -2 so that the caller can try to recover before logging an error. This was done to avoid duplicating the entire body of virNetlinkDelLink() inside virNetDevBridgeDelete(). I am also preparing a patch that does this duplication, but avoids the tricky special boolean and return value. If anyone objects in the slightest to this version, I will gladly use the other. Mostly I wrote this version to illustrate how ugly it would be to eliminate the duplicate code :-)) This resolves a similar issue to that reported in: https://bugzilla.redhat.com/show_bug.cgi?id=1252780 --- src/util/virnetdevbridge.c | 48 +++++++++++++++++++++++++++++++++++---------- src/util/virnetdevmacvlan.c | 2 +- src/util/virnetlink.c | 17 ++++++++++++---- src/util/virnetlink.h | 2 +- 4 files changed, 53 insertions(+), 16 deletions(-) diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c index ae38901..19eaf7b 100644 --- a/src/util/virnetdevbridge.c +++ b/src/util/virnetdevbridge.c @@ -558,16 +558,9 @@ 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; @@ -587,6 +580,41 @@ 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. If it fails + * with EOPNOTSUPP, itwill return a special "retry with something + * older" code, -2. + */ + int ret; + +# if defined(HAVE_STRUCT_IFREQ) && defined(SIOCBRDELBR) + ret = virNetlinkDelLink(brname, true); + if (ret == -2) { + ignore_value(virNetDevSetOnline(brname, false)); + ret = virNetDevBridgeDeleteWithIoctl(brname); + } +# else + ret = virNetlinkDelLink(brname, false); +# endif + return ret; +} + + +#elif defined(HAVE_STRUCT_IFREQ) && defined(SIOCBRDELBR) +int +virNetDevBridgeDelete(const char *brname) +{ + return virNetDevBridgeDeleteWithIoctl(brname); +} + + #elif defined(HAVE_STRUCT_IFREQ) && defined(SIOCIFDESTROY) int virNetDevBridgeDelete(const char *brname) { diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index 213b8eb..48403b3 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -220,7 +220,7 @@ virNetDevMacVLanCreate(const char *ifname, */ int virNetDevMacVLanDelete(const char *ifname) { - return virNetlinkDelLink(ifname); + return virNetlinkDelLink(ifname, false); } diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index 0052ef9..ae7b9c8 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -281,15 +281,17 @@ int virNetlinkCommand(struct nl_msg *nl_msg, * virNetlinkDelLink: * * @ifname: Name of the link + * @silentEOPNOTSUPP * * delete a network "link" (aka interface aka device) with the given * name. This works for many different types of network devices, * including macvtap and bridges. * - * Returns 0 on success, -1 on fatal error. + * Returns 0 on success, -1 on fatal error, -2 if EOPNOTSUPP (meaning + * the caller will need to retry the operation with an alternate method) */ int -virNetlinkDelLink(const char *ifname) +virNetlinkDelLink(const char *ifname, bool silentEOPNOTSUPP) { int rc = -1; struct nlmsghdr *resp = NULL; @@ -324,7 +326,13 @@ virNetlinkDelLink(const char *ifname) err = (struct nlmsgerr *)NLMSG_DATA(resp); if (resp->nlmsg_len < NLMSG_LENGTH(sizeof(*err))) goto malformed_resp; - + if (err->error == -EOPNOTSUPP && silentEOPNOTSUPP) { + /* give the caller a chance to recover without + * logging an error. + */ + rc = -2; + goto cleanup; + } if (err->error) { virReportSystemError(-err->error, _("error destroying network device %s"), @@ -886,7 +894,8 @@ int virNetlinkCommand(struct nl_msg *nl_msg ATTRIBUTE_UNUSED, int -virNetlinkDelLink(const char *ifname ATTRIBUTE_UNUSED) +virNetlinkDelLink(const char *ifname ATTRIBUTE_UNUSED, + bool silentEOPNOTSUPP ATTRIBUTE_UNUSED) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _(unsupported)); return -1; diff --git a/src/util/virnetlink.h b/src/util/virnetlink.h index 06c3cd0..bf9f914 100644 --- a/src/util/virnetlink.h +++ b/src/util/virnetlink.h @@ -51,7 +51,7 @@ int virNetlinkCommand(struct nl_msg *nl_msg, struct nlmsghdr **resp, unsigned int *respbuflen, uint32_t src_pid, uint32_t dst_pid, unsigned int protocol, unsigned int groups); -int virNetlinkDelLink(const char *ifname); +int virNetlinkDelLink(const char *ifname, bool silentEOPNOTSUPP); int virNetlinkGetErrorCode(struct nlmsghdr *resp, unsigned int recvbuflen); -- 2.1.0

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); +} + + +#elif defined(HAVE_STRUCT_IFREQ) && defined(SIOCBRDELBR) +int +virNetDevBridgeDelete(const char *brname) +{ + return virNetDevBridgeDeleteWithIoctl(brname); +} + + #elif defined(HAVE_STRUCT_IFREQ) && defined(SIOCIFDESTROY) -int virNetDevBridgeDelete(const char *brname) +int +virNetDevBridgeDelete(const char *brname) { int s; struct ifreq ifr; diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index 9c4da1f..954736a 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -220,7 +220,7 @@ virNetDevMacVLanCreate(const char *ifname, */ int virNetDevMacVLanDelete(const char *ifname) { - return virNetlinkDelLink(ifname); + return virNetlinkDelLink(ifname, NULL); } diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index 0052ef9..0276522 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -281,6 +281,10 @@ int virNetlinkCommand(struct nl_msg *nl_msg, * virNetlinkDelLink: * * @ifname: Name of the link + * @fallback: pointer to an alternate function that will + * be called to perform the delete if RTM_DELLINK fails + * with EOPNOTSUPP (any other error will simply be treated + * as an error). * * delete a network "link" (aka interface aka device) with the given * name. This works for many different types of network devices, @@ -289,7 +293,7 @@ int virNetlinkCommand(struct nl_msg *nl_msg, * Returns 0 on success, -1 on fatal error. */ int -virNetlinkDelLink(const char *ifname) +virNetlinkDelLink(const char *ifname, virNetlinkDelLinkFallback fallback) { int rc = -1; struct nlmsghdr *resp = NULL; @@ -325,6 +329,10 @@ virNetlinkDelLink(const char *ifname) if (resp->nlmsg_len < NLMSG_LENGTH(sizeof(*err))) goto malformed_resp; + if (-err->error == EOPNOTSUPP && fallback) { + rc = fallback(ifname); + goto cleanup; + } if (err->error) { virReportSystemError(-err->error, _("error destroying network device %s"), @@ -886,7 +894,8 @@ int virNetlinkCommand(struct nl_msg *nl_msg ATTRIBUTE_UNUSED, int -virNetlinkDelLink(const char *ifname ATTRIBUTE_UNUSED) +virNetlinkDelLink(const char *ifname ATTRIBUTE_UNUSED, + virNetlinkDelLinkFallback fallback ATTRIBUTE_UNUSED) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _(unsupported)); return -1; diff --git a/src/util/virnetlink.h b/src/util/virnetlink.h index 06c3cd0..0664a7a 100644 --- a/src/util/virnetlink.h +++ b/src/util/virnetlink.h @@ -51,7 +51,10 @@ int virNetlinkCommand(struct nl_msg *nl_msg, struct nlmsghdr **resp, unsigned int *respbuflen, uint32_t src_pid, uint32_t dst_pid, unsigned int protocol, unsigned int groups); -int virNetlinkDelLink(const char *ifname); + +typedef int (*virNetlinkDelLinkFallback)(const char *ifname); + +int virNetlinkDelLink(const char *ifname, virNetlinkDelLinkFallback fallback); int virNetlinkGetErrorCode(struct nlmsghdr *resp, unsigned int recvbuflen); -- 2.1.0

On 08/27/2015 06:07 AM, Martin Kletzander wrote: Yeah, thanks for pointing that out. I had that check in v2, but forgot it here.