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

[Laine Stump]

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 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 at linux.vnet.ibm.com>
> ---
>  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).