[libvirt] [PATCH 2/2] qemu: change macvtap device options in response to NIC_RX_FILTER_CHANGED

[Laine Stump]

(I didn't try make syntax-check, but am assuming you have and that it
passes)


On 01/19/2015 11:18 AM, akrowiak at linux.vnet.ibm.com wrote:
> From: Tony Krowiak <akrowiak at linux.vnet.ibm.com>
>
> This patch supplies the funtionality of synchronizing the host macvtap
> device options with the guest device's in response to the
> NIC_RX_FILTER_CHANGED event.

"supplies the functionality of" sounds too busy and doesn't add
anything. Instead maybe say "This patch enables synchronization of the
host macvtap options ...."

>
> The following device options will be synchronized:
> * PROMISC
> * MULTICAST
> * ALLMULTI
>
> Signed-off-by: Tony Krowiak <akrowiak at linux.vnet.ibm.com>
> ---
>  src/qemu/qemu_driver.c |   92 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 92 insertions(+), 0 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 5994558..141f91a 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -4168,6 +4168,93 @@ syncNicRxFilterHostMulticast(char *ifname, virNetDevRxFilterPtr guestFilter,
>  
>  
>  static void
> +syncNicRxFilterPromiscMode(char *ifname, virNetDevRxFilterPtr guestFilter,
> +                               virNetDevRxFilterPtr hostFilter)
> +{
> +    bool promisc;
> +    bool setpromisc = false;
> +
> +    /* Set macvtap promisc mode to true if the guest has vlans defined */
> +    /* or synchronize the macvtap promisc mode if different from guest */
> +    if (guestFilter->vlan.nTable > 0) {
> +        if (!hostFilter->promiscuous) {
> +            setpromisc = true;
> +            promisc = true;
> +        }
> +    } else if (hostFilter->promiscuous != guestFilter->promiscuous) {
> +        setpromisc = true;
> +        promisc = guestFilter->promiscuous;
> +    }
> +
> +    if (setpromisc) {
> +        if (virNetDevSetPromiscuous(ifname, promisc) < 0) {
> +            VIR_WARN("Couldn't set PROMISC flag to %s for device %s "
> +                     "while responding to NIC_RX_FILTER_CHANGED",
> +                     promisc ? "true" : "false", ifname);
> +        }
> +    }
> +}
> +
> +
> +static void
> +syncNicRxFilterMultiMode(char *ifname, virNetDevRxFilterPtr guestFilter,
> +                         virNetDevRxFilterPtr hostFilter)
> +{
> +    if (hostFilter->multicast.mode != guestFilter->multicast.mode) {
> +        switch (guestFilter->multicast.mode) {

If you typecast the above to virNetDevRxFilterMode, then replace the
"default" below with VIR_NETDEV_RX_FILTER_MODE_NONE, we will get a nice
reminder to add a new case if a new value is ever added to the enum.

> +            case VIR_NETDEV_RX_FILTER_MODE_ALL:
> +                if (virNetDevSetRcvAllMulti(ifname, true)) {
> +
> +                    VIR_WARN("Couldn't set allmulticast flag to 'on' for "
> +                             "device %s while responding to "
> +                             "NIC_RX_FILTER_CHANGED", ifname);
> +                }
> +                break;
> +
> +            case VIR_NETDEV_RX_FILTER_MODE_NORMAL:
> +                if (virNetDevSetRcvMulti(ifname, true)) {
> +
> +                    VIR_WARN("Couldn't set multicast flag to 'on' for "
> +                             "device %s while responding to "
> +                             "NIC_RX_FILTER_CHANGED", ifname);
> +                }
> +
> +                if (virNetDevSetRcvAllMulti(ifname, false)) {
> +                    VIR_WARN("Couldn't set allmulticast flag to 'off' for "
> +                             "device %s while responding to "
> +                             "NIC_RX_FILTER_CHANGED", ifname);
> +                }
> +                break;
> +
> +            default:

I think this should use VIR_NETDEV_RX_FILTER_MODE_NONE instead of
"default". See above.

> +                if (virNetDevSetRcvAllMulti(ifname, false)) {
> +                    VIR_WARN("Couldn't set allmulticast flag to 'off' for "
> +                             "device %s while responding to "
> +                             "NIC_RX_FILTER_CHANGED", ifname);
> +                }
> +
> +                if (virNetDevSetRcvMulti(ifname, false)) {
> +                    VIR_WARN("Couldn't set multicast flag to 'off' for "
> +                             "device %s while responding to "
> +                             "NIC_RX_FILTER_CHANGED",
> +                             ifname);
> +                }
> +                break;
> +        }
> +    }
> +}
> +
> +
> +static void
> +syncNicRxFilterDeviceOptions(char *ifname, virNetDevRxFilterPtr guestFilter,
> +                           virNetDevRxFilterPtr hostFilter)
> +{
> +    syncNicRxFilterPromiscMode(ifname, guestFilter, hostFilter);
> +    syncNicRxFilterMultiMode(ifname, guestFilter, hostFilter);
> +}
> +
> +
> +static void
>  syncNicRxFilterMulticast(char *ifname,
>                           virNetDevRxFilterPtr guestFilter,
>                           virNetDevRxFilterPtr hostFilter)
> @@ -4250,9 +4337,14 @@ processNicRxFilterChangedEvent(virQEMUDriverPtr driver,
>           * attributes to match those of the guest network device:
>           * - MAC address
>           * - Multicast MAC address table
> +         * - Device options:
> +         *   - PROMISC
> +         *   - MULTICAST
> +         *   - ALLMULTI
>           */
>          syncNicRxFilterMacAddr(def->ifname, guestFilter, hostFilter);
>          syncNicRxFilterMulticast(def->ifname, guestFilter, hostFilter);
> +        syncNicRxFilterDeviceOptions(def->ifname, guestFilter, hostFilter);
>      }
>  
>   endjob:

I think this is otherwise okay.