Re: [libvirt] [PATCH 2/2 (v3)] util: fallback to ioctl(SIOCBRDELBR) if netlink RTM_DELLINK fails
On 08/27/2015 06:07 AM, Martin Kletzander wrote:
> 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.
Yeah, thanks for pointing that out. I had that check in v2, but forgot
it here.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2 (v3)] util: fallback to ioctl(SIOCBRDELBR) if netlink RTM_DELLINK fails
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
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2 (v3)] util: fallback to ioctl(SIOCBRDELBR) if netlink RTM_DELLINK fails
On 08/26/2015 09:24 PM, Michal Privoznik wrote: > On 26.08.2015 21:05, 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(-) > ACK. But don't forget that the counterpart is broken too. Once you > destroy network, on RHEL-6, you can't start it up, because of the same > problem. Yeah, that was patch 1/2 (which hasn't been reviewed yet (hint hint :-) -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2 (v3)] util: fallback to ioctl(SIOCBRDELBR) if netlink RTM_DELLINK fails
On 26.08.2015 21:05, 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(-) ACK. But don't forget that the counterpart is broken too. Once you destroy network, on RHEL-6, you can't start it up, because of the same problem. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/2 (v3)] util: fallback to ioctl(SIOCBRDELBR) if netlink RTM_DELLINK fails
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,
