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

[Michael S. Tsirkin]

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.
> 
> Signed-off-by: Amos Kong <address@hidden>
> ---
>  QMP/qmp-events.txt        | 14 ++++++++++++++
>  hw/net/virtio-net.c       | 12 ++++++++++++
>  include/monitor/monitor.h |  1 +
>  monitor.c                 |  1 +
>  4 files changed, 28 insertions(+)
> 
> diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
> index 92fe5fb..24d62df 100644
> --- a/QMP/qmp-events.txt
> +++ b/QMP/qmp-events.txt
> @@ -154,6 +154,20 @@ Data:
>              "path": "/machine/peripheral/virtio-net-pci-0" },
>    "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
>  
> +MAC_TABLE_CHANGED
> +-----------------
> +
> +Emitted mac-table configuration is changed by the guest.
> +
> +Data:
> +
> +- "name": net client name (json-string)
> +
> +{ "event": "MAC_TABLE_CHANGED",
> +  "data": { "name": "vnet0" },
> +  "timestamp": { "seconds": 1368697518, "microseconds": 326866 }}
> +}
> +
>  DEVICE_TRAY_MOVED
>  -----------------
>  
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index bed0822..a9b8f53 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -21,6 +21,8 @@
>  #include "hw/virtio/virtio-net.h"
>  #include "net/vhost_net.h"
>  #include "hw/virtio/virtio-bus.h"
> +#include "qapi/qmp/qjson.h"
> +#include "monitor/monitor.h"
>  
>  #define VIRTIO_NET_VM_VERSION    11
>  
> @@ -395,6 +397,7 @@ static int virtio_net_handle_rx_mode(VirtIONet *n, 
> uint8_t cmd,
>  {
>      uint8_t on;
>      size_t s;
> +    QObject *event_data;
>  
>      s = iov_to_buf(iov, iov_cnt, 0, &on, sizeof(on));
>      if (s != sizeof(on)) {
> @@ -417,6 +420,10 @@ static int virtio_net_handle_rx_mode(VirtIONet *n, 
> uint8_t cmd,
>          return VIRTIO_NET_ERR;
>      }
>  
> +    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?

> @@ -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?
 
> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> index 1a6cfcf..e88c70f 100644
> --- a/include/monitor/monitor.h
> +++ b/include/monitor/monitor.h
> @@ -40,6 +40,7 @@ typedef enum MonitorEvent {
>      QEVENT_BLOCK_JOB_ERROR,
>      QEVENT_BLOCK_JOB_READY,
>      QEVENT_DEVICE_DELETED,
> +    QEVENT_MAC_TABLE_CHANGED,
>      QEVENT_DEVICE_TRAY_MOVED,
>      QEVENT_SUSPEND,
>      QEVENT_SUSPEND_DISK,
> diff --git a/monitor.c b/monitor.c
> index 62aaebe..9e51545 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -490,6 +490,7 @@ static const char *monitor_event_names[] = {
>      [QEVENT_BLOCK_JOB_ERROR] = "BLOCK_JOB_ERROR",
>      [QEVENT_BLOCK_JOB_READY] = "BLOCK_JOB_READY",
>      [QEVENT_DEVICE_DELETED] = "DEVICE_DELETED",
> +    [QEVENT_MAC_TABLE_CHANGED] = "MAC_TABLE_CHANGED",
>      [QEVENT_DEVICE_TRAY_MOVED] = "DEVICE_TRAY_MOVED",
>      [QEVENT_SUSPEND] = "SUSPEND",
>      [QEVENT_SUSPEND_DISK] = "SUSPEND_DISK",
> -- 
> 1.8.1.4