Re: [Qemu-devel] [PATCH v2 1/2] net: introduce MAC_TABLE_CHANGED event

[Michael S. Tsirkin]

On Tue, May 21, 2013 at 01:04:55PM +0800, Amos Kong wrote:
> On Thu, May 16, 2013 at 03:17:45PM +0300, Michael S. Tsirkin wrote:
> > On Thu, May 16, 2013 at 07:07:24PM +0800, Amos Kong wrote:
> > > Introduce this new QMP event to notify management after guest changes
> > > mac-table configuration.
> 
> 
> > > +    event_data = qobject_from_jsonf("{ 'name': %s }", n->netclient_name);
> > > +    monitor_protocol_event(QEVENT_MAC_TABLE_CHANGED, event_data);
> > > +    qobject_decref(event_data);
> > > +
> > >      return VIRTIO_NET_OK;
> > >  }
> > >  
> > 
> > Sorry, pls ignore my previous mail, I see you actually
> > emit this on rx mode change as well.
> > 
> > I find the name misleading or at least it mislead me :)
> > RX_FILTER_CHANGED?
> 
> Agree.
> 
> What we query contain some of rx modes & mac-table content,
> rx_filter is better.
> 
> I will also change monitor cmd to 'query-rx-filter'.
>  
> > > @@ -425,6 +432,7 @@ static int virtio_net_handle_mac(VirtIONet *n, 
> > > uint8_t cmd,
> > >  {
> > >      struct virtio_net_ctrl_mac mac_data;
> > >      size_t s;
> > > +    QObject *event_data;
> > >  
> > >      if (cmd == VIRTIO_NET_CTRL_MAC_ADDR_SET) {
> > >          if (iov_size(iov, iov_cnt) != sizeof(n->mac)) {
> > > @@ -497,6 +505,10 @@ static int virtio_net_handle_mac(VirtIONet *n, 
> > > uint8_t cmd,
> > >          n->mac_table.multi_overflow = 1;
> > >      }
> > >  
> > > +    event_data = qobject_from_jsonf("{ 'name': %s }", n->netclient_name);
> > > +    monitor_protocol_event(QEVENT_MAC_TABLE_CHANGED, event_data);
> > > +    qobject_decref(event_data);
> > > +
> > >      return VIRTIO_NET_OK;
> > >  }
> > > 
> > 
> > This makes it easy for guest to flood management with
> > spurious events.
> 
> > How about we set a flag after this, and avoid sending any more
> > events until management queries the filter status?
> 
> As you discussed in this thread, we need a flag to turn on/off
> the event notification to avoid the flooding.
> 
> But we could not set the flag in first mac-table change to turn off
> the notification. Becase one action(execute one cmd in guest) might
> cause multiple events.

I still think it will work, since the event does not have any
information, what does it matter that we send one and not many events?

> It would be flexible to add a parameter for query-mac-table to change
> the flag. Or add a new command to change the flag.
>  
> 
> -- 
>                       Amos.

Looks a bit too complex, to me.

-- 
MST