[libvirt] [PATCH] qemu: set macvtap physdevs online when macvtap is set online

[John Ferlan]

On 04/13/2015 01:44 PM, Laine Stump wrote:
> A further fix for:
> 
>   https://bugzilla.redhat.com/show_bug.cgi?id=1113474
> 
> Since there is no possibility that any type of macvtap will work if
> the parent physdev it's attached to is offline, we should bring the
> physdev online at the same time as the macvtap. When taking the
> macvtap offline, it's also necessary to take the physdev offline for
> macvtap passthrough mode (because the physdev has the same MAC address
> as the macvtap device, so could potentially cause problems with
> misdirected packets during migration, as outlined in commits 829770
> and 879c13). We can't set the physdev offline for other macvtap modes
> 1) because there may be other macvtap devices attached to the same
> physdev in the other modes whereas passthrough mode is exclusive to
> one macvtap at a time, and 2) there's no practical reason to do so
> anyway.
> ---
>  src/qemu/qemu_hotplug.c   |  8 +++-----
>  src/qemu/qemu_interface.c | 29 +++++++++++++++++++++++++++--
>  2 files changed, 30 insertions(+), 7 deletions(-)
> 
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 2f0549e..9be2ea3 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -3931,11 +3931,9 @@ qemuDomainDetachNetDevice(virQEMUDriverPtr driver,
>          VIR_WARN("cannot clear bandwidth setting for device : %s",
>                   detach->ifname);
>  
> -    /* deactivate the tap/macvtap device on the host (currently this
> -     * isn't necessary, as everything done in
> -     * qemuInterfaceStopDevice() is made meaningless when the device
> -     * is deleted anyway, but in the future it may be important, and
> -     * doesn't hurt anything for now)
> +    /* deactivate the tap/macvtap device on the host, which could also
> +     * affect the parent device (e.g. macvtap passthrough mode sets
> +     * the parent device offline)
>       */
>      ignore_value(qemuInterfaceStopDevice(detach));
>  
> diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c
> index 201a7dd..01226ac 100644
> --- a/src/qemu/qemu_interface.c
> +++ b/src/qemu/qemu_interface.c
> @@ -63,7 +63,20 @@ qemuInterfaceStartDevice(virDomainNetDefPtr net)
>                  goto cleanup;
>          }
>          break;
> -    case VIR_DOMAIN_NET_TYPE_DIRECT:
> +
> +    case VIR_DOMAIN_NET_TYPE_DIRECT: {
> +        const char *physdev = virDomainNetGetActualDirectDev(net);
> +        bool isOnline = true;

Seems unnecessary to set = true, since virNetDevGetIFFlag (what
virNetDevGetOnline calls) will set either True or False on success.

Doesn't hurt...  Your call though.

> +
> +        /* set the physdev online if necessary. It may already be up,
> +         * in which case we shouldn't re-up it just in case that causes
> +         * some sort of "blip" in the physdev's status.
> +         */
> +        if (physdev && virNetDevGetOnline(physdev, &isOnline) < 0)
> +            goto cleanup;
> +        if (!isOnline && virNetDevSetOnline(physdev, true) < 0)
> +            goto cleanup;
> +

No issue putting it online in order for us to use it; however, if it is
online before we got here, shouldn't we know that for Stop?

>          /* macvtap devices share their MAC address with the guest
>           * domain, and if they are set online prior to the domain CPUs
>           * being started, the host may send out traffic from this
> @@ -79,6 +92,7 @@ qemuInterfaceStartDevice(virDomainNetDefPtr net)
>          if (virNetDevSetOnline(net->ifname, true) < 0)
>              goto cleanup;
>          break;
> +    }
>  
>      case VIR_DOMAIN_NET_TYPE_USER:
>      case VIR_DOMAIN_NET_TYPE_ETHERNET:
> @@ -146,7 +160,9 @@ qemuInterfaceStopDevice(virDomainNetDefPtr net)
>          }
>          break;
>  
> -    case VIR_DOMAIN_NET_TYPE_DIRECT:
> +    case VIR_DOMAIN_NET_TYPE_DIRECT: {
> +        const char *physdev = virDomainNetGetActualDirectDev(net);
> +
>          /* macvtap interfaces need to be marked !IFF_UP (ie "down") to
>           * prevent any host-generated traffic sent from this interface
>           * from putting bad info into the arp caches of other machines
> @@ -154,7 +170,16 @@ qemuInterfaceStopDevice(virDomainNetDefPtr net)
>           */
>          if (virNetDevSetOnline(net->ifname, false) < 0)
>              goto cleanup;
> +
> +        /* also mark the physdev down for passthrough macvtap, as the
> +         * physdev has the same MAC address as the macvtap device.
> +         */
> +        if (virDomainNetGetActualDirectMode(net) ==
> +            VIR_NETDEV_MACVLAN_MODE_PASSTHRU &&
> +            physdev && virNetDevSetOnline(physdev, false) < 0)
> +            goto cleanup;

Because if we were online at Start and didn't need to put it online, but
then this code puts it offline, what affect does that have on something
else that may have been relying on it being online...

John
>          break;
> +    }
>  
>      case VIR_DOMAIN_NET_TYPE_USER:
>      case VIR_DOMAIN_NET_TYPE_ETHERNET:
>