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

(I didn't try make syntax-check, but am assuming you have and that it passes) On 01/19/2015 11:18 AM, akrowiak(a)linux.vnet.ibm.com wrote: > From: Tony Krowiak <akrowiak(a)linux.vnet.ibm.com&gt; > > 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(a)linux.vnet.ibm.com&gt; > --- > 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. -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list