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

[Eric Blake <eblake@redhat.com>]

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

On 05/24/2013 12:44 AM, Amos Kong wrote:
> We want to implement mac programming over macvtap through Libvirt. The
> related rx-filter information of the nic contains main mac, rx-mode
> items.
> 
> This patch adds a QMP event to notify management of rx-filter change,
> and adds a monitor command for management to query rx-filter
> information.
> 
> A flag for each nic is used to avoid events flooding, if management
> doesn't query rx-filter after it receives one event, new events won't
> be emitted to QMP monitor.
> 
> There maybe exist an uncontrollable delay, guests normally expect
> rx-filter updates immediately, but it's another separate issue, we
> can investigate it after Libvirt work is done.
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---

> +++ b/QMP/qmp-events.txt
> @@ -172,6 +172,25 @@ Data:
>    },
>    "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
>  
> +NIC_RX_FILTER_CHANGED
> +-----------------
> +
> +Emitted when rx-filter configuration of nic is changed by the guest.
> +Each nic has a flag to control event emit, the flag is set to false
> +when it emits one event of the nic, the flag is set to true when
> +management queries the rx-filter of the nic. This is used to avoid
> +events flooding.
> +
> +Data:
> +
> +- "name": net client name (json-string)

Missing documentation on the "path" member.

> +
> +{ "event": "NIC_RX_FILTER_CHANGED",
> +  "data": { "name": "vnet0",
> +            "path": "/machine/peripheral/vnet0/virtio-backend" },
> +  "timestamp": { "seconds": 1368697518, "microseconds": 326866 } }
> +}

Hmm - if we are going to add introspection of events, we probably need
to improve this file to call out ALL the events in JSON format (not just
English text describing the event and then an example of an emitted
event) - but that's a different task for a different series.

> +++ b/include/net/net.h
> @@ -74,6 +76,7 @@ struct NetClientState {
>      unsigned receive_disabled : 1;
>      NetClientDestructor *destructor;
>      unsigned int queue_index;
> +    bool rxfilter_notify_enabled:true;

Very unusual to use 'true' instead of '1' for a bitfield length (valid
C, but I've never seen it done in the wild).  Also, the code base has
very few bit fields of type bool (I only found 4: two in
block/raw-posix.c and two in hw/intc/openpic.c); we generally prefer
'int' or 'unsigned' bitfields, where bitfields make sense.  For that
matter, is this really a struct where we are trying to cram as much
information into space such that a bitfield helps us, or can we just use
plain 'bool' instead of trying to make it a bitfield?  And if a bitfield
is important, then why not group this next to the receive_disabled
bitfield so that the two share the same byte, instead of wasting
alignment for two disjoint bitfields?

Everything else looked okay to me.

-- 
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 --]