[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filte
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information |
Date: |
Wed, 26 Jun 2013 11:54:20 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.1 (gnu/linux) |
Amos Kong <address@hidden> writes:
> We want to implement mac programming over macvtap through Libvirt,
> related rx-filter configuration contains main mac, some of rx-mode
> and mac-table.
>
> The previous patch adds QMP event to notify management of rx-filter
> change. This patch adds a monitor command to query rx-filter
> information.
>
> A flag is used to avoid events flooding, if user don't query
> rx-filter after receives one event, new events won't be sent
> to qmp monitor.
>
> (qemu) info rx-filter vnet0
> vnet0:
> \ promiscuous: on
> \ multicast: normal
> \ unicast: normal
> \ broadcast-allowed: off
> \ multicast-overflow: off
> \ unicast-overflow: off
> \ main-mac: 52:54:00:12:34:56
> \ unicast-table:
> \ multicast-table:
> 01:00:5e:00:00:01
> 33:33:00:00:00:01
> 33:33:ff:12:34:56
>
> Signed-off-by: Amos Kong <address@hidden>
> ---
> hmp-commands.hx | 2 ++
> hmp.c | 49 ++++++++++++++++++++++++++++
> hmp.h | 1 +
> hw/net/virtio-net.c | 93
> +++++++++++++++++++++++++++++++++++++++++++++++++++++
> include/net/net.h | 2 ++
> monitor.c | 8 +++++
> net/net.c | 47 +++++++++++++++++++++++++++
> qapi-schema.json | 73 +++++++++++++++++++++++++++++++++++++++++
> qmp-commands.hx | 55 +++++++++++++++++++++++++++++++
> 9 files changed, 330 insertions(+)
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 9cea415..b7863eb 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
> address@hidden info rx-filter [net client name]
> +show the rx-filter information for all nics (or for the given nic)
Humor me: spell it NICs and NIC, because it's an acronym.
> @end table
> ETEXI
>
> diff --git a/hmp.c b/hmp.c
> index 4fb76ec..5b47382 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -653,6 +653,55 @@ void hmp_info_tpm(Monitor *mon, const QDict *qdict)
> qapi_free_TPMInfoList(info_list);
> }
>
> +void hmp_info_rx_filter(Monitor *mon, const QDict *qdict)
> +{
> + RxFilterInfoList *filter_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");
> + }
> +
> + filter_list = qmp_query_rx_filter(has_name, name, NULL);
Rather roundabout way to do
const char *name = qdict_get_str(qdict, "name");
filter_list = qmp_query_rx_filter(name != NULL, name);
> + entry = filter_list;
> +
> + while (entry) {
Recommend to put the loop control in one place:
for (entry = filter_list; entry; entry = entry->next) {
> + monitor_printf(mon, "%s:\n", entry->value->name);
> + monitor_printf(mon, " \\ promiscuous: %s\n",
> + entry->value->promiscuous ? "on" : "off");
> + monitor_printf(mon, " \\ multicast: %s\n",
> + RxState_lookup[entry->value->multicast]);
> + monitor_printf(mon, " \\ unicast: %s\n",
> + RxState_lookup[entry->value->unicast]);
> + monitor_printf(mon, " \\ broadcast-allowed: %s\n",
> + entry->value->broadcast_allowed ? "on" : "off");
> + monitor_printf(mon, " \\ multicast-overflow: %s\n",
> + entry->value->multicast_overflow ? "on" : "off");
> + monitor_printf(mon, " \\ unicast-overflow: %s\n",
> + entry->value->unicast_overflow ? "on" : "off");
> + monitor_printf(mon, " \\ main-mac: %s\n", entry->value->main_mac);
> +
> + str_entry = entry->value->unicast_table;
> + monitor_printf(mon, " \\ unicast-table:\n");
> + while (str_entry) {
> + monitor_printf(mon, " %s\n", str_entry->value);
> + str_entry = str_entry->next;
> + }
> +
> + str_entry = entry->value->multicast_table;
> + monitor_printf(mon, " \\ multicast-table:\n");
> + while (str_entry) {
> + monitor_printf(mon, " %s\n", str_entry->value);
> + str_entry = str_entry->next;
> + }
> +
> + entry = entry->next;
> + }
> + qapi_free_RxFilterInfoList(filter_list);
> +}
> +
> void hmp_quit(Monitor *mon, const QDict *qdict)
> {
> monitor_suspend(mon);
> diff --git a/hmp.h b/hmp.h
> index 95fe76e..9af733e 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_rx_filter(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 1ea9556..f93e021 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
>
> @@ -192,6 +194,90 @@ static void virtio_net_set_link_status(NetClientState
> *nc)
> virtio_net_set_status(vdev, vdev->status);
> }
>
> +static bool notify_enabled = true;
> +
> +static void rxfilter_notify(const char *name)
> +{
> + QObject *event_data;
> +
> + if (notify_enabled) {
> + event_data = qobject_from_jsonf("{ 'name': %s }", name);
> + monitor_protocol_event(QEVENT_RX_FILTER_CHANGED, event_data);
> + qobject_decref(event_data);
> + /* disable event notification to avoid events flooding */
> + notify_enabled = false;
> + }
> +}
> +
> +static RxFilterInfo *virtio_net_query_rxfilter(NetClientState *nc)
> +{
> + VirtIONet *n = qemu_get_nic_opaque(nc);
> + RxFilterInfo *info;
> + strList *str_list = NULL;
> + strList *entry;
> + int i;
> +
> + info = g_malloc0(sizeof(*info));
> + info->name = g_strdup(nc->name);
> + info->promiscuous = n->promisc;
> +
> + if (n->nouni) {
> + info->unicast = RX_STATE_NO;
> + } else if (n->alluni) {
> + info->unicast = RX_STATE_ALL;
> + } else {
> + info->unicast = RX_STATE_NORMAL;
> + }
> +
> + if (n->nomulti) {
> + info->multicast = RX_STATE_NO;
> + } else if (n->allmulti) {
> + info->multicast = RX_STATE_ALL;
> + } else {
> + info->multicast = RX_STATE_NORMAL;
> + }
Makes me wonder whether replacing VirtIONet members noFOO and allFOO by
an enum RxState would make things clearer there. Outside the scope of
this patch.
> +
> + info->broadcast_allowed = n->nobcast;
> + info->multicast_overflow = n->mac_table.multi_overflow;
> + info->unicast_overflow = n->mac_table.uni_overflow;
> + info->main_mac = g_strdup_printf("%.2x:%.2x:%.2x:%.2x:%.2x:%.2x",
> + n->mac[0], n->mac[1], n->mac[2],
> + n->mac[3], n->mac[4], n->mac[5]);
> +
Please initialize str_list here rather than at its declaration, because
that'll make the loop's workings more locally obvious, and because...
> + for (i = 0; i < n->mac_table.first_multi; i++) {
> + 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_table = str_list;
> +
... it's how this loop works.
Actually, the two loops are duplicates. Consider factoring out a helper
function.
> + str_list = NULL;
> + for (i = n->mac_table.first_multi; i < n->mac_table.in_use; i++) {
> + 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_table = str_list;
> + /* enable event notification after query */
> + notify_enabled = true;
Combined with the optional argument, this leads to rather weird
behavior:
1. Querying all NICs reenables the event for all NICs. Okay.
2. Querying a single virtio-net NIC reenables the event for all
virtio-net NICs. Weird.
Currently, only virtio-net NICs support the event, so the weirdness
isn't visible.
Regardless, please clean this up:
* Either move notify_enabled into device state, or
* Move notify_enabled somewhere global, along with rxfilter_notify().
> +
> + return info;
> +}
> +
> static void virtio_net_reset(VirtIODevice *vdev)
> {
> VirtIONet *n = VIRTIO_NET(vdev);
> @@ -442,6 +528,8 @@ static int virtio_net_handle_rx_mode(VirtIONet *n,
> uint8_t cmd,
> return VIRTIO_NET_ERR;
> }
>
> + rxfilter_notify(n->netclient_name);
> +
> return VIRTIO_NET_OK;
> }
>
> @@ -495,6 +583,8 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t
> cmd,
> s = iov_to_buf(iov, iov_cnt, 0, &n->mac, sizeof(n->mac));
> assert(s == sizeof(n->mac));
> qemu_format_nic_info_str(qemu_get_queue(n->nic), n->mac);
> + rxfilter_notify(n->netclient_name);
> +
> return VIRTIO_NET_OK;
> }
>
> @@ -559,6 +649,8 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t
> cmd,
> n->mac_table.multi_overflow = 1;
> }
>
> + rxfilter_notify(n->netclient_name);
> +
> return VIRTIO_NET_OK;
> }
>
The error returns don't trigger the event. We can fail after clearing
n->mac_table. Why is that okay?
> @@ -1312,6 +1404,7 @@ static NetClientInfo net_virtio_info = {
> .receive = virtio_net_receive,
> .cleanup = virtio_net_cleanup,
> .link_status_changed = virtio_net_set_link_status,
> + .query_rx_filter = virtio_net_query_rxfilter,
> };
>
> 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..0af5ba3 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 RxFilterInfo *(QueryRxFilter)(NetClientState *);
>
> typedef struct NetClientInfo {
> NetClientOptionsKind type;
> @@ -59,6 +60,7 @@ typedef struct NetClientInfo {
> NetCanReceive *can_receive;
> NetCleanup *cleanup;
> LinkStatusChanged *link_status_changed;
> + QueryRxFilter *query_rx_filter;
> NetPoll *poll;
> } NetClientInfo;
>
> diff --git a/monitor.c b/monitor.c
> index 4f7bd48..81ac50c 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -2764,6 +2764,14 @@ static mon_cmd_t info_cmds[] = {
> .mhandler.cmd = hmp_info_tpm,
> },
> {
> + .name = "rx-filter",
nic-rx-filter?
> + .args_type = "name:s?",
> + .params = "[net client name]",
> + .help = "show the rx-filter information for all nics (or"
> + " for the given nic)",
NICs, NIC.
> + .mhandler.cmd = hmp_info_rx_filter,
> + },
> + {
> .name = NULL,
> },
> };
> diff --git a/net/net.c b/net/net.c
> index 43a74e4..7b73a10 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -961,6 +961,53 @@ void print_net_client(Monitor *mon, NetClientState *nc)
> nc->info_str);
> }
>
> +RxFilterInfoList *qmp_query_rx_filter(bool has_name, const char *name,
> + Error **errp)
> +{
> + NetClientState *nc;
> + RxFilterInfoList *filter_list = NULL, *last_entry = NULL;
> +
> + QTAILQ_FOREACH(nc, &net_clients, next) {
> + RxFilterInfoList *entry;
> + RxFilterInfo *info;
> +
> + if (nc->info->type != NET_CLIENT_OPTIONS_KIND_NIC) {
> + continue;
> + }
> + if (has_name && strcmp(nc->name, name) != 0) {
> + continue;
> + }
> +
> + if (nc->info->query_rx_filter) {
> + info = nc->info->query_rx_filter(nc);
> + entry = g_malloc0(sizeof(*entry));
> + entry->value = info;
> +
> + if (!filter_list) {
> + filter_list = entry;
> + } else {
> + last_entry->next = entry;
> + }
> + last_entry = entry;
> + } else if (has_name) {
> + error_setg(errp, "net client(%s) doesn't support"
> + " rx-filter querying", name);
> + break;
> + }
> +
> + }
> +
> + if (filter_list == NULL && !error_is_set(errp)) {
> + if (has_name) {
> + error_setg(errp, "invalid net client name: %s", name);
> + } else {
> + error_setg(errp, "no net client supports rx-filter querying");
Why is this an error?
> + }
> + }
> +
> + return filter_list;
> +}
> +
> void do_info_network(Monitor *mon, const QDict *qdict)
> {
> NetClientState *nc, *peer;
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 664b31f..cac3e16 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3619,3 +3619,76 @@
> '*cpuid-input-ecx': 'int',
> 'cpuid-register': 'X86CPURegister32',
> 'features': 'int' } }
> +
> +##
> +# @RxState:
> +#
> +# Packets receiving state
> +#
> +# @normal: filter assigned packets according to the mac-table
> +#
> +# @no: don't receive any assigned packet
> +#
> +# @all: receive all assigned packets
> +#
> +##
> +{ 'enum': 'RxState', 'data': [ 'normal', 'no', 'all' ] }
> +
> +##
> +# @RxFilterInfo:
> +#
> +# Rx-filter information for a net client, it contains main mac, some
> +# of rx-mode items and mac-table.
> +#
> +# @name: net client name
> +#
> +# @promiscuous: whether to ether promiscuous mode
> +#
> +# @multicast: multicast receive state
> +#
> +# @unicast: unicast receive state
> +#
> +# @broadcast-allowed: whether to receive broadcast
> +#
> +# @multicast-overflow: multicast table is overflow or not
> +#
> +# @unicast-overflow: unicast table is overflow or not
> +#
> +# @main-mac: the main macaddr string
> +#
> +# @unicast-table: a list of unicast macaddr string
> +#
> +# @multicast-table: a list of multicast macaddr string
> +#
> +# Since 1.6
> +##
> +
> +{ 'type': 'RxFilterInfo',
> + 'data': {
> + 'name': 'str',
> + 'promiscuous': 'bool',
> + 'multicast': 'RxState',
> + 'unicast': 'RxState',
> + 'broadcast-allowed': 'bool',
> + 'multicast-overflow': 'bool',
> + 'unicast-overflow': 'bool',
> + 'main-mac': 'str',
> + 'unicast-table': ['str'],
> + 'multicast-table': ['str'] }}
> +
> +##
> +# @query-rx-filter:
> +#
> +# Return rx-filter information for all nics (or for the given nic).
> +#
> +# @name: #optional net client name
> +#
> +# Returns: list of @RxFilterInfo for all nics (or for the given nic).
> +# Returns an error if the given @name doesn't exist, or given
> +# nic doesn't support rx-filter querying, or no net client
> +# supports rx-filter querying
NICs, NIC.
The case "no net client supports rx-filter querying" should return an
empty list, not throw an error.
I think the case "@name exists but isn't a NIC" should be a separate
error.
> +#
> +# Since: 1.6
> +##
> +{ 'command': 'query-rx-filter', 'data': { '*name': 'str' },
query-nic-rx-filter?
Do we want to complicate the QMP interface with the optional argument?
Discussed elsewhere in the thread, no need to answer again here.
> + 'returns': ['RxFilterInfo'] }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index ffd130e..817460b 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -2932,3 +2932,58 @@ Example:
> <- { "return": {} }
>
> EQMP
> + {
> + .name = "query-rx-filter",
> + .args_type = "name:s?",
> + .mhandler.cmd_new = qmp_marshal_input_query_rx_filter,
> + },
> +
> +SQMP
> +query-rx-filter
> +---------------
> +
> +Show rx-filter information.
> +
> +Returns a json-array of rx-filter information for all nics (or for the
> +given nic), returning an error if the given nic doesn't exist, or
> +given nic doesn't support rx-filter querying, or no net client
> +supports rx-filter querying.
Comments on @query-rx-filter in qapi-schema.json apply.
> +
> +Each array entry contains the following:
> +
> +- "name": net client name (jaso-string)
json-string
> +- "promiscuous": enter promiscuous mode (json-bool)
> +- "multicast": multicast receive state (one of 'normal', 'no', 'all')
> +- "unicast": unicast receive state (one of 'normal', 'no', 'all')
> +- "broadcast-allowed": allow to receive broadcast (json-bool)
> +- "multicast-overflow": multicast table is overflow (json-bool)
> +- "unicast-overflow": unicast table is overflow (json-bool)
> +- "main-mac": main macaddr string (jaso-string)
json-string
> +- "unicast-table": a json-array of unicast macaddr string
> +- "multicast-table": a json-array of multicast macaddr string
> +
> +Example:
> +
> +-> { "execute": "query-rx-filter", "arguments": { "name": "vnet0" }}
> +<- { "return": [
> + {
> + "promiscuous": true,
> + "name": "vnet0",
> + "main-mac": "52:54:00:12:34:56",
> + "unicast": "normal",
> + "unicast-table": [
> + ],
> + "multicast": "normal",
> + "multicast-overflow": false,
> + "unicast-overflow": false,
> + "multicast-table": [
> + "01:00:5e:00:00:01",
> + "33:33:00:00:00:01",
> + "33:33:ff:12:34:56"
> + ],
> + "broadcast-allowed": false
> + }
> + ]
> + }
> +
> +EQMP
This interface is abstract in the sense that it applies to all NICs. At
this time, it's implemented only virtio-net implements it. I'm
habitually wary of abstractions based on just one concrete instance,
which makes me ask:
1. Ignorant question first: could the feature make sense for other NICs,
too, or is it specific to virtio-net?
2. If the former, are you reasonably sure this object will do for other
NICs?
- Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information, Markus Armbruster, 2013/06/26
- Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information, Markus Armbruster, 2013/06/26
- Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information,
Markus Armbruster <=
- Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information, Markus Armbruster, 2013/06/26
- Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information, Luiz Capitulino, 2013/06/26
- Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information, Markus Armbruster, 2013/06/26
- Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information, Eric Blake, 2013/06/28
- Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information, Markus Armbruster, 2013/06/28
- Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information, Amos Kong, 2013/06/30