diff mbox

[v2,2/2] net: introduce command to query mac-table information

Message ID 1368702445-30733-3-git-send-email-akong@redhat.com
State New
Headers show

Commit Message

Amos Kong May 16, 2013, 11:07 a.m. UTC
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.

(qemu) info mac-table vnet0
vnet0:
 \ promisc: on
 \ allmulti: off
 \ alluni: off
 \ nomulti: off
 \ nouni: off
 \ nobcast: off
 \ multi_overflow: off
 \ uni_overflow: off
 \ multicast:
    01:00:5e:00:00:01
    33:33:00:00:00:01
    33:33:ff:12:34:56

Signed-off-by: Amos Kong <akong@redhat.com>
---
 hmp-commands.hx     |  2 ++
 hmp.c               | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 hmp.h               |  1 +
 hw/net/virtio-net.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++
 include/net/net.h   |  2 ++
 monitor.c           |  8 ++++++
 net/net.c           | 38 ++++++++++++++++++++++++++++
 qapi-schema.json    | 57 ++++++++++++++++++++++++++++++++++++++++++
 qmp-commands.hx     | 53 +++++++++++++++++++++++++++++++++++++++
 9 files changed, 295 insertions(+)

Comments

Michael S. Tsirkin May 16, 2013, 12:19 p.m. UTC | #1
On Thu, May 16, 2013 at 07:07:25PM +0800, 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.
> 
> (qemu) info mac-table vnet0
> vnet0:
>  \ promisc: on
>  \ allmulti: off
>  \ alluni: off
>  \ nomulti: off
>  \ nouni: off
>  \ nobcast: off
>  \ multi_overflow: off
>  \ uni_overflow: off
>  \ multicast:
>     01:00:5e:00:00:01
>     33:33:00:00:00:01
>     33:33:ff:12:34:56
> 
> Signed-off-by: Amos Kong <akong@redhat.com>

It's an interesting example.
There are no unicast addresses at all?

I'm guessing you are missing the main MAC.

