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

[Amos Kong]

On Fri, May 24, 2013 at 06:42:34AM -0600, Eric Blake wrote:
> 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 <address@hidden>
> > ---
> 
> > +++ 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.
 
fixed.

> > +++ 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?

it's more complex.

receive_disabled was used as a 'bool' not bitfield here. I would like
to add a new variable to indicate if notification is needed.

         unsigned rxfilter_notify_enabled:1;
 
> Everything else looked okay to me.



-- 
                        Amos.