[libvirt] [PATCH v3 10/36] network: move fixup for domain actual net def out of network driver

Daniel P. Berrangé posted 36 patches 6 years, 10 months ago
There is a newer version of this series
[libvirt] [PATCH v3 10/36] network: move fixup for domain actual net def out of network driver
Posted by Daniel P. Berrangé 6 years, 10 months ago
The hypervisor drivers are soon going to communicate with the network
driver via public APIs only. As such the network driver will not ever
see the domain actual network def. Thus the backwards compatibility
fixup logic must be moved out of the network driver.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 src/conf/domain_conf.c      | 25 +++++++++++++++++++++++++
 src/network/bridge_driver.c | 20 --------------------
 2 files changed, 25 insertions(+), 20 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 99b75e26f3..35d965d2a3 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -30210,6 +30210,7 @@ virDomainNetNotifyActualDevice(virConnectPtr conn,
                                virDomainDefPtr dom,
                                virDomainNetDefPtr iface)
 {
+    virDomainNetType actualType = virDomainNetGetActualType(iface);
     virNetworkPtr net = NULL;
 
     if (!netNotify)
@@ -30218,6 +30219,30 @@ virDomainNetNotifyActualDevice(virConnectPtr conn,
     if (!(net = virNetworkLookupByName(conn, iface->data.network.name)))
         return;
 
+    /* if we're restarting libvirtd after an upgrade from a version
+     * that didn't save bridge name in actualNetDef for
+     * actualType==network, we need to copy it in so that it will be
+     * available in all cases
+     */
+    if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK &&
+        !iface->data.network.actual->data.bridge.brname) {
+        char *bridge = virNetworkGetBridgeName(net);
+        if (!bridge)
+            goto cleanup;
+        VIR_FREE(iface->data.network.actual->data.bridge.brname);
+        iface->data.network.actual->data.bridge.brname = bridge;
+    }
+
+    /* Older libvirtd uses actualType==network, but we now
+     * just use actualType==bridge, as nothing needs to
+     * distinguish the two cases, and this simplifies virt
+     * drive code */
+    if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK) {
+        iface->data.network.actual->type = VIR_DOMAIN_NET_TYPE_BRIDGE;
+        actualType = VIR_DOMAIN_NET_TYPE_BRIDGE;
+    }
+
+
     if (netNotify(net, dom, iface) < 0)
         goto cleanup;
 
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 5f3371b150..a1ae90a34c 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -4816,26 +4816,6 @@ networkNotifyActualDevice(virNetworkPtr net,
         goto error;
     }
 