> ---
>  hmp-commands.hx     |  2 ++
>  hmp.c               | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hmp.h               |  1 +
>  hw/net/virtio-net.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/net/net.h   |  2 ++
>  monitor.c           |  8 ++++++
>  net/net.c           | 38 ++++++++++++++++++++++++++++
>  qapi-schema.json    | 57 ++++++++++++++++++++++++++++++++++++++++++
>  qmp-commands.hx     | 53 +++++++++++++++++++++++++++++++++++++++
>  9 files changed, 295 insertions(+)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 9cea415..e5c1b14 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1639,6 +1639,8 @@ show qdev device model list
>  show roms
>  @item info tpm
>  show the TPM device
> +@item info mac-table [net client name]
> +show the mac-table information for all nics (or for the given nic)
>  @end table
>  ETEXI
>  
> diff --git a/hmp.c b/hmp.c
> index 4fb76ec..3e19df0 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -653,6 +653,77 @@ void hmp_info_tpm(Monitor *mon, const QDict *qdict)
>      qapi_free_TPMInfoList(info_list);
>  }
>  
> +void hmp_info_mac_table(Monitor *mon, const QDict *qdict)
> +{
> +    MacTableInfoList *table_list, *entry;
> +    strList *str_entry;
> +    bool has_name = qdict_haskey(qdict, "name");
> +    const char *name = NULL;
> +
> +    if (has_name) {
> +        name = qdict_get_str(qdict, "name");
> +    }
> +
> +    table_list = qmp_query_mac_table(has_name, name, NULL);
> +    entry = table_list;
> +
> +    while (entry) {
> +        monitor_printf(mon, "%s:\n", entry->value->name);
> +        if (entry->value->has_promisc) {
> +            monitor_printf(mon, " \\ promisc: %s\n",
> +                           entry->value->promisc ? "on" : "off");
> +        }
> +        if (entry->value->has_allmulti) {
> +            monitor_printf(mon, " \\ allmulti: %s\n",
> +                           entry->value->allmulti ? "on" : "off");
> +        }
> +        if (entry->value->has_alluni) {
> +            monitor_printf(mon, " \\ alluni: %s\n",
> +                           entry->value->alluni ? "on" : "off");
> +        }
> +        if (entry->value->has_nomulti) {
> +            monitor_printf(mon, " \\ nomulti: %s\n",
> +                           entry->value->nomulti ? "on" : "off");
> +        }
> +        if (entry->value->has_nouni) {
> +            monitor_printf(mon, " \\ nouni: %s\n",
> +                           entry->value->nouni ? "on" : "off");
> +        }
> +        if (entry->value->has_nobcast) {
> +            monitor_printf(mon, " \\ nobcast: %s\n",
> +                           entry->value->nobcast ? "on" : "off");
> +        }
> +        if (entry->value->has_multi_overflow) {
> +            monitor_printf(mon, " \\ multi_overflow: %s\n",
> +                           entry->value->multi_overflow ? "on" : "off");
> +        }
> +        if (entry->value->has_uni_overflow) {
> +            monitor_printf(mon, " \\ uni_overflow: %s\n",
> +                           entry->value->uni_overflow ? "on" : "off");
> +        }
> +
> +        if (entry->value->has_unicast) {
> +            str_entry = entry->value->unicast;
> +            monitor_printf(mon, " \\ unicast:\n");
> +            while (str_entry) {
> +                monitor_printf(mon, "    %s\n", str_entry->value);
> +                str_entry = str_entry->next;
> +            }
> +        }
> +        if (entry->value->has_multicast) {
> +            str_entry = entry->value->multicast;
> +            monitor_printf(mon, " \\ multicast:\n");
> +            while (str_entry) {
> +                monitor_printf(mon, "    %s\n", str_entry->value);
> +                str_entry = str_entry->next;
> +            }
> +        }
> +
> +        entry = entry->next;
> +    }
> +    qapi_free_MacTableInfoList(table_list);
> +}
> +
>  void hmp_quit(Monitor *mon, const QDict *qdict)
>  {
>      monitor_suspend(mon);
> diff --git a/hmp.h b/hmp.h
> index 95fe76e..278c722 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -37,6 +37,7 @@ void hmp_info_balloon(Monitor *mon, const QDict *qdict);
>  void hmp_info_pci(Monitor *mon, const QDict *qdict);
>  void hmp_info_block_jobs(Monitor *mon, const QDict *qdict);
>  void hmp_info_tpm(Monitor *mon, const QDict *qdict);
> +void hmp_info_mac_table(Monitor *mon, const QDict *qdict);
>  void hmp_quit(Monitor *mon, const QDict *qdict);
>  void hmp_stop(Monitor *mon, const QDict *qdict);
>  void hmp_system_reset(Monitor *mon, const QDict *qdict);
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index a9b8f53..e4b2358 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -194,6 +194,68 @@ static void virtio_net_set_link_status(NetClientState *nc)
>      virtio_net_set_status(vdev, vdev->status);
>  }
>  
> +static MacTableInfo *virtio_net_query_mactable(NetClientState *nc)
> +{
> +    VirtIONet *n = qemu_get_nic_opaque(nc);
> +    MacTableInfo *info;
> +    strList *str_list = NULL;
> +    strList *entry;
> +    int i;
> +
> +    info = g_malloc0(sizeof(*info));
> +    info->name = g_strdup(nc->name);
> +
> +    info->promisc = n->promisc;
> +    info->has_promisc = true;
> +    info->allmulti = n->allmulti;
> +    info->has_allmulti = true;
> +    info->alluni = n->alluni;
> +    info->has_alluni = true;
> +    info->nomulti = n->nomulti;
> +    info->has_nomulti = true;
> +    info->nouni = n->nouni;
> +    info->has_nouni = true;
> +    info->nobcast = n->nobcast;
> +    info->has_nobcast = true;
> +    info->multi_overflow = n->mac_table.multi_overflow;
> +    info->has_multi_overflow = true;
> +    info->uni_overflow = n->mac_table.uni_overflow;
> +    info->has_uni_overflow = true;
> +
> +    for (i = 0; i < n->mac_table.first_multi; i++) {
> +        info->has_unicast = true;
> +        entry = g_malloc0(sizeof(*entry));
> +        entry->value = g_strdup_printf("%.2x:%.2x:%.2x:%.2x:%.2x:%.2x",
> +                                       n->mac_table.macs[i * ETH_ALEN],
> +                                       n->mac_table.macs[i * ETH_ALEN + 1],
> +                                       n->mac_table.macs[i * ETH_ALEN + 2],
> +                                       n->mac_table.macs[i * ETH_ALEN + 3],
> +                                       n->mac_table.macs[i * ETH_ALEN + 4],
> +                                       n->mac_table.macs[i * ETH_ALEN + 5]);
> +        entry->next = str_list;
> +        str_list = entry;
> +    }
> +    info->unicast = str_list;
> +
> +    str_list = NULL;
> +    for (i = n->mac_table.first_multi; i < n->mac_table.in_use; i++) {
> +        info->has_multicast = true;
> +        entry = g_malloc0(sizeof(*entry));
> +        entry->value = g_strdup_printf("%.2x:%.2x:%.2x:%.2x:%.2x:%.2x",
> +                                       n->mac_table.macs[i * ETH_ALEN],
> +                                       n->mac_table.macs[i * ETH_ALEN + 1],
> +                                       n->mac_table.macs[i * ETH_ALEN + 2],
> +                                       n->mac_table.macs[i * ETH_ALEN + 3],
> +                                       n->mac_table.macs[i * ETH_ALEN + 4],
> +                                       n->mac_table.macs[i * ETH_ALEN + 5]);
> +        entry->next = str_list;
> +        str_list = entry;
> +    }
> +    info->multicast = str_list;
> +
> +    return info;
> +}
> +
>  static void virtio_net_reset(VirtIODevice *vdev)
>  {
>      VirtIONet *n = VIRTIO_NET(vdev);
> @@ -1255,6 +1317,7 @@ static NetClientInfo net_virtio_info = {
>      .receive = virtio_net_receive,
>          .cleanup = virtio_net_cleanup,
>      .link_status_changed = virtio_net_set_link_status,
> +    .query_mac_table = virtio_net_query_mactable,
>  };
>  
>  static bool virtio_net_guest_notifier_pending(VirtIODevice *vdev, int idx)
> diff --git a/include/net/net.h b/include/net/net.h
> index 43d85a1..c3ca4ea 100644
> --- a/include/net/net.h
> +++ b/include/net/net.h
> @@ -49,6 +49,7 @@ typedef ssize_t (NetReceiveIOV)(NetClientState *, const struct iovec *, int);
>  typedef void (NetCleanup) (NetClientState *);
>  typedef void (LinkStatusChanged)(NetClientState *);
>  typedef void (NetClientDestructor)(NetClientState *);
> +typedef MacTableInfo *(QueryMacTable)(NetClientState *);
>  
>  typedef struct NetClientInfo {
>      NetClientOptionsKind type;
> @@ -59,6 +60,7 @@ typedef struct NetClientInfo {
>      NetCanReceive *can_receive;
>      NetCleanup *cleanup;
>      LinkStatusChanged *link_status_changed;
> +    QueryMacTable *query_mac_table;
>      NetPoll *poll;
>  } NetClientInfo;
>  
> diff --git a/monitor.c b/monitor.c
> index 9e51545..a01cdbd 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -2765,6 +2765,14 @@ static mon_cmd_t info_cmds[] = {
>          .mhandler.cmd = hmp_info_tpm,
>      },
>      {
> +        .name       = "mac-table",
> +        .args_type  = "name:s?",
> +        .params     = "[net client name]",
> +        .help       = "show the mac-table information for all nics (or"
> +                      " for the given nic)",
> +        .mhandler.cmd = hmp_info_mac_table,
> +    },
> +    {
>          .name       = NULL,
>      },
>  };
> diff --git a/net/net.c b/net/net.c
> index 43a74e4..9ff3006 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -961,6 +961,44 @@ void print_net_client(Monitor *mon, NetClientState *nc)
>                     nc->info_str);
>  }
>  
> +MacTableInfoList *qmp_query_mac_table(bool has_name, const char *name,
> +                                      Error **errp)
> +{
> +    NetClientState *nc;
> +    MacTableInfoList *table_list = NULL, *last_entry = NULL;
> +
> +    QTAILQ_FOREACH(nc, &net_clients, next) {
> +        MacTableInfoList *entry;
> +        MacTableInfo *info;
> +
> +        if (nc->info->type != NET_CLIENT_OPTIONS_KIND_NIC) {
> +            continue;
> +        }
> +        if (has_name && strcmp(nc->name, name) != 0) {
> +            continue;
> +        }
> +
> +        if (nc->info->query_mac_table) {
> +            info = nc->info->query_mac_table(nc);
> +            entry = g_malloc0(sizeof(*entry));
> +            entry->value = info;
> +
> +            if (!table_list) {
> +                table_list = entry;
> +            } else {
> +                last_entry->next = entry;
> +            }
> +            last_entry = entry;
> +        }
> +    }
> +
> +    if (table_list == NULL) {
> +        error_setg(errp, "invalid net client name: %s", name);
> +    }
> +
> +    return table_list;
> +}
> +
>  void do_info_network(Monitor *mon, const QDict *qdict)
>  {
>      NetClientState *nc, *peer;
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 199744a..866650c 100644
> --- a/qapi-schema.json
> +++ 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)
> +#
> +# @nouni: #optional no unicast mode (default: false)
> +#
> +# @nobcast: #optional no broadcast mode (default: false)
> +#
> +# @multi_overflow: #optional multicast overflow mode (default: false)
> +#
> +# @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',
> +    '*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'] }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index ffd130e..66826ab 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -2932,3 +2932,56 @@ Example:
>  <- { "return": {} }
>  
>  EQMP
> +    {
> +        .name       = "query-mac-table",
> +        .args_type  = "name:s?",
> +        .mhandler.cmd_new = qmp_marshal_input_query_mac_table,
> +    },
> +
> +SQMP
> +query-mac-table
> +---------------
> +
> +Show mac-table information.
> +
> +Returns a json-array of mac-table information for all nics (or for the
> +given nic), returning an error if the given nic doesn't exist.
> +
> +Each array entry contains the following:
> +
> +- "name": net client name (jaso-string)
> +- "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)
> +- "unicast": a jason-array of unicast macaddr string (optional)
> +- "multicast": a jason-array of multicast macaddr string (optional)
> +
> +Example:
> +
> +-> { "execute": "query-mac-table", "arguments": { "name": "vnet0" }}
> +<- { "return": [
> +        {
> +            "multi-overflow": false,
> +            "name": "vnet0",
> +            "uni-overflow": false,
> +            "nobcast": false,
> +            "promisc": true,
> +            "multicast": [
> +                "01:00:5e:00:00:01",
> +                "33:33:00:00:00:01",
> +                "33:33:ff:12:34:56"
> +            ],
> +            "nouni": false,
> +            "nomulti": false,
> +            "allmulti": false,
> +            "alluni": false
> +        }
> +      ]
> +   }
> +
> +EQMP
> -- 
> 1.8.1.4
Eric Blake May 16, 2013, 3:38 p.m. UTC | #2
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.
Stefan Hajnoczi May 17, 2013, 7:39 a.m. UTC | #3
On Thu, May 16, 2013 at 07:07:25PM +0800, Amos Kong wrote:
> @@ -961,6 +961,44 @@ void print_net_client(Monitor *mon, NetClientState *nc)
>                     nc->info_str);
>  }
>  
> +MacTableInfoList *qmp_query_mac_table(bool has_name, const char *name,
> +                                      Error **errp)
> +{
> +    NetClientState *nc;
> +    MacTableInfoList *table_list = NULL, *last_entry = NULL;
> +
> +    QTAILQ_FOREACH(nc, &net_clients, next) {
> +        MacTableInfoList *entry;
> +        MacTableInfo *info;
> +
> +        if (nc->info->type != NET_CLIENT_OPTIONS_KIND_NIC) {
> +            continue;
> +        }
> +        if (has_name && strcmp(nc->name, name) != 0) {
> +            continue;
> +        }
> +
> +        if (nc->info->query_mac_table) {
> +            info = nc->info->query_mac_table(nc);
> +            entry = g_malloc0(sizeof(*entry));
> +            entry->value = info;
> +
> +            if (!table_list) {
> +                table_list = entry;
> +            } else {
> +                last_entry->next = entry;
> +            }
> +            last_entry = entry;
> +        }
> +    }
> +
> +    if (table_list == NULL) {
> +        error_setg(errp, "invalid net client name: %s", name);
> +    }

Produces confusing errors:

1. If query-mac-table is used without a name argument and the guest has
   no NIC or no NICs support ->query_mac_table().

2. If the named NIC does not support ->query_mac_table().
Amos Kong May 21, 2013, 3:31 a.m. UTC | #4
On Thu, May 16, 2013 at 03:19:54PM +0300, Michael S. Tsirkin wrote:
> On Thu, May 16, 2013 at 07:07:25PM +0800, 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.
> > 
> > (qemu) info mac-table vnet0
> > vnet0:
> >  \ promisc: on
> >  \ allmulti: off
> >  \ alluni: off
> >  \ nomulti: off
> >  \ nouni: off
> >  \ nobcast: off
> >  \ multi_overflow: off
> >  \ uni_overflow: off
> >  \ multicast:
> >     01:00:5e:00:00:01
> >     33:33:00:00:00:01
> >     33:33:ff:12:34:56
> > 
> > Signed-off-by: Amos Kong <akong@redhat.com>
 
Hi Michael,

> It's an interesting example.
>
> There are no unicast addresses at all?

Currently we save the main MAC in n->mac (not in n->mactable).
I only output the content of n->mactable.

And main MAC doesn't exist in guest's virtio_net_dev->uc, so it's not
outputted.

The main MAC can also be got by 'info network', but not coverted to QMP.
I will add main MAC in query result and add event for its change.

----
I just started a guest by :
qemu .. -device virtio-net-pci,netdev=ndev1,id=id1 -netdev tap,id=ndev1 \
        -device e1000,netdev=ndev2,id=id2 -netdev tap,id=ndev2

The default mac address was used.
    virtio-nic: 52:54:00:12:34:56
     e1000 nic: 52:54:00:12:34:57

52:54:00:12:34:56 doesn't exist in guest's virtio_net_dev->uc.

----
Another example:
I tried to add a new vlan for virtio-nic in guest
 # vconfig add eth1 3 up

then changed mac of eth1
 # ifconfig eth1 hw ether 12:34:00:00:00:00

then I can see a mac address in unicast part of mac-table
 (qemu) info mac-table 
 id1:
 ...
  \ unicast:
     33:33:ff:12:34:56
  \ multicast:
     00:00:00:00:00:00
     01:80:c2:00:00:21
     01:00:5e:00:00:01
     33:33:00:00:00:01
     33:33:ff:00:00:00
 
> I'm guessing you are missing the main MAC.
Amos Kong May 21, 2013, 4:46 a.m. UTC | #5
On Fri, May 17, 2013 at 09:39:31AM +0200, Stefan Hajnoczi wrote:
> On Thu, May 16, 2013 at 07:07:25PM +0800, Amos Kong wrote:

Hi Stefan,

> > @@ -961,6 +961,44 @@ void print_net_client(Monitor *mon, NetClientState *nc)
> >                     nc->info_str);
> >  }
> >  
> > +MacTableInfoList *qmp_query_mac_table(bool has_name, const char *name,
> > +                                      Error **errp)
> > +{
> > +    NetClientState *nc;
> > +    MacTableInfoList *table_list = NULL, *last_entry = NULL;
> > +
> > +    QTAILQ_FOREACH(nc, &net_clients, next) {
> > +        MacTableInfoList *entry;
> > +        MacTableInfo *info;
> > +
> > +        if (nc->info->type != NET_CLIENT_OPTIONS_KIND_NIC) {
> > +            continue;
> > +        }

> > +        if (has_name && strcmp(nc->name, name) != 0) {
> > +            continue;
> > +        }

             if (has_name && strcmp(nc->name, name) != 0) {
                 error_setg(errp, "invalid net client name: %s", name);
                 break;
             }

> > +
> > +        if (nc->info->query_mac_table) {
> > +            info = nc->info->query_mac_table(nc);
> > +            entry = g_malloc0(sizeof(*entry));
> > +            entry->value = info;
> > +
> > +            if (!table_list) {
> > +                table_list = entry;
> > +            } else {
> > +                last_entry->next = entry;
> > +            }
> > +            last_entry = entry;

> > +        }

             } else if (has_name) {
                 error_setg(errp, "net client(%s) doesn't support mac-table querying", name);
                 break;
             }

> > +    }
> > +
> > +    if (table_list == NULL) {
> > +        error_setg(errp, "invalid net client name: %s", name);
> > +    }

         if (table_list == NULL && !error_is_set(errp)) {
             error_setg(errp, "no net client supports mac-table querying");
         }

