Re: [libvirt] [PATCH 1/2] util: Functions for getting/setting device options

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 provides the utility functions needed to synchronize > the rxfilter changes made to a guest domain with the corresponding > macvtap devices on the host: > > * Get/set PROMISC flag > * Get/set ALLMULTI, MULTICAST > > Signed-off-by: Tony Krowiak <akrowiak(a)linux.vnet.ibm.com&gt; > --- > src/libvirt_private.syms | 6 + > src/util/virnetdev.c | 346 ++++++++++++++++++++++++++++++++++++++++++++++ > src/util/virnetdev.h | 14 ++ > 3 files changed, 366 insertions(+), 0 deletions(-) > > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms > index a2eec83..6b49b08 100644 > --- a/src/libvirt_private.syms > +++ b/src/libvirt_private.syms > @@ -1646,6 +1646,9 @@ virNetDevGetVirtualFunctionInfo; > virNetDevGetVirtualFunctions; > virNetDevGetVLanID; > virNetDevIsOnline; > +virNetDevIsPromiscuous; > +virNetDevIsRcvAllMulti; > +virNetDevIsRcvMulti; > virNetDevIsVirtualFunction; > virNetDevLinkDump; > virNetDevReplaceMacAddress; > @@ -1663,6 +1666,9 @@ virNetDevSetMTUFromDevice; > virNetDevSetName; > virNetDevSetNamespace; > virNetDevSetOnline; > +virNetDevSetPromiscuous; > +virNetDevSetRcvAllMulti; > +virNetDevSetRcvMulti; > virNetDevSetupControl; > virNetDevValidateConfig; > > diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c > index ef96b2b..c610f5b 100644 > --- a/src/util/virnetdev.c > +++ b/src/util/virnetdev.c > @@ -2337,6 +2337,333 @@ int virNetDevDelMulti(const char *ifname ATTRIBUTE_UNUSED, > } > #endif There's a lot of duplicated code here. Rather than completely reimplementing the contents of virNetDev(Set|Is)Online for each of these, how about making two static helper functions: int virNetDevGetIFFlag(const char *ifname, int flag, bool *val) int virNetDevSetIFFlag(const char *ifname, int flag, bool val) then the exported function for each flag would simply be, eg: int virNetDevGetPromiscuous(const char *ifname, bool *promiscuous) { return virNetDevGetIFFlag(ifname, IFF_PROMISC, promiscuous); } int virNetDevSetPromiscuous(const char *ifname, bool promiscuous) { return virNetDevSetIFFlag(ifname, IFF_PROMISC, promiscuous); } (also reimplement virNetDev(Set|Is)Online based on the helper functions) Because IFF_blah may not exist on all platforms, you'll need to provide the stub implementation of the toplevel functions, rather than stubs for virNetDev(Get|Set)IFFlag(), but it's still much less code. Oh, and I would define *all* of these SIOC[SG]IFFLAGS-using functions inside a single #if, rather than a separate one for each. Finally, although the existing function for "Online" uses "Is" as the verb in its name, I think it is more proper to use "Get", so all the functions you named "virNetDevIsBlah" should be named "virNetDevGetBlah" instead (rename virNetDevIsOnline while you're at it - don't forget to change the name in the symfile).

-- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list