If virDomainUpdateDeviceFlags() was used to update an <interface>, and
the interface type changed from type='network' where the network was
an unmanaged bridge (so actualType == bridge) to type='bridge'
(i.e. actualType *also* == bridge), the update would fail due to the
perceived change in type.

In practice it is okay to switch between any interface types that end
up using a tap device, since libvirt just needs to attach the device
to a new bridge. But in this case we were erroneously rejecting it due
to a conditional that was too restrictive. This is what the code was doing:

  if (old->type != new->type)
     [allow update]
  else
     if ((oldActual == bridge and newActual == network)
         || (oldActual == network and newActual == bridge)) {
         [allow update]
     else
         [error]

In the case described above though, old->type and new->type don't match,
but oldActual and newActual are both 'bridge', so we get an error.

This patch changes the inner conditional so that any combination of
'network' and 'bridge' for oldActual and newActual, since they both
use a tap device connected to a bridge.

Signed-off-by: Laine Stump <laine@redhat.com>
---
 src/qemu/qemu_hotplug.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index a5d57702eb..dc3bd8245f 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -3770,15 +3770,15 @@ qemuDomainChangeNet(virQEMUDriverPtr driver,
          * where this can only require a minor (or even no) change,
          * but in most cases we need to do a full reconnection.
          *
-         * If we switch (in either direction) between type='bridge'
-         * and type='network' (for a traditional managed virtual
-         * network that uses a host bridge, i.e. forward
-         * mode='route|nat'), we just need to change the bridge.
+         * As long as both the new and old types use a tap device
+         * connected to a host bridge (ie VIR_DOMAIN_NET_TYPE_NETWORK
+         * or VIR_DOMAIN_NET_TYPE_BRIDGE), we just need to connect to
+         * the new bridge.
          */
-        if ((oldType == VIR_DOMAIN_NET_TYPE_NETWORK &&
-             newType == VIR_DOMAIN_NET_TYPE_BRIDGE) ||
-            (oldType == VIR_DOMAIN_NET_TYPE_BRIDGE &&
-             newType == VIR_DOMAIN_NET_TYPE_NETWORK)) {
+        if ((oldType == VIR_DOMAIN_NET_TYPE_NETWORK ||
+             oldType == VIR_DOMAIN_NET_TYPE_BRIDGE) &&
+            (newType == VIR_DOMAIN_NET_TYPE_NETWORK ||
+             newType == VIR_DOMAIN_NET_TYPE_BRIDGE)) {
 
             needBridgeChange = true;
 
-- 
2.25.4

On 6/3/20 9:20 PM, Laine Stump wrote:
> If virDomainUpdateDeviceFlags() was used to update an <interface>, and
> the interface type changed from type='network' where the network was
> an unmanaged bridge (so actualType == bridge) to type='bridge'
> (i.e. actualType *also* == bridge), the update would fail due to the
> perceived change in type.
> 
> In practice it is okay to switch between any interface types that end
> up using a tap device, since libvirt just needs to attach the device
> to a new bridge. But in this case we were erroneously rejecting it due
> to a conditional that was too restrictive. This is what the code was doing:
> 
>    if (old->type != new->type)
>       [allow update]
>    else
>       if ((oldActual == bridge and newActual == network)
>           || (oldActual == network and newActual == bridge)) {
>           [allow update]
>       else
>           [error]
> 
> In the case described above though, old->type and new->type don't match,
> but oldActual and newActual are both 'bridge', so we get an error.
> 
> This patch changes the inner conditional so that any combination of
> 'network' and 'bridge' for oldActual and newActual, since they both
> use a tap device connected to a bridge.
> 
> Signed-off-by: Laine Stump <laine@redhat.com>
> ---
>   src/qemu/qemu_hotplug.c | 16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Michal

On 6/3/20 4:20 PM, Laine Stump wrote:
> If virDomainUpdateDeviceFlags() was used to update an <interface>, and
> the interface type changed from type='network' where the network was
> an unmanaged bridge (so actualType == bridge) to type='bridge'
> (i.e. actualType *also* == bridge), the update would fail due to the
> perceived change in type.
> 
> In practice it is okay to switch between any interface types that end
> up using a tap device, since libvirt just needs to attach the device
> to a new bridge. But in this case we were erroneously rejecting it due
> to a conditional that was too restrictive. This is what the code was doing:
> 
>    if (old->type != new->type)
>       [allow update]
>    else
>       if ((oldActual == bridge and newActual == network)
>           || (oldActual == network and newActual == bridge)) {
>           [allow update]
>       else
>           [error]
> 
> In the case described above though, old->type and new->type don't match,
> but oldActual and newActual are both 'bridge', so we get an error.
> 
> This patch changes the inner conditional so that any combination of
> 'network' and 'bridge' for oldActual and newActual, since they both
> use a tap device connected to a bridge.
> 
> Signed-off-by: Laine Stump <laine@redhat.com>
> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>