> 
> Produces confusing errors:
> 
> 1. If query-mac-table is used without a name argument and the guest has
>    no NIC or no NICs support ->query_mac_table().
> 
> 2. If the named NIC does not support ->query_mac_table().

Thanks, will fix it.
Stefan Hajnoczi May 21, 2013, 7:38 a.m. UTC | #6
On Tue, May 21, 2013 at 12:46:09PM +0800, Amos Kong wrote:
> On Fri, May 17, 2013 at 09:39:31AM +0200, Stefan Hajnoczi wrote:
> > On Thu, May 16, 2013 at 07:07:25PM +0800, Amos Kong wrote:
> 
> Hi Stefan,
> 
> > > @@ -961,6 +961,44 @@ void print_net_client(Monitor *mon, NetClientState *nc)
> > >                     nc->info_str);
> > >  }
> > >  
> > > +MacTableInfoList *qmp_query_mac_table(bool has_name, const char *name,
> > > +                                      Error **errp)
> > > +{
> > > +    NetClientState *nc;
> > > +    MacTableInfoList *table_list = NULL, *last_entry = NULL;
> > > +
> > > +    QTAILQ_FOREACH(nc, &net_clients, next) {
> > > +        MacTableInfoList *entry;
> > > +        MacTableInfo *info;
> > > +
> > > +        if (nc->info->type != NET_CLIENT_OPTIONS_KIND_NIC) {
> > > +            continue;
> > > +        }
> 
> > > +        if (has_name && strcmp(nc->name, name) != 0) {
> > > +            continue;
> > > +        }
> 
>              if (has_name && strcmp(nc->name, name) != 0) {
>                  error_setg(errp, "invalid net client name: %s", name);
>                  break;
>              }

We still need to search the other NICs.  Bailing early doesn't work:
imagine we have nic1 and nic2.  If the user invokes query-mac-table nic2
then this code would return an error!

Stefan
Amos Kong May 23, 2013, 4:03 a.m. UTC | #7
On Thu, May 16, 2013 at 09:38:01AM -0600, Eric Blake wrote:
> 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)?

allmulti: receive all multicast packets
nomulti: don't receive multicast packets
normal: only receive multicast (in mac-table) packets

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

Sounds good. I frankly output the raw RX filter controls without
process & integrate.
 
> We don't necessarily need to abbreviate; would it be better giving these
> fields a longer name, such as no-multicast?

I used the abbreviations because they are same as parameters of config
tool. Management don't need this reminder, I will use 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?

Ok.
 
> Is the point of labeling these fields #optional so that you can avoid
> emitting them if they are in their default state?

No.

Currently we only use rx-filter for virtio-nic, some rx-filer may
not be used by other emulated nic, so I used optional.

option 1:
   nic doesn't have the rx filter, nothing emitted (default :false)
   nic have the rx filter, emit the real value no matter it's true/falue

option 2:
   only emit when nic has the rx filter and value is true

option 2 should be right.

> Does it hurt to
> always list all fields, instead of omitting ones in their default state?
> 
> > +#
> > +# @multi_overflow: #optional multicast overflow mode (default: false)
 
...
 
> > +- "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.

Yes, emit them all the time. If nic doesn't have the rx-filter, just set
it to default 'false'.
 
> > +- "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.

So will remove the optional for unicast/multicast
 
> > +- "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.

Thanks.
Jason Wang May 29, 2013, 5:31 a.m. UTC | #8
On 05/16/2013 07:07 PM, 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.
>
> (qemu) info mac-table vnet0
> vnet0:
>  \ promisc: on
>  \ allmulti: off
>  \ alluni: off
>  \ nomulti: off
>  \ nouni: off
>  \ nobcast: off
>  \ multi_overflow: off
>  \ uni_overflow: off
>  \ multicast:
>     01:00:5e:00:00:01
>     33:33:00:00:00:01
>     33:33:ff:12:34:56
>
> Signed-off-by: Amos Kong <akong@redhat.com>

Maybe you also need a command to query the vlan table, or rename the
command as "info filter" and do it here.
> ---
>  hmp-commands.hx     |  2 ++
>  hmp.c               | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hmp.h               |  1 +
>  hw/net/virtio-net.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/net/net.h   |  2 ++
>  monitor.c           |  8 ++++++
>  net/net.c           | 38 ++++++++++++++++++++++++++++
>  qapi-schema.json    | 57 ++++++++++++++++++++++++++++++++++++++++++
>  qmp-commands.hx     | 53 +++++++++++++++++++++++++++++++++++++++
>  9 files changed, 295 insertions(+)
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 9cea415..e5c1b14 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1639,6 +1639,8 @@ show qdev device model list
>  show roms
>  @item info tpm
>  show the TPM device
> +@item info mac-table [net client name]
> +show the mac-table information for all nics (or for the given nic)
>  @end table
>  ETEXI
>  
> diff --git a/hmp.c b/hmp.c
> index 4fb76ec..3e19df0 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -653,6 +653,77 @@ void hmp_info_tpm(Monitor *mon, const QDict *qdict)
>      qapi_free_TPMInfoList(info_list);
>  }
>  
> +void hmp_info_mac_table(Monitor *mon, const QDict *qdict)
> +{
> +    MacTableInfoList *table_list, *entry;
> +    strList *str_entry;
> +    bool has_name = qdict_haskey(qdict, "name");
> +    const char *name = NULL;
> +
> +    if (has_name) {
> +        name = qdict_get_str(qdict, "name");
> +    }
> +
> +    table_list = qmp_query_mac_table(has_name, name, NULL);
> +    entry = table_list;
> +
> +    while (entry) {
> +        monitor_printf(mon, "%s:\n", entry->value->name);
> +        if (entry->value->has_promisc) {
> +            monitor_printf(mon, " \\ promisc: %s\n",
> +                           entry->value->promisc ? "on" : "off");
> +        }
> +        if (entry->value->has_allmulti) {
> +            monitor_printf(mon, " \\ allmulti: %s\n",
> +                           entry->value->allmulti ? "on" : "off");
> +        }
> +        if (entry->value->has_alluni) {
> +            monitor_printf(mon, " \\ alluni: %s\n",
> +                           entry->value->alluni ? "on" : "off");
> +        }
> +        if (entry->value->has_nomulti) {
> +            monitor_printf(mon, " \\ nomulti: %s\n",
> +                           entry->value->nomulti ? "on" : "off");
> +        }
> +        if (entry->value->has_nouni) {
> +            monitor_printf(mon, " \\ nouni: %s\n",
> +                           entry->value->nouni ? "on" : "off");
> +        }
> +        if (entry->value->has_nobcast) {
> +            monitor_printf(mon, " \\ nobcast: %s\n",
> +                           entry->value->nobcast ? "on" : "off");
> +        }
> +        if (entry->value->has_multi_overflow) {
> +            monitor_printf(mon, " \\ multi_overflow: %s\n",
> +                           entry->value->multi_overflow ? "on" : "off");
> +        }
> +        if (entry->value->has_uni_overflow) {
> +            monitor_printf(mon, " \\ uni_overflow: %s\n",
> +                           entry->value->uni_overflow ? "on" : "off");
> +        }
> +
> +        if (entry->value->has_unicast) {
> +            str_entry = entry->value->unicast;
> +            monitor_printf(mon, " \\ unicast:\n");
> +            while (str_entry) {
> +                monitor_printf(mon, "    %s\n", str_entry->value);
> +                str_entry = str_entry->next;
> +            }
> +        }
> +        if (entry->value->has_multicast) {
> +            str_entry = entry->value->multicast;
> +            monitor_printf(mon, " \\ multicast:\n");
> +            while (str_entry) {
> +                monitor_printf(mon, "    %s\n", str_entry->value);
> +                str_entry = str_entry->next;
> +            }
> +        }
> +
> +        entry = entry->next;
> +    }
> +    qapi_free_MacTableInfoList(table_list);
> +}
> +
>  void hmp_quit(Monitor *mon, const QDict *qdict)
>  {
>      monitor_suspend(mon);
> diff --git a/hmp.h b/hmp.h
> index 95fe76e..278c722 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -37,6 +37,7 @@ void hmp_info_balloon(Monitor *mon, const QDict *qdict);
>  void hmp_info_pci(Monitor *mon, const QDict *qdict);
>  void hmp_info_block_jobs(Monitor *mon, const QDict *qdict);
>  void hmp_info_tpm(Monitor *mon, const QDict *qdict);
> +void hmp_info_mac_table(Monitor *mon, const QDict *qdict);
>  void hmp_quit(Monitor *mon, const QDict *qdict);
>  void hmp_stop(Monitor *mon, const QDict *qdict);
>  void hmp_system_reset(Monitor *mon, const QDict *qdict);
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index a9b8f53..e4b2358 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -194,6 +194,68 @@ static void virtio_net_set_link_status(NetClientState *nc)
>      virtio_net_set_status(vdev, vdev->status);
>  }
>  
> +static MacTableInfo *virtio_net_query_mactable(NetClientState *nc)
> +{
> +    VirtIONet *n = qemu_get_nic_opaque(nc);
> +    MacTableInfo *info;
> +    strList *str_list = NULL;
> +    strList *entry;
> +    int i;
> +
> +    info = g_malloc0(sizeof(*info));
> +    info->name = g_strdup(nc->name);
> +
> +    info->promisc = n->promisc;
> +    info->has_promisc = true;
> +    info->allmulti = n->allmulti;
> +    info->has_allmulti = true;
> +    info->alluni = n->alluni;
> +    info->has_alluni = true;
> +    info->nomulti = n->nomulti;
> +    info->has_nomulti = true;
> +    info->nouni = n->nouni;
> +    info->has_nouni = true;
> +    info->nobcast = n->nobcast;
> +    info->has_nobcast = true;
> +    info->multi_overflow = n->mac_table.multi_overflow;
> +    info->has_multi_overflow = true;
> +    info->uni_overflow = n->mac_table.uni_overflow;
> +    info->has_uni_overflow = true;
> +
> +    for (i = 0; i < n->mac_table.first_multi; i++) {
> +        info->has_unicast = true;
> +        entry = g_malloc0(sizeof(*entry));
> +        entry->value = g_strdup_printf("%.2x:%.2x:%.2x:%.2x:%.2x:%.2x",
> +                                       n->mac_table.macs[i * ETH_ALEN],
> +                                       n->mac_table.macs[i * ETH_ALEN + 1],
> +                                       n->mac_table.macs[i * ETH_ALEN + 2],
> +                                       n->mac_table.macs[i * ETH_ALEN + 3],
> +                                       n->mac_table.macs[i * ETH_ALEN + 4],
> +                                       n->mac_table.macs[i * ETH_ALEN + 5]);
> +        entry->next = str_list;
> +        str_list = entry;
> +    }
> +    info->unicast = str_list;
> +
> +    str_list = NULL;
> +    for (i = n->mac_table.first_multi; i < n->mac_table.in_use; i++) {
> +        info->has_multicast = true;
> +        entry = g_malloc0(sizeof(*entry));
> +        entry->value = g_strdup_printf("%.2x:%.2x:%.2x:%.2x:%.2x:%.2x",
> +                                       n->mac_table.macs[i * ETH_ALEN],
> +                                       n->mac_table.macs[i * ETH_ALEN + 1],
> +                                       n->mac_table.macs[i * ETH_ALEN + 2],
> +                                       n->mac_table.macs[i * ETH_ALEN + 3],
> +                                       n->mac_table.macs[i * ETH_ALEN + 4],
> +                                       n->mac_table.macs[i * ETH_ALEN + 5]);
> +        entry->next = str_list;
> +        str_list = entry;
> +    }
> +    info->multicast = str_list;
> +
> +    return info;
> +}
> +
>  static void virtio_net_reset(VirtIODevice *vdev)
>  {
>      VirtIONet *n = VIRTIO_NET(vdev);
> @@ -1255,6 +1317,7 @@ static NetClientInfo net_virtio_info = {
>      .receive = virtio_net_receive,
>          .cleanup = virtio_net_cleanup,
>      .link_status_changed = virtio_net_set_link_status,
> +    .query_mac_table = virtio_net_query_mactable,
>  };
>  
>  static bool virtio_net_guest_notifier_pending(VirtIODevice *vdev, int idx)
> diff --git a/include/net/net.h b/include/net/net.h
> index 43d85a1..c3ca4ea 100644
> --- a/include/net/net.h
> +++ b/include/net/net.h
> @@ -49,6 +49,7 @@ typedef ssize_t (NetReceiveIOV)(NetClientState *, const struct iovec *, int);
>  typedef void (NetCleanup) (NetClientState *);
>  typedef void (LinkStatusChanged)(NetClientState *);
>  typedef void (NetClientDestructor)(NetClientState *);
> +typedef MacTableInfo *(QueryMacTable)(NetClientState *);
>  
>  typedef struct NetClientInfo {
>      NetClientOptionsKind type;
> @@ -59,6 +60,7 @@ typedef struct NetClientInfo {
>      NetCanReceive *can_receive;
>      NetCleanup *cleanup;
>      LinkStatusChanged *link_status_changed;
> +    QueryMacTable *query_mac_table;
>      NetPoll *poll;
>  } NetClientInfo;
>  
> diff --git a/monitor.c b/monitor.c
> index 9e51545..a01cdbd 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -2765,6 +2765,14 @@ static mon_cmd_t info_cmds[] = {
>          .mhandler.cmd = hmp_info_tpm,
>      },
>      {
> +        .name       = "mac-table",
> +        .args_type  = "name:s?",
> +        .params     = "[net client name]",
> +        .help       = "show the mac-table information for all nics (or"
> +                      " for the given nic)",
> +        .mhandler.cmd = hmp_info_mac_table,
> +    },
> +    {
>          .name       = NULL,
>      },
>  };
> diff --git a/net/net.c b/net/net.c
> index 43a74e4..9ff3006 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -961,6 +961,44 @@ void print_net_client(Monitor *mon, NetClientState *nc)
>                     nc->info_str);
>  }
>  
> +MacTableInfoList *qmp_query_mac_table(bool has_name, const char *name,
> +                                      Error **errp)
> +{
> +    NetClientState *nc;
> +    MacTableInfoList *table_list = NULL, *last_entry = NULL;
> +
> +    QTAILQ_FOREACH(nc, &net_clients, next) {
> +        MacTableInfoList *entry;
> +        MacTableInfo *info;
> +
> +        if (nc->info->type != NET_CLIENT_OPTIONS_KIND_NIC) {
> +            continue;
> +        }
> +        if (has_name && strcmp(nc->name, name) != 0) {
> +            continue;
> +        }
> +
> +        if (nc->info->query_mac_table) {
> +            info = nc->info->query_mac_table(nc);
> +            entry = g_malloc0(sizeof(*entry));
> +            entry->value = info;
> +
> +            if (!table_list) {
> +                table_list = entry;
> +            } else {
> +                last_entry->next = entry;
> +            }
> +            last_entry = entry;
> +        }
> +    }
> +
> +    if (table_list == NULL) {
> +        error_setg(errp, "invalid net client name: %s", name);
> +    }
> +
> +    return table_list;
> +}
> +
>  void do_info_network(Monitor *mon, const QDict *qdict)
>  {
>      NetClientState *nc, *peer;
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 199744a..866650c 100644
> --- a/qapi-schema.json
> +++ 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)
> +#
> +# @nouni: #optional no unicast mode (default: false)
> +#
> +# @nobcast: #optional no broadcast mode (default: false)
> +#
> +# @multi_overflow: #optional multicast overflow mode (default: false)
> +#
> +# @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',
> +    '*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'] }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index ffd130e..66826ab 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -2932,3 +2932,56 @@ Example:
>  <- { "return": {} }
>  
>  EQMP
> +    {
> +        .name       = "query-mac-table",
> +        .args_type  = "name:s?",
> +        .mhandler.cmd_new = qmp_marshal_input_query_mac_table,
> +    },
> +
> +SQMP
> +query-mac-table
> +---------------
> +
> +Show mac-table information.
> +
> +Returns a json-array of mac-table information for all nics (or for the
> +given nic), returning an error if the given nic doesn't exist.
> +
> +Each array entry contains the following:
> +
> +- "name": net client name (jaso-string)
> +- "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)
> +- "unicast": a jason-array of unicast macaddr string (optional)
> +- "multicast": a jason-array of multicast macaddr string (optional)
> +
> +Example:
> +
> +-> { "execute": "query-mac-table", "arguments": { "name": "vnet0" }}
> +<- { "return": [
> +        {
> +            "multi-overflow": false,
> +            "name": "vnet0",
> +            "uni-overflow": false,
> +            "nobcast": false,
> +            "promisc": true,
> +            "multicast": [
> +                "01:00:5e:00:00:01",
> +                "33:33:00:00:00:01",
> +                "33:33:ff:12:34:56"
> +            ],
> +            "nouni": false,
> +            "nomulti": false,
> +            "allmulti": false,
> +            "alluni": false
> +        }
> +      ]
> +   }
> +
> +EQMP
Amos Kong June 5, 2013, 7:18 a.m. UTC | #9
On Wed, May 29, 2013 at 01:31:12PM +0800, Jason Wang wrote:
> On 05/16/2013 07:07 PM, 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.
> >
> > (qemu) info mac-table vnet0
> > vnet0:
> >  \ promisc: on
> >  \ allmulti: off
> >  \ alluni: off
> >  \ nomulti: off
> >  \ nouni: off
> >  \ nobcast: off
> >  \ multi_overflow: off
> >  \ uni_overflow: off
> >  \ multicast:
> >     01:00:5e:00:00:01
> >     33:33:00:00:00:01
> >     33:33:ff:12:34:56
> >
> > Signed-off-by: Amos Kong <akong@redhat.com>
> 
> Maybe you also need a command to query the vlan table, or rename the
> command as "info filter" and do it here.

