Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4673 bytes --]

On 05/23/2013 03:08 AM, Amos Kong wrote:
> We want to implement mac programming over macvtap through Libvirt,
> related rx-filter configuration contains main mac, some of rx-mode
> and mac-table.
> 
> The previous patch adds QMP event to notify management of rx-filter
> change. This patch adds a monitor command to query rx-filter
> information.
> 
> A flag is used to avoid events flooding, if user don't query

s/don't/doesn't/

> rx-filter after receives one event, new events won't be sent

s/after/after it/

> to qmp monitor.
> 
> +++ b/net/net.c
> @@ -961,6 +961,53 @@ void print_net_client(Monitor *mon, NetClientState *nc)
>                     nc->info_str);
>  }
>  
> +RxFilterInfoList *qmp_query_rx_filter(bool has_name, const char *name,
> +                                      Error **errp)
> +{
> +    NetClientState *nc;
> +    RxFilterInfoList *filter_list = NULL, *last_entry = NULL;
> +
> +    QTAILQ_FOREACH(nc, &net_clients, next) {
> +        RxFilterInfoList *entry;
> +        RxFilterInfo *info;
> +
> +        if (nc->info->type != NET_CLIENT_OPTIONS_KIND_NIC) {
> +            continue;
> +        }
> +        if (has_name && strcmp(nc->name, name) != 0) {

Do you need the has_name argument here, or can you ensure that the
caller passes NULL when the caller's has_name was false, for one less
parameter and the same amount of information?

> +++ b/qapi-schema.json
> @@ -3619,3 +3619,76 @@
>              '*cpuid-input-ecx': 'int',
>              'cpuid-register': 'X86CPURegister32',
>              'features': 'int' } }
> +
> +##
> +# @RxState:
> +#
> +# Packets receiving state
> +#
> +# @normal: filter assigned packets according to the mac-table
> +#
> +# @no: don't receive any assigned packet
> +#
> +# @all: receive all assigned packets
> +#
> +##
> +{ 'enum': 'RxState', 'data': [ 'normal', 'no', 'all' ] }

I think s/no/none/ would make slightly more sense (usually, you pair
no/yes and none/all, not no/all).

> +
> +##
> +# @RxFilterInfo:
> +#
> +# Rx-filter information for a net client, it contains main mac, some
> +# of rx-mode items and mac-table.
> +#
> +# @name: net client name
> +#
> +# @promiscuous: whether to ether promiscuous mode

s/to ether//; s/$/is enabled/

> +#
> +# @multicast: multicast receive state
> +#
> +# @unicast: unicast receive state
> +#
> +# @broadcast-allowed: whether to receive broadcast
> +#
> +# @multicast-overflow: multicast table is overflow or not
> +#
> +# @unicast-overflow: unicast table is overflow or not
> +#
> +# @main-mac: the main macaddr string
> +#
> +# @unicast-table: a list of unicast macaddr string
> +#
> +# @multicast-table: a list of multicast macaddr string

Naming is reasonable; thanks for improving things from v1.

> +#
> +# Since 1.6
> +##
> +
> +{ 'type': 'RxFilterInfo',
> +  'data': {
> +    'name':               'str',
> +    'promiscuous':        'bool',
> +    'multicast':          'RxState',
> +    'unicast':            'RxState',
> +    'broadcast-allowed':  'bool',
> +    'multicast-overflow': 'bool',
> +    'unicast-overflow':   'bool',
> +    'main-mac':           'str',
> +    'unicast-table':      ['str'],
> +    'multicast-table':    ['str'] }}
> +
> +##
> +# @query-rx-filter:
> +#
> +# Return rx-filter information for all nics (or for the given nic).
> +#
> +# @name: #optional net client name
> +#
> +# Returns: list of @RxFilterInfo for all nics (or for the given nic).
> +#          Returns an error if the given @name doesn't exist, or given
> +#          nic doesn't support rx-filter querying, or no net client
> +#          supports rx-filter querying
> +#
> +# Since: 1.6
> +##
> +{ 'command': 'query-rx-filter', 'data': { '*name': 'str' },
> +  'returns': ['RxFilterInfo'] }

Interface looks reasonable.  I didn't check the code closely, but agree
with Michael's assessment that the event-suppression flag has to be
per-device, not global.

> +
> +Each array entry contains the following:
> +
> +- "name": net client name (jaso-string)
> +- "promiscuous": enter promiscuous mode (json-bool)
> +- "multicast": multicast receive state (one of 'normal', 'no', 'all')
> +- "unicast": unicast receive state  (one of 'normal', 'no', 'all')
> +- "broadcast-allowed": allow to receive broadcast (json-bool)
> +- "multicast-overflow": multicast table is overflow (json-bool)

s/is overflow/overflowed/

> +- "unicast-overflow": unicast table is overflow (json-bool)

and again

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]