[libvirt] [PATCH 6/6] qemu: change macvtap device MAC address in response to NIC_RX_FILTER_CHANGED

[John Ferlan]

On 09/24/2014 05:50 AM, Laine Stump wrote:
> This patch fills in the functionality of
> processNicRxFilterChangedEvent().  It now checks if it is appropriate
> to respond to the NIC_RX_FILTER_CHANGED event (based on device type
> and configuration) and takes appropriate action. Currently it checks
> if the guest interface has been configured with
> trustGuestRxFilters='yes', and if the host side device is macvtap. If
> so, and the MAC address on the guest has changed, the MAC address of
> the macvtap device is changed to match.
> 
> The result of this is that networking from the guest will continue to
> work if the mac address of a macvtap-connected network device is
> changed from within the guest, as long as trustGuestRxFilters='yes'
> (previously changing the MAC address in the guest would break
> networking).
> ---
>  src/qemu/qemu_driver.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 64f1d45..7801d91 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -4146,8 +4146,13 @@ processNicRxFilterChangedEvent(virQEMUDriverPtr driver,
>                                 char *devAlias)
>  {
>      virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> +    qemuDomainObjPrivatePtr priv = vm->privateData;
>      virDomainDeviceDef dev;
>      virDomainNetDefPtr def;
> +    virNetDevRxFilterPtr filter = NULL;
> +    virMacAddr oldMAC;
> +    char newMacStr[VIR_MAC_STRING_BUFLEN];
> +    int ret;
>  
>      VIR_DEBUG("Received NIC_RX_FILTER_CHANGED event for device %s "
>                "from domain %p %s",
> @@ -4175,11 +4180,55 @@ processNicRxFilterChangedEvent(virQEMUDriverPtr driver,
>      }
>      def = dev.data.net;
>  
> +    if (!virDomainNetGetActualTrustGuestRxFilters(def)) {
> +        VIR_WARN("ignore NIC_RX_FILTER_CHANGED event for network "
> +                  "device %s in domain %s",
> +                  def->info.alias, vm->def->name);

So how often could this be emitted? Do we need some sort of "filter" to
only message once per life of the domain?

> +        /* not sending "query-rx-filter" will also suppress any
> +         * further NIC_RX_FILTER_CHANGED events for this device
> +         */
> +        goto endjob;
> +    }
> +
>      /* handle the event - send query-rx-filter and respond to it. */
>  
>      VIR_DEBUG("process NIC_RX_FILTER_CHANGED event for network "
>                "device %s in domain %s", def->info.alias, vm->def->name);
>  

There's no capabilities check necessary?  What if the command doesn't
exist in the underlying qemu?

> +    qemuDomainObjEnterMonitor(driver, vm);
> +    ret = qemuMonitorQueryRxFilter(priv->mon, devAlias, &filter);
> +    qemuDomainObjExitMonitor(driver, vm);
> +    if (ret < 0)
> +        goto endjob;

You filled in a lot of data from the qemuMonitorQueryRxFilter(), but
you're only using filter->mac (e.g. main-mac).

Or am I missing something?

John

> +
> +    virMacAddrFormat(&filter->mac, newMacStr);
> +
> +    if (virDomainNetGetActualType(def) == VIR_DOMAIN_NET_TYPE_DIRECT) {
> +
> +        /* For macvtap connections, set the macvtap device's MAC
> +         * address to match that of the guest device.
> +         */
> +
> +        if (virNetDevGetMAC(def->ifname, &oldMAC) < 0) {
> +            VIR_WARN("Couldn't get current MAC address of device %s "
> +                     "while responding to NIC_RX_FILTER_CHANGED",
> +                     def->ifname);
> +            goto endjob;
> +        }
> +
> +        if (virMacAddrCmp(&oldMAC, &filter->mac)) {
> +            /* set new MAC address from guest to associated macvtap device */
> +            if (virNetDevSetMAC(def->ifname, &filter->mac) < 0) {
> +                VIR_WARN("Couldn't set new MAC address %s to device %s "
> +                         "while responding to NIC_RX_FILTER_CHANGED",
> +                         newMacStr, def->ifname);
> +            } else {
> +                VIR_DEBUG("device %s MAC address set to %s",
> +                          def->ifname, newMacStr);
> +            }
> +        }
> +    }
> +
>   endjob:
>      /* We had an extra reference to vm before starting a new job so ending the
>       * job is guaranteed not to remove the last reference.
> @@ -4187,6 +4236,7 @@ processNicRxFilterChangedEvent(virQEMUDriverPtr driver,
>      ignore_value(qemuDomainObjEndJob(driver, vm));
>  
>   cleanup:
> +    virNetDevRxFilterFree(filter);
>      VIR_FREE(devAlias);
>      virObjectUnref(cfg);
>  }
>