Thanks for your reminder.

Yes, we need to include all filters that are used in receive_filter().
It contains main-mac, rx-mode items(mac-table, promisc, unit/multi/broadcast
flags), vlan-table.

It's not good to return all(128) entries of vlan-table to monitor client,
and management only use QMP to query info, so I will drop HMP command.


Amos.
diff mbox

Patch

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 9cea415..e5c1b14 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1639,6 +1639,8 @@  show qdev device model list
 show roms
 @item info tpm
 show the TPM device
+@item info mac-table [net client name]
+show the mac-table information for all nics (or for the given nic)
 @end table
 ETEXI
 
diff --git a/hmp.c b/hmp.c
index 4fb76ec..3e19df0 100644
--- a/hmp.c
+++ b/hmp.c
@@ -653,6 +653,77 @@  void hmp_info_tpm(Monitor *mon, const QDict *qdict)
     qapi_free_TPMInfoList(info_list);
 }
 
+void hmp_info_mac_table(Monitor *mon, const QDict *qdict)
+{
+    MacTableInfoList *table_list, *entry;
+    strList *str_entry;
+    bool has_name = qdict_haskey(qdict, "name");
+    const char *name = NULL;
+
+    if (has_name) {
+        name = qdict_get_str(qdict, "name");
+    }
+
+    table_list = qmp_query_mac_table(has_name, name, NULL);
+    entry = table_list;
+
+    while (entry) {
+        monitor_printf(mon, "%s:\n", entry->value->name);
+        if (entry->value->has_promisc) {
+            monitor_printf(mon, " \\ promisc: %s\n",
+                           entry->value->promisc ? "on" : "off");
+        }
+        if (entry->value->has_allmulti) {
+            monitor_printf(mon, " \\ allmulti: %s\n",
+                           entry->value->allmulti ? "on" : "off");
+        }
+        if (entry->value->has_alluni) {
+            monitor_printf(mon, " \\ alluni: %s\n",
+                           entry->value->alluni ? "on" : "off");
+        }
+        if (entry->value->has_nomulti) {
+            monitor_printf(mon, " \\ nomulti: %s\n",
+                           entry->value->nomulti ? "on" : "off");
+        }
+        if (entry->value->has_nouni) {
+            monitor_printf(mon, " \\ nouni: %s\n",
+                           entry->value->nouni ? "on" : "off");
+        }
+        if (entry->value->has_nobcast) {
+            monitor_printf(mon, " \\ nobcast: %s\n",
+                           entry->value->nobcast ? "on" : "off");
+        }
+        if (entry->value->has_multi_overflow) {
+            monitor_printf(mon, " \\ multi_overflow: %s\n",
+                           entry->value->multi_overflow ? "on" : "off");
+        }
+        if (entry->value->has_uni_overflow) {
+            monitor_printf(mon, " \\ uni_overflow: %s\n",
+                           entry->value->uni_overflow ? "on" : "off");
+        }
+
+        if (entry->value->has_unicast) {
+            str_entry = entry->value->unicast;
+            monitor_printf(mon, " \\ unicast:\n");
+            while (str_entry) {
+                monitor_printf(mon, "    %s\n", str_entry->value);
+                str_entry = str_entry->next;
+            }
+        }
+        if (entry->value->has_multicast) {
+            str_entry = entry->value->multicast;
+            monitor_printf(mon, " \\ multicast:\n");
+            while (str_entry) {
+                monitor_printf(mon, "    %s\n", str_entry->value);
+                str_entry = str_entry->next;
+            }
+        }
+
+        entry = entry->next;
+    }
+    qapi_free_MacTableInfoList(table_list);
+}
+
 void hmp_quit(Monitor *mon, const QDict *qdict)
 {
     monitor_suspend(mon);
diff --git a/hmp.h b/hmp.h
index 95fe76e..278c722 100644
--- a/hmp.h
+++ b/hmp.h
@@ -37,6 +37,7 @@  void hmp_info_balloon(Monitor *mon, const QDict *qdict);
 void hmp_info_pci(Monitor *mon, const QDict *qdict);
 void hmp_info_block_jobs(Monitor *mon, const QDict *qdict);
 void hmp_info_tpm(Monitor *mon, const QDict *qdict);
+void hmp_info_mac_table(Monitor *mon, const QDict *qdict);
 void hmp_quit(Monitor *mon, const QDict *qdict);
 void hmp_stop(Monitor *mon, const QDict *qdict);
 void hmp_system_reset(Monitor *mon, const QDict *qdict);
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index a9b8f53..e4b2358 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -194,6 +194,68 @@  static void virtio_net_set_link_status(NetClientState *nc)
     virtio_net_set_status(vdev, vdev->status);
 }
 