-    /* if we're restarting libvirtd after an upgrade from a version
-     * that didn't save bridge name in actualNetDef for
-     * actualType==network, we need to copy it in so that it will be
-     * available in all cases
-     */
-    if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK &&
-        !iface->data.network.actual->data.bridge.brname &&
-        (VIR_STRDUP(iface->data.network.actual->data.bridge.brname,
-                    netdef->bridge) < 0))
-            goto error;
-
-    /* Older libvirtd uses actualType==network, but we now
-     * just use actualType==bridge, as nothing needs to
-     * distinguish the two cases, and this simplifies virt
-     * drive code */
-    if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK) {
-        iface->data.network.actual->type = VIR_DOMAIN_NET_TYPE_BRIDGE;
-        actualType = VIR_DOMAIN_NET_TYPE_BRIDGE;
-    }
-
     if (!iface->data.network.actual ||
         (actualType != VIR_DOMAIN_NET_TYPE_DIRECT &&
          actualType != VIR_DOMAIN_NET_TYPE_HOSTDEV)) {
-- 
2.20.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 10/36] network: move fixup for domain actual net def out of network driver
Posted by Laine Stump 6 years, 10 months ago
On 3/19/19 8:46 AM, Daniel P. Berrangé wrote:
> The hypervisor drivers are soon going to communicate with the network
> driver via public APIs only. As such the network driver will not ever
> see the domain actual network def. Thus the backwards compatibility
> fixup logic must be moved out of the network driver.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com
> ---
>   src/conf/domain_conf.c      | 25 +++++++++++++++++++++++++
>   src/network/bridge_driver.c | 20 --------------------
>   2 files changed, 25 insertions(+), 20 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 99b75e26f3..35d965d2a3 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -30210,6 +30210,7 @@ virDomainNetNotifyActualDevice(virConnectPtr conn,
>                                  virDomainDefPtr dom,
>                                  virDomainNetDefPtr iface)
>   {
> +    virDomainNetType actualType = virDomainNetGetActualType(iface);
>       virNetworkPtr net = NULL;
>   
>       if (!netNotify)
> @@ -30218,6 +30219,30 @@ virDomainNetNotifyActualDevice(virConnectPtr conn,
>       if (!(net = virNetworkLookupByName(conn, iface->data.network.name)))
>           return;
>   
> +    /* if we're restarting libvirtd after an upgrade from a version
> +     * that didn't save bridge name in actualNetDef for
> +     * actualType==network, we need to copy it in so that it will be
> +     * available in all cases
> +     */
> +    if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK &&
> +        !iface->data.network.actual->data.bridge.brname) {
> +        char *bridge = virNetworkGetBridgeName(net);
> +        if (!bridge)
> +            goto cleanup;
> +        VIR_FREE(iface->data.network.actual->data.bridge.brname);
> +        iface->data.network.actual->data.bridge.brname = bridge;
> +    }
> +


The above code was added in libvirt 1.2.11 in December 2014. It seems 
nearly impossible that someone would be upgrading from a libvirt that 
was 1.2.10 or older straight to libvirt-5.2.0 *without rebooting their 
host*. For this reason, I think the above code can/should be retired.


Reviewed-by: Laine Stump <laine@laine.org>


(either way, whether you decide to leave that code in or not.)


> +    /* Older libvirtd uses actualType==network, but we now
> +     * just use actualType==bridge, as nothing needs to
> +     * distinguish the two cases, and this simplifies virt
> +     * drive code */
> +    if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK) {
> +        iface->data.network.actual->type = VIR_DOMAIN_NET_TYPE_BRIDGE;
> +        actualType = VIR_DOMAIN_NET_TYPE_BRIDGE;
> +    }
> +
> +
>       if (netNotify(net, dom, iface) < 0)
>           goto cleanup;
>   
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 5f3371b150..a1ae90a34c 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -4816,26 +4816,6 @@ networkNotifyActualDevice(virNetworkPtr net,
>           goto error;
>       }
>   
> -    /* if we're restarting libvirtd after an upgrade from a version
> -     * that didn't save bridge name in actualNetDef for
> -     * actualType==network, we need to copy it in so that it will be
> -     * available in all cases
> -     */
> -    if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK &&
> -        !iface->data.network.actual->data.bridge.brname &&
> -        (VIR_STRDUP(iface->data.network.actual->data.bridge.brname,
> -                    netdef->bridge) < 0))
> -            goto error;
> -
> -    /* Older libvirtd uses actualType==network, but we now
> -     * just use actualType==bridge, as nothing needs to
> -     * distinguish the two cases, and this simplifies virt
> -     * drive code */
> -    if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK) {
> -        iface->data.network.actual->type = VIR_DOMAIN_NET_TYPE_BRIDGE;
> -        actualType = VIR_DOMAIN_NET_TYPE_BRIDGE;
> -    }
> -
>       if (!iface->data.network.actual ||
>           (actualType != VIR_DOMAIN_NET_TYPE_DIRECT &&
>            actualType != VIR_DOMAIN_NET_TYPE_HOSTDEV)) {


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 10/36] network: move fixup for domain actual net def out of network driver
Posted by Daniel P. Berrangé 6 years, 10 months ago
On Thu, Mar 21, 2019 at 10:33:30PM -0400, Laine Stump wrote:
> On 3/19/19 8:46 AM, Daniel P. Berrangé wrote:
> > The hypervisor drivers are soon going to communicate with the network
> > driver via public APIs only. As such the network driver will not ever
> > see the domain actual network def. Thus the backwards compatibility
> > fixup logic must be moved out of the network driver.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com
> > ---
> >   src/conf/domain_conf.c      | 25 +++++++++++++++++++++++++
> >   src/network/bridge_driver.c | 20 --------------------
> >   2 files changed, 25 insertions(+), 20 deletions(-)
> > 
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index 99b75e26f3..35d965d2a3 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -30210,6 +30210,7 @@ virDomainNetNotifyActualDevice(virConnectPtr conn,
> >                                  virDomainDefPtr dom,
> >                                  virDomainNetDefPtr iface)
> >   {
> > +    virDomainNetType actualType = virDomainNetGetActualType(iface);
> >       virNetworkPtr net = NULL;
> >       if (!netNotify)
> > @@ -30218,6 +30219,30 @@ virDomainNetNotifyActualDevice(virConnectPtr conn,
> >       if (!(net = virNetworkLookupByName(conn, iface->data.network.name)))
> >           return;
> > +    /* if we're restarting libvirtd after an upgrade from a version
> > +     * that didn't save bridge name in actualNetDef for
> > +     * actualType==network, we need to copy it in so that it will be
> > +     * available in all cases
> > +     */
> > +    if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK &&
> > +        !iface->data.network.actual->data.bridge.brname) {
> > +        char *bridge = virNetworkGetBridgeName(net);
> > +        if (!bridge)
> > +            goto cleanup;
> > +        VIR_FREE(iface->data.network.actual->data.bridge.brname);
> > +        iface->data.network.actual->data.bridge.brname = bridge;
> > +    }
> > +
> 
> 
> The above code was added in libvirt 1.2.11 in December 2014. It seems nearly
> impossible that someone would be upgrading from a libvirt that was 1.2.10 or
> older straight to libvirt-5.2.0 *without rebooting their host*. For this
> reason, I think the above code can/should be retired.

Yeah, I think that's probably ok

> 
> 
> Reviewed-by: Laine Stump <laine@laine.org>
> 
> 
> (either way, whether you decide to leave that code in or not.)
> 

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