Re: [Qemu-devel] [PATCH v2 2/2] net: introduce command to query mac-table information

[Eric Blake]

On 05/16/2013 05:07 AM, Amos Kong wrote:
> We want to implement mac programming over macvtap through Libvirt.
> The previous patch adds QMP event to notify management of mac-table
> change. This patch adds a monitor command to query rx mode information
> of mac-tables.
> 

Focusing my review on just the QMP interface this go-around:

> +++ b/qapi-schema.json
> @@ -3619,3 +3619,60 @@
>              '*cpuid-input-ecx': 'int',
>              'cpuid-register': 'X86CPURegister32',
>              'features': 'int' } }
> +
> +# @MacTableInfo:
> +#
> +# Rx-mode information of mac-table for a net client.
> +#
> +# @name: the net client name
> +#
> +# @promisc: #optional promiscuous mode (default: false)
> +#
> +# @allmulti: #optional all multicast mode (default: false)
> +#
> +# @alluni: #optional all unicast mode (default: false)
> +#
> +# @nomulti: #optional no multicast mode (default: false)

Are allmulti and numulti orthogonal (all four pairings of possible), or
are they tied together (tri-state)?  If the latter, then maybe it's
better to have an enum value for the three states (none, normal, all)
and a singe @multi that resolves to one of those three states.

We don't necessarily need to abbreviate; would it be better giving these
fields a longer name, such as no-multicast?

> +#
> +# @nouni: #optional no unicast mode (default: false)

Same orthogonal vs. tri-state question about alluni/nouni.

> +#
> +# @nobcast: #optional no broadcast mode (default: false)

Again, if we don't abbreviate, should this be no-broadcast?

Double negatives can be awkward to work with; is it better to name this
'broadcast-allowed' with true being the default?

Is the point of labeling these fields #optional so that you can avoid
emitting them if they are in their default state?  Does it hurt to
always list all fields, instead of omitting ones in their default state?

> +#
> +# @multi_overflow: #optional multicast overflow mode (default: false)

New QMP interfaces should use '-' rather than '_'.

> +#
> +# @uni_overflow: #optional unicast overflow mode (default: false)
> +#
> +# @unicast: #optional a list of unicast macaddr string
> +#
> +# @multicast: #optional a list of multicast macaddr string
> +#
> +# Since 1.6
> +##
> +{ 'type': 'MacTableInfo',
> +  'data': {
> +    'name':            'str',
> +    '*promisc':        'bool',
> +    '*allmulti':       'bool',
> +    '*alluni':         'bool',
> +    '*nomulti':        'bool',
> +    '*nouni':          'bool',
> +    '*nobcast':        'bool',
> +    '*multi-overflow': 'bool',
> +    '*uni-overflow':   'bool',

Oh, you DID use dash, so your doc comments above are mismatched.

> +    '*unicast':        ['str'],
> +    '*multicast':      ['str'] }}
> +
> +##
> +# @query-mac-table:
> +#
> +# Return mac-table information for all nics (or for the given nic).
> +#
> +# @name: #optional net client name
> +#
> +# Returns: list of @MacTableInfo for all nics (or for the given nic).
> +#          Returns an error if the given @name doesn't exist.
> +#
> +# Since: 1.6
> +##
> +{ 'command': 'query-mac-table', 'data': { '*name': 'str' },
> +  'returns': ['MacTableInfo'] }

I don't know if we answered the generic question of whether query-
commands should allow filtering; I kind of like it, though.  And for
this particular command, where we have an event telling us WHICH device
had a change to the table, libvirt is likely to take advantage of the
filtering (different from the query-command-line-options where I argued
that libvirt is unlikely to use the filtering).

> +
> +Each array entry contains the following:
> +
> +- "name": net client name (jaso-string)

s/jaso/json/

> +- "promisc": promiscuous mode (json-bool, optional)
> +- "allmulti": all multicast mode (json-bool, optional)
> +- "alluni": all unicast mode (json-bool, optional)
> +- "nomulti":no multicast mode (json-bool, optional)
> +- "nouni": no unicast mode (json-bool, optional)
> +- "nobcast": no broadcast mode (json-bool, optional)
> +- "multi-overflow": multicast overflow mode (json-bool, optional)
> +- "uni-overflow": unicast overflow mode (json-bool, optional)

For all of the optional bools, please document what the default is if
the field is omitted.  Or maybe we should just always emit them.

> +- "unicast": a jason-array of unicast macaddr string (optional)

s/jason/json/

Isn't it better to omit a 0-length array when there is explicitly no
unicast MAC registered, rather than omitting the field?  An omitted
field implies that there is not enough information available to decide
how many unicast addresses are registered.

> +- "multicast": a jason-array of multicast macaddr string (optional)

Likewise on preferring a 0-length array rather than omitting the field
altogether.

Looking forward to v2.

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

signature.asc
Description: OpenPGP digital signature