+static MacTableInfo *virtio_net_query_mactable(NetClientState *nc)
+{
+    VirtIONet *n = qemu_get_nic_opaque(nc);
+    MacTableInfo *info;
+    strList *str_list = NULL;
+    strList *entry;
+    int i;
+
+    info = g_malloc0(sizeof(*info));
+    info->name = g_strdup(nc->name);
+
+    info->promisc = n->promisc;
+    info->has_promisc = true;
+    info->allmulti = n->allmulti;
+    info->has_allmulti = true;
+    info->alluni = n->alluni;
+    info->has_alluni = true;
+    info->nomulti = n->nomulti;
+    info->has_nomulti = true;
+    info->nouni = n->nouni;
+    info->has_nouni = true;
+    info->nobcast = n->nobcast;
+    info->has_nobcast = true;
+    info->multi_overflow = n->mac_table.multi_overflow;
+    info->has_multi_overflow = true;
+    info->uni_overflow = n->mac_table.uni_overflow;
+    info->has_uni_overflow = true;
+
+    for (i = 0; i < n->mac_table.first_multi; i++) {
+        info->has_unicast = true;
+        entry = g_malloc0(sizeof(*entry));
+        entry->value = g_strdup_printf("%.2x:%.2x:%.2x:%.2x:%.2x:%.2x",
+                                       n->mac_table.macs[i * ETH_ALEN],
+                                       n->mac_table.macs[i * ETH_ALEN + 1],
+                                       n->mac_table.macs[i * ETH_ALEN + 2],
+                                       n->mac_table.macs[i * ETH_ALEN + 3],
+                                       n->mac_table.macs[i * ETH_ALEN + 4],
+                                       n->mac_table.macs[i * ETH_ALEN + 5]);
+        entry->next = str_list;
+        str_list = entry;
+    }
+    info->unicast = str_list;
+
+    str_list = NULL;
+    for (i = n->mac_table.first_multi; i < n->mac_table.in_use; i++) {
+        info->has_multicast = true;
+        entry = g_malloc0(sizeof(*entry));
+        entry->value = g_strdup_printf("%.2x:%.2x:%.2x:%.2x:%.2x:%.2x",
+                                       n->mac_table.macs[i * ETH_ALEN],
+                                       n->mac_table.macs[i * ETH_ALEN + 1],
+                                       n->mac_table.macs[i * ETH_ALEN + 2],
+                                       n->mac_table.macs[i * ETH_ALEN + 3],
+                                       n->mac_table.macs[i * ETH_ALEN + 4],
+                                       n->mac_table.macs[i * ETH_ALEN + 5]);
+        entry->next = str_list;
+        str_list = entry;
+    }
+    info->multicast = str_list;
+
+    return info;
+}
+
 static void virtio_net_reset(VirtIODevice *vdev)
 {
     VirtIONet *n = VIRTIO_NET(vdev);
@@ -1255,6 +1317,7 @@  static NetClientInfo net_virtio_info = {
     .receive = virtio_net_receive,
         .cleanup = virtio_net_cleanup,
     .link_status_changed = virtio_net_set_link_status,
+    .query_mac_table = virtio_net_query_mactable,
 };
 
 static bool virtio_net_guest_notifier_pending(VirtIODevice *vdev, int idx)
diff --git a/include/net/net.h b/include/net/net.h
index 43d85a1..c3ca4ea 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -49,6 +49,7 @@  typedef ssize_t (NetReceiveIOV)(NetClientState *, const struct iovec *, int);
 typedef void (NetCleanup) (NetClientState *);
 typedef void (LinkStatusChanged)(NetClientState *);
 typedef void (NetClientDestructor)(NetClientState *);
+typedef MacTableInfo *(QueryMacTable)(NetClientState *);
 
 typedef struct NetClientInfo {
     NetClientOptionsKind type;
@@ -59,6 +60,7 @@  typedef struct NetClientInfo {
     NetCanReceive *can_receive;
     NetCleanup *cleanup;
     LinkStatusChanged *link_status_changed;
+    QueryMacTable *query_mac_table;
     NetPoll *poll;
 } NetClientInfo;
 
diff --git a/monitor.c b/monitor.c
index 9e51545..a01cdbd 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2765,6 +2765,14 @@  static mon_cmd_t info_cmds[] = {
         .mhandler.cmd = hmp_info_tpm,
     },
     {
+        .name       = "mac-table",
+        .args_type  = "name:s?",
+        .params     = "[net client name]",
+        .help       = "show the mac-table information for all nics (or"
+                      " for the given nic)",
+        .mhandler.cmd = hmp_info_mac_table,
+    },
+    {
         .name       = NULL,
     },
 };
diff --git a/net/net.c b/net/net.c
index 43a74e4..9ff3006 100644
--- a/net/net.c
+++ b/net/net.c
@@ -961,6 +961,44 @@  void print_net_client(Monitor *mon, NetClientState *nc)
                    nc->info_str);
 }
 
