1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH v3 00/36] network: refactor to decouple virt drivers from network driver
  4. View patch

[libvirt] [PATCH v3 06/36] util: add helper method for re-attaching a tap device to a bridge

Daniel P. Berrangé posted 36 patches 6 years, 10 months ago
There is a newer version of this series
[libvirt] [PATCH v3 06/36] util: add helper method for re-attaching a tap device to a bridge
Posted by Daniel P. Berrangé 6 years, 10 months ago 
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 src/libvirt_private.syms |  1 +
 src/util/virnetdevtap.c  | 69 ++++++++++++++++++++++++++++++++++++++++
 src/util/virnetdevtap.h  | 12 +++++++
 3 files changed, 82 insertions(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index d9494a04bb..6f5a734fdb 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2461,6 +2461,7 @@ virNetDevTapDelete;
 virNetDevTapGetName;
 virNetDevTapGetRealDeviceName;
 virNetDevTapInterfaceStats;
+virNetDevTapReattachBridge;
 
 
 # util/virnetdevveth.h
diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c
index 972f3405aa..0484c7c5a4 100644
--- a/src/util/virnetdevtap.c
+++ b/src/util/virnetdevtap.c
@@ -553,6 +553,75 @@ virNetDevTapAttachBridge(const char *tapname,
 }
 
 
+/**
+ * virNetDevTapReattachBridge:
+ * @tapname: the tap interface name (or name template)
+ * @brname: the bridge name
+ * @macaddr: desired MAC address
+ * @virtPortProfile: bridge/port specific configuration
+ * @virtVlan: vlan tag info
+ * @mtu: requested MTU for port (or 0 for "default")
+ * @actualMTU: MTU actually set for port (after accounting for bridge's MTU)
+ *
+ * Ensures that the tap device (@tapname) is connected to the bridge
+ * (@brname), potentially removing it from any existing bridge that
+ * does not match.
+ *
+ * Returns 0 in case of success or -1 on failure
+ */
+int
+virNetDevTapReattachBridge(const char *tapname,
+                           const char *brname,
+                           const virMacAddr *macaddr,
+                           const unsigned char *vmuuid,
+                           virNetDevVPortProfilePtr virtPortProfile,
+                           virNetDevVlanPtr virtVlan,
+                           unsigned int mtu,
+                           unsigned int *actualMTU)
+{
+    bool useOVS = false;
+    char *master = NULL;
+
+    if (virNetDevGetMaster(tapname, &master) < 0)
+        return -1;
+
+    /* IFLA_MASTER for a tap on an OVS switch is always "ovs-system" */
+    if (STREQ_NULLABLE(master, "ovs-system")) {
+        useOVS = true;
+        VIR_FREE(master);
+        if (virNetDevOpenvswitchInterfaceGetMaster(tapname, &master) < 0)
+            return -1;
+    }
+
+    /* Nothing more todo if we're on the right bridge already */
+    if (STREQ_NULLABLE(brname, master))
+        return 0;
+
+    /* disconnect from current (incorrect) bridge, if any  */
+    if (master) {
+        int ret;
+        VIR_INFO("Removing %s from %s", tapname, master);
+        if (useOVS)
+            ret = virNetDevOpenvswitchRemovePort(master, tapname);
+        else
+            ret = virNetDevBridgeRemovePort(master, tapname);
+        VIR_FREE(master);
+        if (ret < 0)
+            return -1;
+    }
+
+    VIR_INFO("Attaching %s to %s", tapname, brname);
+    if (virNetDevTapAttachBridge(tapname, brname,
+                                 macaddr, vmuuid,
+                                 virtPortProfile,
+                                 virtVlan,
+                                 mtu, actualMTU) < 0)
+        return -1;
+
+    return 0;
+}
+
+
 /**
  * virNetDevTapCreateInBridgePort:
  * @brname: the bridge name
diff --git a/src/util/virnetdevtap.h b/src/util/virnetdevtap.h
index 226122aa2c..2b3893cd37 100644
--- a/src/util/virnetdevtap.h
+++ b/src/util/virnetdevtap.h
@@ -71,6 +71,18 @@ virNetDevTapAttachBridge(const char *tapname,
     ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
     ATTRIBUTE_RETURN_CHECK;
 
+int
+virNetDevTapReattachBridge(const char *tapname,
+                           const char *brname,
+                           const virMacAddr *macaddr,
+                           const unsigned char *vmuuid,
+                           virNetDevVPortProfilePtr virtPortProfile,
+                           virNetDevVlanPtr virtVlan,
+                           unsigned int mtu,
+                           unsigned int *actualMTU)
+    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
+    ATTRIBUTE_RETURN_CHECK;
+
 int virNetDevTapCreateInBridgePort(const char *brname,
                                    char **ifname,
                                    const virMacAddr *macaddr,
-- 
2.20.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 06/36] util: add helper method for re-attaching a tap device to a bridge
Posted by Cole Robinson 6 years, 10 months ago 
On 3/19/19 8:46 AM, Daniel P. Berrangé wrote:
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  src/libvirt_private.syms |  1 +
>  src/util/virnetdevtap.c  | 69 ++++++++++++++++++++++++++++++++++++++++
>  src/util/virnetdevtap.h  | 12 +++++++
>  3 files changed, 82 insertions(+)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index d9494a04bb..6f5a734fdb 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2461,6 +2461,7 @@ virNetDevTapDelete;
>  virNetDevTapGetName;
>  virNetDevTapGetRealDeviceName;
>  virNetDevTapInterfaceStats;
> +virNetDevTapReattachBridge;
>  
>  
>  # util/virnetdevveth.h
> diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c
> index 972f3405aa..0484c7c5a4 100644
> --- a/src/util/virnetdevtap.c
> +++ b/src/util/virnetdevtap.c
> @@ -553,6 +553,75 @@ virNetDevTapAttachBridge(const char *tapname,
>  }
>  
>  
> +/**
> + * virNetDevTapReattachBridge:
> + * @tapname: the tap interface name (or name template)
> + * @brname: the bridge name
> + * @macaddr: desired MAC address
> + * @virtPortProfile: bridge/port specific configuration
> + * @virtVlan: vlan tag info
> + * @mtu: requested MTU for port (or 0 for "default")
> + * @actualMTU: MTU actually set for port (after accounting for bridge's MTU)
> + *
> + * Ensures that the tap device (@tapname) is connected to the bridge
> + * (@brname), potentially removing it from any existing bridge that
> + * does not match.
> + *
> + * Returns 0 in case of success or -1 on failure
> + */
> +int
> +virNetDevTapReattachBridge(const char *tapname,
> +                           const char *brname,
> +                           const virMacAddr *macaddr,
> +                           const unsigned char *vmuuid,
> +                           virNetDevVPortProfilePtr virtPortProfile,
> +                           virNetDevVlanPtr virtVlan,
> +                           unsigned int mtu,
> +                           unsigned int *actualMTU)
> +{
> +    bool useOVS = false;
> +    char *master = NULL;
> +

Could use VIR_AUTOFREE to simplify things a bit

> +    if (virNetDevGetMaster(tapname, &master) < 0)
> +        return -1;
> +
> +    /* IFLA_MASTER for a tap on an OVS switch is always "ovs-system" */
> +    if (STREQ_NULLABLE(master, "ovs-system")) {
> +        useOVS = true;
> +        VIR_FREE(master);
> +        if (virNetDevOpenvswitchInterfaceGetMaster(tapname, &master) < 0)
> +            return -1;
> +    }
> +
> +    /* Nothing more todo if we're on the right bridge already */
> +    if (STREQ_NULLABLE(brname, master))
> +        return 0;
> +
> +    /* disconnect from current (incorrect) bridge, if any  */
> +    if (master) {
> +        int ret;
> +        VIR_INFO("Removing %s from %s", tapname, master);
> +        if (useOVS)
> +            ret = virNetDevOpenvswitchRemovePort(master, tapname);
> +        else
> +            ret = virNetDevBridgeRemovePort(master, tapname);
> +        VIR_FREE(master);
> +        if (ret < 0)
> +            return -1;
> +    }
> +

These were non-fatal in the original code. Is it okay to change?

Provided there's an answer:

Reviewed-by: Cole Robinson <crobinso@redhat.com>

- Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 06/36] util: add helper method for re-attaching a tap device to a bridge
Posted by Laine Stump 6 years, 10 months ago 
On 3/21/19 9:14 PM, Cole Robinson wrote:
> On 3/19/19 8:46 AM, Daniel P. Berrangé wrote:
>> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>> ---
>>   src/libvirt_private.syms |  1 +
>>   src/util/virnetdevtap.c  | 69 ++++++++++++++++++++++++++++++++++++++++
>>   src/util/virnetdevtap.h  | 12 +++++++
>>   3 files changed, 82 insertions(+)
>>
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index d9494a04bb..6f5a734fdb 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -2461,6 +2461,7 @@ virNetDevTapDelete;
>>   virNetDevTapGetName;
>>   virNetDevTapGetRealDeviceName;
>>   virNetDevTapInterfaceStats;
>> +virNetDevTapReattachBridge;
>>   
>>   
>>   # util/virnetdevveth.h
>> diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c
>> index 972f3405aa..0484c7c5a4 100644
>> --- a/src/util/virnetdevtap.c
>> +++ b/src/util/virnetdevtap.c
>> @@ -553,6 +553,75 @@ virNetDevTapAttachBridge(const char *tapname,
>>   }
>>   
>>   
>> +/**
>> + * virNetDevTapReattachBridge:
>> + * @tapname: the tap interface name (or name template)
>> + * @brname: the bridge name
>> + * @macaddr: desired MAC address
>> + * @virtPortProfile: bridge/port specific configuration
>> + * @virtVlan: vlan tag info
>> + * @mtu: requested MTU for port (or 0 for "default")
>> + * @actualMTU: MTU actually set for port (after accounting for bridge's MTU)
>> + *
>> + * Ensures that the tap device (@tapname) is connected to the bridge
>> + * (@brname), potentially removing it from any existing bridge that
>> + * does not match.
>> + *
>> + * Returns 0 in case of success or -1 on failure
>> + */
>> +int
>> +virNetDevTapReattachBridge(const char *tapname,
>> +                           const char *brname,
>> +                           const virMacAddr *macaddr,
>> +                           const unsigned char *vmuuid,
>> +                           virNetDevVPortProfilePtr virtPortProfile,
>> +                           virNetDevVlanPtr virtVlan,
>> +                           unsigned int mtu,
>> +                           unsigned int *actualMTU)
>> +{
>> +    bool useOVS = false;
>> +    char *master = NULL;
>> +
> Could use VIR_AUTOFREE to simplify things a bit
>
>> +    if (virNetDevGetMaster(tapname, &master) < 0)
>> +        return -1;
>> +
>> +    /* IFLA_MASTER for a tap on an OVS switch is always "ovs-system" */
>> +    if (STREQ_NULLABLE(master, "ovs-system")) {
>> +        useOVS = true;
>> +        VIR_FREE(master);
>> +        if (virNetDevOpenvswitchInterfaceGetMaster(tapname, &master) < 0)
>> +            return -1;
>> +    }
>> +
>> +    /* Nothing more todo if we're on the right bridge already */
>> +    if (STREQ_NULLABLE(brname, master))
>> +        return 0;
>> +
>> +    /* disconnect from current (incorrect) bridge, if any  */
>> +    if (master) {
>> +        int ret;
>> +        VIR_INFO("Removing %s from %s", tapname, master);
>> +        if (useOVS)
>> +            ret = virNetDevOpenvswitchRemovePort(master, tapname);
>> +        else
>> +            ret = virNetDevBridgeRemovePort(master, tapname);
>> +        VIR_FREE(master);
>> +        if (ret < 0)
>> +            return -1;
>> +    }
>> +
> These were non-fatal in the original code. Is it okay to change?


I don't remember why I made those non-fatal in the original code. 
Probably wanted to give as high a chance as possible of muddling through 
even in the face of errors from ovs. But looking at the code again, I 
agree that it's just as well to fail hard instead of ignoring the errors.


>
> Provided there's an answer:
>
> Reviewed-by: Cole Robinson <crobinso@redhat.com>
>
> - Cole
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 06/36] util: add helper method for re-attaching a tap device to a bridge
Posted by Daniel P. Berrangé 6 years, 10 months ago 
On Thu, Mar 21, 2019 at 09:14:13PM -0400, Cole Robinson wrote:
> On 3/19/19 8:46 AM, Daniel P. Berrangé wrote:
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  src/libvirt_private.syms |  1 +
> >  src/util/virnetdevtap.c  | 69 ++++++++++++++++++++++++++++++++++++++++
> >  src/util/virnetdevtap.h  | 12 +++++++
> >  3 files changed, 82 insertions(+)
> > 
> > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> > index d9494a04bb..6f5a734fdb 100644
> > --- a/src/libvirt_private.syms
> > +++ b/src/libvirt_private.syms
> > @@ -2461,6 +2461,7 @@ virNetDevTapDelete;
> >  virNetDevTapGetName;
> >  virNetDevTapGetRealDeviceName;
> >  virNetDevTapInterfaceStats;
> > +virNetDevTapReattachBridge;
> >  
> >  
> >  # util/virnetdevveth.h
> > diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c
> > index 972f3405aa..0484c7c5a4 100644
> > --- a/src/util/virnetdevtap.c
> > +++ b/src/util/virnetdevtap.c
> > @@ -553,6 +553,75 @@ virNetDevTapAttachBridge(const char *tapname,
> >  }
> >  
> >  
> > +/**
> > + * virNetDevTapReattachBridge:
> > + * @tapname: the tap interface name (or name template)
> > + * @brname: the bridge name
> > + * @macaddr: desired MAC address
> > + * @virtPortProfile: bridge/port specific configuration
> > + * @virtVlan: vlan tag info
> > + * @mtu: requested MTU for port (or 0 for "default")
> > + * @actualMTU: MTU actually set for port (after accounting for bridge's MTU)
> > + *
> > + * Ensures that the tap device (@tapname) is connected to the bridge
> > + * (@brname), potentially removing it from any existing bridge that
> > + * does not match.
> > + *
> > + * Returns 0 in case of success or -1 on failure
> > + */
> > +int
> > +virNetDevTapReattachBridge(const char *tapname,
> > +                           const char *brname,
> > +                           const virMacAddr *macaddr,
> > +                           const unsigned char *vmuuid,
> > +                           virNetDevVPortProfilePtr virtPortProfile,
> > +                           virNetDevVlanPtr virtVlan,
> > +                           unsigned int mtu,
> > +                           unsigned int *actualMTU)
> > +{
> > +    bool useOVS = false;
> > +    char *master = NULL;
> > +
> 
> Could use VIR_AUTOFREE to simplify things a bit
> 
> > +    if (virNetDevGetMaster(tapname, &master) < 0)
> > +        return -1;
> > +
> > +    /* IFLA_MASTER for a tap on an OVS switch is always "ovs-system" */
> > +    if (STREQ_NULLABLE(master, "ovs-system")) {
> > +        useOVS = true;
> > +        VIR_FREE(master);
> > +        if (virNetDevOpenvswitchInterfaceGetMaster(tapname, &master) < 0)
> > +            return -1;
> > +    }
> > +
> > +    /* Nothing more todo if we're on the right bridge already */
> > +    if (STREQ_NULLABLE(brname, master))
> > +        return 0;
> > +
> > +    /* disconnect from current (incorrect) bridge, if any  */
> > +    if (master) {
> > +        int ret;
> > +        VIR_INFO("Removing %s from %s", tapname, master);
> > +        if (useOVS)
> > +            ret = virNetDevOpenvswitchRemovePort(master, tapname);
> > +        else
> > +            ret = virNetDevBridgeRemovePort(master, tapname);
> > +        VIR_FREE(master);
> > +        if (ret < 0)
> > +            return -1;
> > +    }
> > +
> 
> These were non-fatal in the original code. Is it okay to change?

That's broken code IMHO. If we fail to remove the existing bridge
port, then we are about to fail to attach to the new bridge, because
a nic can only be part of 1 bridge at any time.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.