1:21 p.m.
On 09/24/2014 07:45 PM, John Ferlan wrote:
On 09/24/2014 05:50 AM, Laine Stump wrote:
> This same structure will be used to retrieve RX filter info for
> interfaces on the host via netlink messages, and RX filter info for
> interfaces on the guest via the qemu "query-rx-filter" command.
> ---
> src/libvirt_private.syms | 8 +++++++
> src/util/virnetdev.c | 40 +++++++++++++++++++++++++++++++++
> src/util/virnetdev.h | 57 +++++++++++++++++++++++++++++++++++++++++++++++-
> 3 files changed, 104 insertions(+), 1 deletion(-)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index bb2b9a3..e5723d2 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1595,6 +1595,14 @@ virNetDevReplaceMacAddress;
> virNetDevReplaceNetConfig;
> virNetDevRestoreMacAddress;
> virNetDevRestoreNetConfig;
> +virNetDevRxFilterFree;
> +virNetDevRxFilterMulticastModeTypeFromString;
> +virNetDevRxFilterMulticastModeTypeToString;
> +virNetDevRxFilterNew;
> +virNetDevRxFilterUnicastModeTypeFromString;
> +virNetDevRxFilterUnicastModeTypeToString;
> +virNetDevRxFilterVlanModeTypeFromString;
> +virNetDevRxFilterVlanModeTypeToString;
> virNetDevSetIPv4Address;
> virNetDevSetMAC;
> virNetDevSetMTU;
> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
> index 8815e18..dd1f530 100644
> --- a/src/util/virnetdev.c
> +++ b/src/util/virnetdev.c
> @@ -1932,3 +1932,43 @@ virNetDevGetLinkInfo(const char *ifname,
> return 0;
> }
> #endif /* defined(__linux__) */
> +
> +
> +VIR_ENUM_IMPL(virNetDevRxFilterUnicastMode,
> + VIR_NETDEV_RX_FILTER_UNICAST_MODE_LAST,
> + "none",
> + "normal");
> +
> +VIR_ENUM_IMPL(virNetDevRxFilterMulticastMode,
> + VIR_NETDEV_RX_FILTER_MULTICAST_MODE_LAST,
> + "none",
> + "normal");
> +
> +VIR_ENUM_IMPL(virNetDevRxFilterVlanMode,
> + VIR_NETDEV_RX_FILTER_VLAN_MODE_LAST,
> + "none",
> + "normal");
Is there ever a chance these could be different for one of these types?
I looked back through my IRC discussions with Vlad (who worked on the
qemu/kernel parts of this) and found that, while initially he thought
they might have different values, that after looking it up in the qemu
source he found that they have the same potential values. But I had
already forgotten that by the time I got around to the implementation,
and had put in three different enums simply because it's easier to
combine them later than to split them out into separate enums when I
find a difference.
I've just now verified that all three have the same possible values
according to qemu's qmp-commands.hx, so I will combine them into a
single enum.
BTW, I also see that in qemu, the associated enum is called RxState, but
I think it is really more of a mode, and it's only related to the RX
filters, so I'm still going to name it virNetDevRxFilterMode rather than
the less descriptive virNetDevRxState.
Seems "excessive" to make 3 specific definitions when 1
generic one
could suffice (drop the "Unicast, Multicast, Vlan")
> +
> +
> +virNetDevRxFilterPtr
> +virNetDevRxFilterNew(void)
> +{
> + virNetDevRxFilterPtr filter;
> +
> + if (VIR_ALLOC(filter) < 0)
> + return NULL;
> + return filter;
> +}
> +
> +
> +void
> +virNetDevRxFilterFree(virNetDevRxFilterPtr filter)
> +{
> + if (filter) {
> + VIR_FREE(filter->name);
> + VIR_FREE(filter->unicast.table);
> + VIR_FREE(filter->multicast.table);
> + VIR_FREE(filter->vlan.table);
I haven't peeked ahead yet, but are these tables where the allocation is
allocate space for 10 table entries, allocate entries in each array
entry... If so, then the free will need to run through the tables.
They are tables, but each entry is a virMacAddr, *not* a virMacAddrPtr
(if that was the case, the definition would say "virMacAddrPtr *table"),
so there is no extra allocation.
Reason why I ask is I see size_t's for each structure indicating to me
it's a array of entries.
> + VIR_FREE(filter);
> + }
> +}
> diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h
> index 69e365e..307871c 100644
> --- a/src/util/virnetdev.h
> +++ b/src/util/virnetdev.h
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (C) 2007-2013 Red Hat, Inc.
> + * Copyright (C) 2007-2014 Red Hat, Inc.
> *
> * This library is free software; you can redistribute it and/or
> * modify it under the terms of the GNU Lesser General Public
> @@ -37,6 +37,57 @@ typedef struct ifreq virIfreq;
> typedef void virIfreq;
> # endif
>
> +typedef enum {
> + VIR_NETDEV_RX_FILTER_UNICAST_MODE_NONE = 0,
> + VIR_NETDEV_RX_FILTER_UNICAST_MODE_NORMAL,
> +
> + VIR_NETDEV_RX_FILTER_UNICAST_MODE_LAST
> +} virNetDevRxFilterUnicastMode;
> +VIR_ENUM_DECL(virNetDevRxFilterUnicastMode)
> +
> +typedef enum {
> + VIR_NETDEV_RX_FILTER_MULTICAST_MODE_NONE = 0,
> + VIR_NETDEV_RX_FILTER_MULTICAST_MODE_NORMAL,
> +
> + VIR_NETDEV_RX_FILTER_MULTICAST_MODE_LAST
> +} virNetDevRxFilterMulticastMode;
> +VIR_ENUM_DECL(virNetDevRxFilterMulticastMode)
> +
> +typedef enum {
> + VIR_NETDEV_RX_FILTER_VLAN_MODE_NONE = 0,
> + VIR_NETDEV_RX_FILTER_VLAN_MODE_NORMAL,
> +
> + VIR_NETDEV_RX_FILTER_VLAN_MODE_LAST
> +} virNetDevRxFilterVlanMode;
> +VIR_ENUM_DECL(virNetDevRxFilterVlanMode)
Same here with respect to generic rather than specific
> +
> +typedef struct _virNetDevRxFilter virNetDevRxFilter;
> +typedef virNetDevRxFilter *virNetDevRxFilterPtr;
> +struct _virNetDevRxFilter {
> + char *name; /* the alias used by qemu, *not* name used by guest */
> + virMacAddr mac;
> + bool promiscuous;
> + bool broadcastAllowed;
> +
> + struct {
> + int mode; /* enum virNetDevRxFilterUnicastMode */
> + bool overflow;
> + virMacAddrPtr table;
> + size_t nTable;
> + } unicast;
> + struct {
> + int mode; /* enum virNetDevRxFilterMulticastMode */
> + bool overflow;
> + virMacAddrPtr table;
> + size_t nTable;
> + } multicast;
> + struct {
> + int mode; /* enum virNetDevRxFilterVlanMode */
> + unsigned int *table;
> + size_t nTable;
> + } vlan;
> +};
> +
Each of the mode's would just use the generic FilterMode and the
comments adjusted accordingly...
soft ACK with changes depending on future patches which I haven't peeked
at yet.
John
> int virNetDevSetupControl(const char *ifname,
> virIfreq *ifr)
> ATTRIBUTE_RETURN_CHECK;
> @@ -150,4 +201,8 @@ int virNetDevGetLinkInfo(const char *ifname,
> virInterfaceLinkPtr lnk)
> ATTRIBUTE_NONNULL(1);
>
> +virNetDevRxFilterPtr virNetDevRxFilterNew(void)
> + ATTRIBUTE_RETURN_CHECK;
> +void virNetDevRxFilterFree(virNetDevRxFilterPtr filter);
> +
> #endif /* __VIR_NETDEV_H__ */
>