+MacTableInfoList *qmp_query_mac_table(bool has_name, const char *name,
+                                      Error **errp)
+{
+    NetClientState *nc;
+    MacTableInfoList *table_list = NULL, *last_entry = NULL;
+
+    QTAILQ_FOREACH(nc, &net_clients, next) {
+        MacTableInfoList *entry;
+        MacTableInfo *info;
+
+        if (nc->info->type != NET_CLIENT_OPTIONS_KIND_NIC) {
+            continue;
+        }
+        if (has_name && strcmp(nc->name, name) != 0) {
+            continue;
+        }
+
+        if (nc->info->query_mac_table) {
+            info = nc->info->query_mac_table(nc);
+            entry = g_malloc0(sizeof(*entry));
+            entry->value = info;
+
+            if (!table_list) {
+                table_list = entry;
+            } else {
+                last_entry->next = entry;
+            }
+            last_entry = entry;
+        }
+    }
+
+    if (table_list == NULL) {
+        error_setg(errp, "invalid net client name: %s", name);
+    }
+
+    return table_list;
+}
+
 void do_info_network(Monitor *mon, const QDict *qdict)
 {
     NetClientState *nc, *peer;
diff --git a/qapi-schema.json b/qapi-schema.json
index 199744a..866650c 100644
--- a/qapi-schema.json
+++ 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)
+#
+# @nouni: #optional no unicast mode (default: false)
+#
+# @nobcast: #optional no broadcast mode (default: false)
+#
+# @multi_overflow: #optional multicast overflow mode (default: false)
+#
+# @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',
+    '*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'] }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index ffd130e..66826ab 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -2932,3 +2932,56 @@  Example:
 <- { "return": {} }
 
 EQMP
+    {
+        .name       = "query-mac-table",
+        .args_type  = "name:s?",
+        .mhandler.cmd_new = qmp_marshal_input_query_mac_table,
+    },
+
+SQMP
+query-mac-table
+---------------
+
+Show mac-table information.
+
+Returns a json-array of mac-table information for all nics (or for the
+given nic), returning an error if the given nic doesn't exist.
+
+Each array entry contains the following:
+
+- "name": net client name (jaso-string)
+- "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)
+- "unicast": a jason-array of unicast macaddr string (optional)
+- "multicast": a jason-array of multicast macaddr string (optional)
+
+Example:
+
+-> { "execute": "query-mac-table", "arguments": { "name": "vnet0" }}
+<- { "return": [
+        {
+            "multi-overflow": false,
+            "name": "vnet0",
+            "uni-overflow": false,
+            "nobcast": false,
+            "promisc": true,
+            "multicast": [
+                "01:00:5e:00:00:01",
+                "33:33:00:00:00:01",
+                "33:33:ff:12:34:56"
+            ],
+            "nouni": false,
+            "nomulti": false,
+            "allmulti": false,
+            "alluni": false
+        }
+      ]
+   }
+
+EQMP