* Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
2013-05-23 9:08 ` [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information Amos Kong
@ 2013-05-23 10:23 ` Michael S. Tsirkin
2013-05-24 4:53 ` Amos Kong
2013-05-23 12:14 ` Eric Blake
` (2 subsequent siblings)
3 siblings, 1 reply; 47+ messages in thread
From: Michael S. Tsirkin @ 2013-05-23 10:23 UTC (permalink / raw)
To: Amos Kong; +Cc: qemu-devel, stefanha, lcapitulino
On Thu, May 23, 2013 at 05:08:00PM +0800, Amos Kong wrote:
> 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 <akong@redhat.com>
> ---
> 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
> +@item info rx-filter [net client name]
> +show the rx-filter information for all nics (or for the given nic)
> @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);
> + entry = filter_list;
> +
> + while (entry) {
> + 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;
Please, make this part of the device state, not a global variable. E.g.
if I query rxfilter for device X, no need to resend events for device Y.
And rename to rxfilter_notify_enabled.
> +
> +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;
> + }
> +
> + 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]);
We really want a helper for this g_strdup_printf thing IMO.
> +
> + 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;
> +
> + 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;
> +
> + 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;
> }
>
> @@ -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",
> + .args_type = "name:s?",
> + .params = "[net client name]",
> + .help = "show the rx-filter information for all nics (or"
> + " for the given 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");
> + }
> + }
> +
> + 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
> +#
> +# Since: 1.6
> +##
> +{ 'command': 'query-rx-filter', 'data': { '*name': 'str' },
> + '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.
> +
> +Each array entry contains the following:
> +
> +- "name": net client name (jaso-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)
> +- "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
> --
> 1.8.1.4
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
2013-05-23 10:23 ` Michael S. Tsirkin
@ 2013-05-24 4:53 ` Amos Kong
0 siblings, 0 replies; 47+ messages in thread
From: Amos Kong @ 2013-05-24 4:53 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel, stefanha, lcapitulino
On Thu, May 23, 2013 at 01:23:40PM +0300, Michael S. Tsirkin wrote:
> On Thu, May 23, 2013 at 05:08:00PM +0800, Amos Kong wrote:
> > + 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]);
>
> We really want a helper for this g_strdup_printf thing IMO.
entry->value = mac_strdup_printf(n->mac_table.macs + i * ETH_ALEN);
static char *mac_strdup_printf(uint8_t *mac)
{
return g_strdup_printf("%.2x:%.2x:%.2x:%.2x:%.2x:%.2x", mac[0],
mac[1], mac[2], mac[3], mac[4], mac[5]);
}
--
Amos.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
2013-05-23 9:08 ` [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information Amos Kong
2013-05-23 10:23 ` Michael S. Tsirkin
@ 2013-05-23 12:14 ` Eric Blake
2013-05-24 3:03 ` Amos Kong
2013-06-26 7:14 ` Markus Armbruster
2013-05-23 16:03 ` Luiz Capitulino
2013-06-26 9:54 ` Markus Armbruster
3 siblings, 2 replies; 47+ messages in thread
From: Eric Blake @ 2013-05-23 12:14 UTC (permalink / raw)
To: Amos Kong; +Cc: mst, qemu-devel, stefanha, lcapitulino
[-- Attachment #1: Type: text/plain, Size: 4673 bytes --]
On 05/23/2013 03:08 AM, Amos Kong wrote:
> 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
s/don't/doesn't/
> rx-filter after receives one event, new events won't be sent
s/after/after it/
> to qmp monitor.
>
> +++ 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) {
Do you need the has_name argument here, or can you ensure that the
caller passes NULL when the caller's has_name was false, for one less
parameter and the same amount of information?
> +++ 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' ] }
I think s/no/none/ would make slightly more sense (usually, you pair
no/yes and none/all, not 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
s/to ether//; s/$/is enabled/
> +#
> +# @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
Naming is reasonable; thanks for improving things from v1.
> +#
> +# 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
> +#
> +# Since: 1.6
> +##
> +{ 'command': 'query-rx-filter', 'data': { '*name': 'str' },
> + 'returns': ['RxFilterInfo'] }
Interface looks reasonable. I didn't check the code closely, but agree
with Michael's assessment that the event-suppression flag has to be
per-device, not global.
> +
> +Each array entry contains the following:
> +
> +- "name": net client name (jaso-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)
s/is overflow/overflowed/
> +- "unicast-overflow": unicast table is overflow (json-bool)
and again
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
2013-05-23 12:14 ` Eric Blake
@ 2013-05-24 3:03 ` Amos Kong
2013-06-26 7:14 ` Markus Armbruster
1 sibling, 0 replies; 47+ messages in thread
From: Amos Kong @ 2013-05-24 3:03 UTC (permalink / raw)
To: Eric Blake; +Cc: mst, qemu-devel, stefanha, lcapitulino
On Thu, May 23, 2013 at 06:14:19AM -0600, Eric Blake wrote:
> On 05/23/2013 03:08 AM, Amos Kong wrote:
> > +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) {
>
> Do you need the has_name argument here, or can you ensure that the
> caller passes NULL when the caller's has_name was false,
hmp_info_rx_filter() passes NULL name when has_name is false.
'has_name' is need here. Or we can change it to:
if (name && strcmp(nc->name, name) != 0) {
I think using 'has_name' is clearer.
> for one less parameter and the same amount of information?
qmp_query_rx_filter() define is generated by QAPI infrastructure,
the parameters are fixed. We also use it in hmp_info_rx_filter().
--
Amos.
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
2013-05-23 12:14 ` Eric Blake
2013-05-24 3:03 ` Amos Kong
@ 2013-06-26 7:14 ` Markus Armbruster
1 sibling, 0 replies; 47+ messages in thread
From: Markus Armbruster @ 2013-06-26 7:14 UTC (permalink / raw)
To: Eric Blake; +Cc: lcapitulino, Amos Kong, qemu-devel, stefanha, mst
Eric Blake <eblake@redhat.com> writes:
> On 05/23/2013 03:08 AM, Amos Kong wrote:
>> 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
>
> s/don't/doesn't/
>
>> rx-filter after receives one event, new events won't be sent
>
> s/after/after it/
>
>> to qmp monitor.
>>
>> +++ 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) {
>
> Do you need the has_name argument here, or can you ensure that the
> caller passes NULL when the caller's has_name was false, for one less
> parameter and the same amount of information?
QAPI always generates a has_FOO parameter for an optional FOO parameter,
even when FOO is a pointer, which has the perfectly obvious special "not
given" value NULL. I hate that.
[...]
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
2013-05-23 9:08 ` [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information Amos Kong
2013-05-23 10:23 ` Michael S. Tsirkin
2013-05-23 12:14 ` Eric Blake
@ 2013-05-23 16:03 ` Luiz Capitulino
2013-05-24 3:08 ` Amos Kong
2013-06-26 9:54 ` Markus Armbruster
3 siblings, 1 reply; 47+ messages in thread
From: Luiz Capitulino @ 2013-05-23 16:03 UTC (permalink / raw)
To: Amos Kong; +Cc: qemu-devel, stefanha, mst
On Thu, 23 May 2013 17:08:00 +0800
Amos Kong <akong@redhat.com> wrote:
> 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 <akong@redhat.com>
Looks good in general. I have some comments below and restarted the
dicussion about notify_enabled.
> ---
> 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
> +@item info rx-filter [net client name]
> +show the rx-filter information for all nics (or for the given nic)
> @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);
qmp_query_rx_filter() can fail.
> + entry = filter_list;
> +
> + while (entry) {
> + 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;
> + }
Can't that loop be moved to a function?
> +
> + 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;
> + }
> +
> + 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]);
> +
> + 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;
> +
> + 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;
> +
> + 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;
> }
>
> @@ -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",
> + .args_type = "name:s?",
> + .params = "[net client name]",
> + .help = "show the rx-filter information for all nics (or"
> + " for the given 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;
> + }
I don't think we need this argument. This command is quite simple in its
response, let's do this filtering in HMP only.
> +
> + 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");
> + }
> + }
> +
> + 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
> +#
> +# Since: 1.6
> +##
> +{ 'command': 'query-rx-filter', 'data': { '*name': 'str' },
> + '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.
> +
> +Each array entry contains the following:
> +
> +- "name": net client name (jaso-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)
> +- "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
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
2013-05-23 16:03 ` Luiz Capitulino
@ 2013-05-24 3:08 ` Amos Kong
2013-05-24 12:23 ` Luiz Capitulino
0 siblings, 1 reply; 47+ messages in thread
From: Amos Kong @ 2013-05-24 3:08 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: qemu-devel, stefanha, mst
On Thu, May 23, 2013 at 12:03:25PM -0400, Luiz Capitulino wrote:
> On Thu, 23 May 2013 17:08:00 +0800
> Amos Kong <akong@redhat.com> wrote:
> > 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.
> > +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;
> > + }
>
> I don't think we need this argument. This command is quite simple in its
> response, let's do this filtering in HMP only.
Event message contains the net client name, management might only want
to query the single net client.
And we plan to use a flag for _each nic_ to control the event notification,
querying single net client will only clear it's flag.
So we need to support query by net-client-name.
--
Amos.
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
2013-05-24 3:08 ` Amos Kong
@ 2013-05-24 12:23 ` Luiz Capitulino
2013-05-24 12:55 ` Eric Blake
0 siblings, 1 reply; 47+ messages in thread
From: Luiz Capitulino @ 2013-05-24 12:23 UTC (permalink / raw)
To: Amos Kong; +Cc: qemu-devel, stefanha, mst
On Fri, 24 May 2013 11:08:13 +0800
Amos Kong <akong@redhat.com> wrote:
> On Thu, May 23, 2013 at 12:03:25PM -0400, Luiz Capitulino wrote:
> > On Thu, 23 May 2013 17:08:00 +0800
> > Amos Kong <akong@redhat.com> wrote:
>
> > > 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.
>
>
> > > +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;
> > > + }
> >
> > I don't think we need this argument. This command is quite simple in its
> > response, let's do this filtering in HMP only.
>
> Event message contains the net client name, management might only want
> to query the single net client.
The client can do the filtering itself.
>
> And we plan to use a flag for _each nic_ to control the event notification,
> querying single net client will only clear it's flag.
>
> So we need to support query by net-client-name.
>
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
2013-05-24 12:23 ` Luiz Capitulino
@ 2013-05-24 12:55 ` Eric Blake
2013-05-24 13:08 ` Luiz Capitulino
2013-05-24 15:20 ` Markus Armbruster
0 siblings, 2 replies; 47+ messages in thread
From: Eric Blake @ 2013-05-24 12:55 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: Amos Kong, qemu-devel, stefanha, mst
[-- Attachment #1: Type: text/plain, Size: 1876 bytes --]
On 05/24/2013 06:23 AM, Luiz Capitulino wrote:
>>> I don't think we need this argument. This command is quite simple in its
>>> response, let's do this filtering in HMP only.
>>
>> Event message contains the net client name, management might only want
>> to query the single net client.
>
> The client can do the filtering itself.
If we're arguing that we want this to be as responsive as possible, then
the less data we send over the wire, the faster management can react to
the guest's request for a particular NIC. That is, if libvirt is
listening to events that says NIC2 wants to change rx-filter, libvirt
would rather do a filtered query where it knows the JSON array of 1
element matches NIC2 data, rather than do a global query and search
through the returned array until it finds NIC2.
Filtering is relatively easy to add, whether you do it in QMP or make
every client add it. Libvirt will survive if you don't have filtering,
but I don't see why we can't have it in QMP. Also, if you DO decide to
rip filtering out of QMP, you STILL need to keep a per-NIC flag. Since
the events say which NIC is requesting a change, even if the query reads
all nics, libvirt will only change the macvtap settings of the nic(s)
for which it has received an event (it doesn't make sense to waste time
requesting a (no-op) change to macvtap settings on a nic that hasn't
requested a change). But if you argue that having no filtering in the
QMP command means that you can get away with a single flag instead of a
per-nic flag, then you will fail to emit an event for NIC2 if it changes
in between the time that NIC1 fired an event and libvirt finally does
the query, and libvirt wouldn't realize that NIC2 also needs a macvtap
change.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
2013-05-24 12:55 ` Eric Blake
@ 2013-05-24 13:08 ` Luiz Capitulino
2013-05-24 13:23 ` Eric Blake
2013-05-24 15:20 ` Markus Armbruster
1 sibling, 1 reply; 47+ messages in thread
From: Luiz Capitulino @ 2013-05-24 13:08 UTC (permalink / raw)
To: Eric Blake; +Cc: Amos Kong, qemu-devel, stefanha, mst
On Fri, 24 May 2013 06:55:44 -0600
Eric Blake <eblake@redhat.com> wrote:
> On 05/24/2013 06:23 AM, Luiz Capitulino wrote:
> >>> I don't think we need this argument. This command is quite simple in its
> >>> response, let's do this filtering in HMP only.
> >>
> >> Event message contains the net client name, management might only want
> >> to query the single net client.
> >
> > The client can do the filtering itself.
>
> If we're arguing that we want this to be as responsive as possible, then
> the less data we send over the wire, the faster management can react to
> the guest's request for a particular NIC. That is, if libvirt is
> listening to events that says NIC2 wants to change rx-filter, libvirt
> would rather do a filtered query where it knows the JSON array of 1
> element matches NIC2 data, rather than do a global query and search
> through the returned array until it finds NIC2.
This sounds like premature optimization to me, but I wonder if instead
of cluttering commands with arguments to do the filtering we could add
some standard way of doing this in the QAPI.
It was you who suggested a filter command?
> Filtering is relatively easy to add, whether you do it in QMP or make
> every client add it. Libvirt will survive if you don't have filtering,
> but I don't see why we can't have it in QMP. Also, if you DO decide to
> rip filtering out of QMP, you STILL need to keep a per-NIC flag. Since
> the events say which NIC is requesting a change, even if the query reads
> all nics, libvirt will only change the macvtap settings of the nic(s)
> for which it has received an event (it doesn't make sense to waste time
> requesting a (no-op) change to macvtap settings on a nic that hasn't
> requested a change). But if you argue that having no filtering in the
> QMP command means that you can get away with a single flag instead of a
> per-nic flag, then you will fail to emit an event for NIC2 if it changes
> in between the time that NIC1 fired an event and libvirt finally does
> the query, and libvirt wouldn't realize that NIC2 also needs a macvtap
> change.
>
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
2013-05-24 13:08 ` Luiz Capitulino
@ 2013-05-24 13:23 ` Eric Blake
2013-06-26 9:35 ` Markus Armbruster
0 siblings, 1 reply; 47+ messages in thread
From: Eric Blake @ 2013-05-24 13:23 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: Amos Kong, qemu-devel, stefanha, mst
[-- Attachment #1: Type: text/plain, Size: 900 bytes --]
On 05/24/2013 07:08 AM, Luiz Capitulino wrote:
> This sounds like premature optimization to me, but I wonder if instead
> of cluttering commands with arguments to do the filtering we could add
> some standard way of doing this in the QAPI.
Maybe we could make QAPI support generic filtering for all query-*
commands. But there's still a cost with making all query-* commands
malloc the space for an entire list only to then have a QAPI layer on
top of it do filtering before handing back the single element over the
wire, compared to passing the filtering down to the query-*
implementation to do the filtering in place.
>
> It was you who suggested a filter command?
No, Stefan suggested it on v1:
https://lists.gnu.org/archive/html/qemu-devel/2013-04/msg03102.html
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
2013-05-24 13:23 ` Eric Blake
@ 2013-06-26 9:35 ` Markus Armbruster
0 siblings, 0 replies; 47+ messages in thread
From: Markus Armbruster @ 2013-06-26 9:35 UTC (permalink / raw)
To: Eric Blake; +Cc: Amos Kong, mst, qemu-devel, stefanha, Luiz Capitulino
Eric Blake <eblake@redhat.com> writes:
> On 05/24/2013 07:08 AM, Luiz Capitulino wrote:
>> This sounds like premature optimization to me, but I wonder if instead
>> of cluttering commands with arguments to do the filtering we could add
>> some standard way of doing this in the QAPI.
>
> Maybe we could make QAPI support generic filtering for all query-*
> commands. But there's still a cost with making all query-* commands
> malloc the space for an entire list only to then have a QAPI layer on
> top of it do filtering before handing back the single element over the
> wire, compared to passing the filtering down to the query-*
> implementation to do the filtering in place.
I'd expect the time spent on malloc to be dwarved several times over by
I/O latency.
If we worry about malloc impacting our latency, then we should not be
using QAPI! It's really, really malloc-happy.
In QMP, we generally don't.
>> It was you who suggested a filter command?
>
> No, Stefan suggested it on v1:
> https://lists.gnu.org/archive/html/qemu-devel/2013-04/msg03102.html
So far, I'm with Luiz here.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
2013-05-24 12:55 ` Eric Blake
2013-05-24 13:08 ` Luiz Capitulino
@ 2013-05-24 15:20 ` Markus Armbruster
2013-05-24 16:12 ` Michael S. Tsirkin
1 sibling, 1 reply; 47+ messages in thread
From: Markus Armbruster @ 2013-05-24 15:20 UTC (permalink / raw)
To: Eric Blake; +Cc: Amos Kong, mst, qemu-devel, stefanha, Luiz Capitulino
Eric Blake <eblake@redhat.com> writes:
> On 05/24/2013 06:23 AM, Luiz Capitulino wrote:
>>>> I don't think we need this argument. This command is quite simple in its
>>>> response, let's do this filtering in HMP only.
>>>
>>> Event message contains the net client name, management might only want
>>> to query the single net client.
>>
>> The client can do the filtering itself.
>
> If we're arguing that we want this to be as responsive as possible, then
> the less data we send over the wire, the faster management can react to
> the guest's request for a particular NIC. That is, if libvirt is
> listening to events that says NIC2 wants to change rx-filter, libvirt
> would rather do a filtered query where it knows the JSON array of 1
> element matches NIC2 data, rather than do a global query and search
> through the returned array until it finds NIC2.
>
> Filtering is relatively easy to add, whether you do it in QMP or make
> every client add it. Libvirt will survive if you don't have filtering,
> but I don't see why we can't have it in QMP. Also, if you DO decide to
> rip filtering out of QMP, you STILL need to keep a per-NIC flag. Since
> the events say which NIC is requesting a change, even if the query reads
> all nics, libvirt will only change the macvtap settings of the nic(s)
> for which it has received an event (it doesn't make sense to waste time
> requesting a (no-op) change to macvtap settings on a nic that hasn't
> requested a change). But if you argue that having no filtering in the
> QMP command means that you can get away with a single flag instead of a
> per-nic flag, then you will fail to emit an event for NIC2 if it changes
> in between the time that NIC1 fired an event and libvirt finally does
> the query, and libvirt wouldn't realize that NIC2 also needs a macvtap
> change.
No disagreement on the need for a per-NIC flag.
I'm not sure I buy the responsiveness argument. Sure, the fastest I/O
is no I/O, but whether you read and parse 100 bytes or 1000 from a Unix
domain socket once in a great while shouldn't make a difference.
My main concern is to keep the external interface simple. I'm rather
reluctant to have query commands grow options.
In a case where we need the "give me everything" query anyway, the "give
me this particular part" option is additional complexity. Needs
justification, say arguments involving throughput, latency or client
complexity.
Perhaps cases exist where we never want to ask for everything. Then the
"give me everything" query is useless, and the option should be
mandatory.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
2013-05-24 15:20 ` Markus Armbruster
@ 2013-05-24 16:12 ` Michael S. Tsirkin
2013-05-24 18:05 ` Eric Blake
0 siblings, 1 reply; 47+ messages in thread
From: Michael S. Tsirkin @ 2013-05-24 16:12 UTC (permalink / raw)
To: Markus Armbruster
Cc: libvir-list, qemu-devel, Luiz Capitulino, stefanha, Amos Kong, laine
On Fri, May 24, 2013 at 05:20:13PM +0200, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
> > On 05/24/2013 06:23 AM, Luiz Capitulino wrote:
> >>>> I don't think we need this argument. This command is quite simple in its
> >>>> response, let's do this filtering in HMP only.
> >>>
> >>> Event message contains the net client name, management might only want
> >>> to query the single net client.
> >>
> >> The client can do the filtering itself.
> >
> > If we're arguing that we want this to be as responsive as possible, then
> > the less data we send over the wire, the faster management can react to
> > the guest's request for a particular NIC. That is, if libvirt is
> > listening to events that says NIC2 wants to change rx-filter, libvirt
> > would rather do a filtered query where it knows the JSON array of 1
> > element matches NIC2 data, rather than do a global query and search
> > through the returned array until it finds NIC2.
> >
> > Filtering is relatively easy to add, whether you do it in QMP or make
> > every client add it. Libvirt will survive if you don't have filtering,
> > but I don't see why we can't have it in QMP. Also, if you DO decide to
> > rip filtering out of QMP, you STILL need to keep a per-NIC flag. Since
> > the events say which NIC is requesting a change, even if the query reads
> > all nics, libvirt will only change the macvtap settings of the nic(s)
> > for which it has received an event (it doesn't make sense to waste time
> > requesting a (no-op) change to macvtap settings on a nic that hasn't
> > requested a change). But if you argue that having no filtering in the
> > QMP command means that you can get away with a single flag instead of a
> > per-nic flag, then you will fail to emit an event for NIC2 if it changes
> > in between the time that NIC1 fired an event and libvirt finally does
> > the query, and libvirt wouldn't realize that NIC2 also needs a macvtap
> > change.
>
> No disagreement on the need for a per-NIC flag.
>
> I'm not sure I buy the responsiveness argument. Sure, the fastest I/O
> is no I/O, but whether you read and parse 100 bytes or 1000 from a Unix
> domain socket once in a great while shouldn't make a difference.
>
> My main concern is to keep the external interface simple. I'm rather
> reluctant to have query commands grow options.
>
> In a case where we need the "give me everything" query anyway, the "give
> me this particular part" option is additional complexity. Needs
> justification, say arguments involving throughput, latency or client
> complexity.
>
> Perhaps cases exist where we never want to ask for everything. Then the
> "give me everything" query is useless, and the option should be
> mandatory.
We need the query for macvtap devices. We don't need it
for tap devices. In that case you don't want tap device info.
Maybe some libvirt guys can tell us whether they prefer
a per device query or a global one with info for all NICs?
I think for HMP it's best to have nic optional.
Is it a good idea to make QMP match HMP closely?
--
MST
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
2013-05-24 16:12 ` Michael S. Tsirkin
@ 2013-05-24 18:05 ` Eric Blake
2013-05-24 20:00 ` Luiz Capitulino
0 siblings, 1 reply; 47+ messages in thread
From: Eric Blake @ 2013-05-24 18:05 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: libvir-list, qemu-devel, Markus Armbruster, stefanha,
Luiz Capitulino, Amos Kong, laine
[-- Attachment #1: Type: text/plain, Size: 4470 bytes --]
On 05/24/2013 10:12 AM, Michael S. Tsirkin wrote:
>>>>>
>>>>> Event message contains the net client name, management might only want
>>>>> to query the single net client.
>>>>
>>>> The client can do the filtering itself.
>>>
>> I'm not sure I buy the responsiveness argument. Sure, the fastest I/O
>> is no I/O, but whether you read and parse 100 bytes or 1000 from a Unix
>> domain socket once in a great while shouldn't make a difference.
And the time spent malloc'ing the larger message to send from qemu, as
well as the time spent malloc'ing the libvirt side that parses the qemu
string into C code for use, and the time spent strcmp'ing every entry to
find the right one...
It really IS more efficient to filter as low down in the stack as
possible, once it is determined that filtering is desirable.
Whether filtering makes a difference in performance is a different
question - you may be right that always returning the entire list and
making libvirt do its own filtering will still not add any more
noticeable delay compared to libvirt doing a filtered query, if the
bottleneck lies elsewhere (such as libvirt telling macvtap its new
configration).
>>
>> My main concern is to keep the external interface simple. I'm rather
>> reluctant to have query commands grow options.
>>
>> In a case where we need the "give me everything" query anyway, the "give
>> me this particular part" option is additional complexity. Needs
>> justification, say arguments involving throughput, latency or client
>> complexity.
>>
>> Perhaps cases exist where we never want to ask for everything. Then the
>> "give me everything" query is useless, and the option should be
>> mandatory.
For this _particular_ interface, I'm not sure whether libvirt will ever
use an unfiltered query - that is, the rx-filter query will probably
always be invoked in response to an event, at which point libvirt only
cares about the filter status of the nic named in the event. And
ultimately libvirt knows what nics it passed to the guest, so even if
there isn't a global query and I guessed wrong about libvirt never
wanting all state at once, it would still be possible for libvirt to
iterate over one query per nic. On the other hand, consistency with
other query-* QMP commands says that most of them return as much
information as possible all the time, and generally libvirt likes this -
even the newly-added query-command-line-options has a filtering option,
but current libvirt.git only uses it once in global mode rather than
once-per-option in filtered mode.
>
> We need the query for macvtap devices. We don't need it
> for tap devices. In that case you don't want tap device info.
>
> Maybe some libvirt guys can tell us whether they prefer
> a per device query or a global one with info for all NICs?
Libvirt can cope either way. I personally like the idea of allowing
both global and filtered queries, without second-guessing what
management apps will prefer to use, and don't think filtering adds that
much complexity. But if you want to insist on avoiding filtering, I'd
rather have a global query than a mandatory name argument, for
consistency with other query-* commands, even if libvirt then ends up
doing its own filtering.
If we get introspection into qemu 1.6 at the same time as the new query
for rx-filters, it really won't matter whether you start with
global-only or mandatory name-only; either way, if we change our mind
and add the other mode in qemu 1.7, libvirt will still be able to use
introspection to determine whether the argument is present in one
direction (going from global-only to optional filtering), or whether the
argument has been made optionl in the other direction (going from
mandatory name to optional global).
> I think for HMP it's best to have nic optional.
This is true, no matter what we decide for QMP.
> Is it a good idea to make QMP match HMP closely?
QMP has to provide enough information for HMP to do its job. How will
HMP do global listing if QMP doesn't provide a way to get all the
devices at once? Remember, libvirt knows what devices it told qemu to
create, but I don't know that HMP has the same visibility into the list
of possible devices that can be queried. So you may need a global mode
to begin with.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
2013-05-24 18:05 ` Eric Blake
@ 2013-05-24 20:00 ` Luiz Capitulino
2013-05-26 7:38 ` Michael S. Tsirkin
0 siblings, 1 reply; 47+ messages in thread
From: Luiz Capitulino @ 2013-05-24 20:00 UTC (permalink / raw)
To: Eric Blake
Cc: Michael S. Tsirkin, libvir-list, qemu-devel, Markus Armbruster,
stefanha, Amos Kong, laine
On Fri, 24 May 2013 12:05:12 -0600
Eric Blake <eblake@redhat.com> wrote:
> On 05/24/2013 10:12 AM, Michael S. Tsirkin wrote:
> >>>>>
> >>>>> Event message contains the net client name, management might only want
> >>>>> to query the single net client.
> >>>>
> >>>> The client can do the filtering itself.
> >>>
>
> >> I'm not sure I buy the responsiveness argument. Sure, the fastest I/O
> >> is no I/O, but whether you read and parse 100 bytes or 1000 from a Unix
> >> domain socket once in a great while shouldn't make a difference.
>
> And the time spent malloc'ing the larger message to send from qemu, as
> well as the time spent malloc'ing the libvirt side that parses the qemu
> string into C code for use, and the time spent strcmp'ing every entry to
> find the right one...
>
> It really IS more efficient to filter as low down in the stack as
> possible, once it is determined that filtering is desirable.
>
> Whether filtering makes a difference in performance is a different
> question - you may be right that always returning the entire list and
> making libvirt do its own filtering will still not add any more
> noticeable delay compared to libvirt doing a filtered query, if the
> bottleneck lies elsewhere (such as libvirt telling macvtap its new
> configration).
>
> >>
> >> My main concern is to keep the external interface simple. I'm rather
> >> reluctant to have query commands grow options.
> >>
> >> In a case where we need the "give me everything" query anyway, the "give
> >> me this particular part" option is additional complexity. Needs
> >> justification, say arguments involving throughput, latency or client
> >> complexity.
> >>
> >> Perhaps cases exist where we never want to ask for everything. Then the
> >> "give me everything" query is useless, and the option should be
> >> mandatory.
>
> For this _particular_ interface, I'm not sure whether libvirt will ever
> use an unfiltered query -
If having the argument is useful for libvirt, then it's fine to have it.
But I'd be very reluctant to buy any performance argument w/o real
numbers to back them up.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
2013-05-24 20:00 ` Luiz Capitulino
@ 2013-05-26 7:38 ` Michael S. Tsirkin
2013-05-27 8:36 ` Stefan Hajnoczi
0 siblings, 1 reply; 47+ messages in thread
From: Michael S. Tsirkin @ 2013-05-26 7:38 UTC (permalink / raw)
To: Luiz Capitulino
Cc: libvir-list, Markus Armbruster, qemu-devel, stefanha, Amos Kong, laine
On Fri, May 24, 2013 at 04:00:42PM -0400, Luiz Capitulino wrote:
> On Fri, 24 May 2013 12:05:12 -0600
> Eric Blake <eblake@redhat.com> wrote:
>
> > On 05/24/2013 10:12 AM, Michael S. Tsirkin wrote:
> > >>>>>
> > >>>>> Event message contains the net client name, management might only want
> > >>>>> to query the single net client.
> > >>>>
> > >>>> The client can do the filtering itself.
> > >>>
> >
> > >> I'm not sure I buy the responsiveness argument. Sure, the fastest I/O
> > >> is no I/O, but whether you read and parse 100 bytes or 1000 from a Unix
> > >> domain socket once in a great while shouldn't make a difference.
> >
> > And the time spent malloc'ing the larger message to send from qemu, as
> > well as the time spent malloc'ing the libvirt side that parses the qemu
> > string into C code for use, and the time spent strcmp'ing every entry to
> > find the right one...
> >
> > It really IS more efficient to filter as low down in the stack as
> > possible, once it is determined that filtering is desirable.
> >
> > Whether filtering makes a difference in performance is a different
> > question - you may be right that always returning the entire list and
> > making libvirt do its own filtering will still not add any more
> > noticeable delay compared to libvirt doing a filtered query, if the
> > bottleneck lies elsewhere (such as libvirt telling macvtap its new
> > configration).
> >
> > >>
> > >> My main concern is to keep the external interface simple. I'm rather
> > >> reluctant to have query commands grow options.
> > >>
> > >> In a case where we need the "give me everything" query anyway, the "give
> > >> me this particular part" option is additional complexity. Needs
> > >> justification, say arguments involving throughput, latency or client
> > >> complexity.
> > >>
> > >> Perhaps cases exist where we never want to ask for everything. Then the
> > >> "give me everything" query is useless, and the option should be
> > >> mandatory.
> >
> > For this _particular_ interface, I'm not sure whether libvirt will ever
> > use an unfiltered query -
>
> If having the argument is useful for libvirt, then it's fine to have it.
>
> But I'd be very reluctant to buy any performance argument w/o real
> numbers to back them up.
Me too. I think it's more convenience than performance.
--
MST
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
2013-05-26 7:38 ` Michael S. Tsirkin
@ 2013-05-27 8:36 ` Stefan Hajnoczi
0 siblings, 0 replies; 47+ messages in thread
From: Stefan Hajnoczi @ 2013-05-27 8:36 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: libvir-list, qemu-devel, Markus Armbruster, Luiz Capitulino,
Amos Kong, laine
On Sun, May 26, 2013 at 10:38:14AM +0300, Michael S. Tsirkin wrote:
> On Fri, May 24, 2013 at 04:00:42PM -0400, Luiz Capitulino wrote:
> > On Fri, 24 May 2013 12:05:12 -0600
> > Eric Blake <eblake@redhat.com> wrote:
> >
> > > On 05/24/2013 10:12 AM, Michael S. Tsirkin wrote:
> > > >>>>>
> > > >>>>> Event message contains the net client name, management might only want
> > > >>>>> to query the single net client.
> > > >>>>
> > > >>>> The client can do the filtering itself.
> > > >>>
> > >
> > > >> I'm not sure I buy the responsiveness argument. Sure, the fastest I/O
> > > >> is no I/O, but whether you read and parse 100 bytes or 1000 from a Unix
> > > >> domain socket once in a great while shouldn't make a difference.
> > >
> > > And the time spent malloc'ing the larger message to send from qemu, as
> > > well as the time spent malloc'ing the libvirt side that parses the qemu
> > > string into C code for use, and the time spent strcmp'ing every entry to
> > > find the right one...
> > >
> > > It really IS more efficient to filter as low down in the stack as
> > > possible, once it is determined that filtering is desirable.
> > >
> > > Whether filtering makes a difference in performance is a different
> > > question - you may be right that always returning the entire list and
> > > making libvirt do its own filtering will still not add any more
> > > noticeable delay compared to libvirt doing a filtered query, if the
> > > bottleneck lies elsewhere (such as libvirt telling macvtap its new
> > > configration).
> > >
> > > >>
> > > >> My main concern is to keep the external interface simple. I'm rather
> > > >> reluctant to have query commands grow options.
> > > >>
> > > >> In a case where we need the "give me everything" query anyway, the "give
> > > >> me this particular part" option is additional complexity. Needs
> > > >> justification, say arguments involving throughput, latency or client
> > > >> complexity.
> > > >>
> > > >> Perhaps cases exist where we never want to ask for everything. Then the
> > > >> "give me everything" query is useless, and the option should be
> > > >> mandatory.
> > >
> > > For this _particular_ interface, I'm not sure whether libvirt will ever
> > > use an unfiltered query -
> >
> > If having the argument is useful for libvirt, then it's fine to have it.
> >
> > But I'd be very reluctant to buy any performance argument w/o real
> > numbers to back them up.
>
> Me too. I think it's more convenience than performance.
Agreed. I suggested filtering on a NIC for usability rather than
performance reasons.
QMP should be easy to use. Requiring every client to fish for the right
NIC in a bunch of output that gets discarded is not convenient.
Stefan
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
2013-05-23 9:08 ` [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information Amos Kong
` (2 preceding siblings ...)
2013-05-23 16:03 ` Luiz Capitulino
@ 2013-06-26 9:54 ` Markus Armbruster
2013-06-26 12:00 ` Markus Armbruster
2013-07-02 6:33 ` Amos Kong
3 siblings, 2 replies; 47+ messages in thread
From: Markus Armbruster @ 2013-06-26 9:54 UTC (permalink / raw)
To: Amos Kong; +Cc: lcapitulino, qemu-devel, stefanha, mst
Amos Kong <akong@redhat.com> 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 <akong@redhat.com>
> ---
> 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
> +@item 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?
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
2013-06-26 9:54 ` Markus Armbruster
@ 2013-06-26 12:00 ` Markus Armbruster
2013-06-26 14:02 ` Luiz Capitulino
2013-07-02 6:33 ` Amos Kong
1 sibling, 1 reply; 47+ messages in thread
From: Markus Armbruster @ 2013-06-26 12:00 UTC (permalink / raw)
To: Amos Kong; +Cc: lcapitulino, qemu-devel, stefanha, mst
Meh, I got confused and reviewed an out-of-date version. Hope it's not
entirely noise.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
2013-06-26 12:00 ` Markus Armbruster
@ 2013-06-26 14:02 ` Luiz Capitulino
2013-06-26 14:15 ` Markus Armbruster
0 siblings, 1 reply; 47+ messages in thread
From: Luiz Capitulino @ 2013-06-26 14:02 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Amos Kong, qemu-devel, stefanha, mst
On Wed, 26 Jun 2013 14:00:30 +0200
Markus Armbruster <armbru@redhat.com> wrote:
> Meh, I got confused and reviewed an out-of-date version. Hope it's not
> entirely noise.
I don't think it is. But this series got applied to Michael's tree, so
if you want your comments addressed before applying to master (I think
we do) then it's better to state it clearly.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
2013-06-26 14:02 ` Luiz Capitulino
@ 2013-06-26 14:15 ` Markus Armbruster
2013-06-28 14:04 ` Eric Blake
0 siblings, 1 reply; 47+ messages in thread
From: Markus Armbruster @ 2013-06-26 14:15 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: Amos Kong, qemu-devel, stefanha, mst
Luiz Capitulino <lcapitulino@redhat.com> writes:
> On Wed, 26 Jun 2013 14:00:30 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Meh, I got confused and reviewed an out-of-date version. Hope it's not
>> entirely noise.
>
> I don't think it is. But this series got applied to Michael's tree, so
> if you want your comments addressed before applying to master (I think
> we do) then it's better to state it clearly.
Michael, please give Amos a chance to reply to my review.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
2013-06-26 14:15 ` Markus Armbruster
@ 2013-06-28 14:04 ` Eric Blake
2013-06-28 17:27 ` Markus Armbruster
0 siblings, 1 reply; 47+ messages in thread
From: Eric Blake @ 2013-06-28 14:04 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Amos Kong, mst, qemu-devel, stefanha, Luiz Capitulino
[-- Attachment #1: Type: text/plain, Size: 874 bytes --]
On 06/26/2013 08:15 AM, Markus Armbruster wrote:
> Luiz Capitulino <lcapitulino@redhat.com> writes:
>
>> On Wed, 26 Jun 2013 14:00:30 +0200
>> Markus Armbruster <armbru@redhat.com> wrote:
>>
>>> Meh, I got confused and reviewed an out-of-date version. Hope it's not
>>> entirely noise.
>>
>> I don't think it is. But this series got applied to Michael's tree, so
>> if you want your comments addressed before applying to master (I think
>> we do) then it's better to state it clearly.
>
> Michael, please give Amos a chance to reply to my review.
Looks like the pull request is already live but had issues; meanwhile, I
also found an issue when reviewing the pull request:
https://lists.gnu.org/archive/html/qemu-devel/2013-06/msg05127.html
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
2013-06-28 14:04 ` Eric Blake
@ 2013-06-28 17:27 ` Markus Armbruster
2013-07-01 3:24 ` Amos Kong
0 siblings, 1 reply; 47+ messages in thread
From: Markus Armbruster @ 2013-06-28 17:27 UTC (permalink / raw)
To: Eric Blake; +Cc: Amos Kong, mst, qemu-devel, stefanha, Luiz Capitulino
Eric Blake <eblake@redhat.com> writes:
> On 06/26/2013 08:15 AM, Markus Armbruster wrote:
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>>
>>> On Wed, 26 Jun 2013 14:00:30 +0200
>>> Markus Armbruster <armbru@redhat.com> wrote:
>>>
>>>> Meh, I got confused and reviewed an out-of-date version. Hope it's not
>>>> entirely noise.
>>>
>>> I don't think it is. But this series got applied to Michael's tree, so
>>> if you want your comments addressed before applying to master (I think
>>> we do) then it's better to state it clearly.
>>
>> Michael, please give Amos a chance to reply to my review.
>
> Looks like the pull request is already live but had issues; meanwhile, I
> also found an issue when reviewing the pull request:
> https://lists.gnu.org/archive/html/qemu-devel/2013-06/msg05127.html
Missed it, thanks!
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
2013-06-28 17:27 ` Markus Armbruster
@ 2013-07-01 3:24 ` Amos Kong
0 siblings, 0 replies; 47+ messages in thread
From: Amos Kong @ 2013-07-01 3:24 UTC (permalink / raw)
To: Markus Armbruster; +Cc: mst, qemu-devel, stefanha, Luiz Capitulino
On Fri, Jun 28, 2013 at 07:27:21PM +0200, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
> > On 06/26/2013 08:15 AM, Markus Armbruster wrote:
> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >>
> >>> On Wed, 26 Jun 2013 14:00:30 +0200
> >>> Markus Armbruster <armbru@redhat.com> wrote:
> >>>
> >>>> Meh, I got confused and reviewed an out-of-date version. Hope it's not
> >>>> entirely noise.
> >>>
> >>> I don't think it is. But this series got applied to Michael's tree, so
> >>> if you want your comments addressed before applying to master (I think
> >>> we do) then it's better to state it clearly.
> >>
> >> Michael, please give Amos a chance to reply to my review.
> >
> > Looks like the pull request is already live but had issues; meanwhile, I
> > also found an issue when reviewing the pull request:
> > https://lists.gnu.org/archive/html/qemu-devel/2013-06/msg05127.html
>
> Missed it, thanks!
In mst's v2 pull, it still contains my patch, it's not merged to
master by Anthony. I think mst should re-send a v3 without my patch
as said in changelog.
I will fix the problem in your reply, thanks.
--
Amos.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
2013-06-26 9:54 ` Markus Armbruster
2013-06-26 12:00 ` Markus Armbruster
@ 2013-07-02 6:33 ` Amos Kong
2013-07-02 9:05 ` Markus Armbruster
1 sibling, 1 reply; 47+ messages in thread
From: Amos Kong @ 2013-07-02 6:33 UTC (permalink / raw)
To: Markus Armbruster; +Cc: lcapitulino, qemu-devel, stefanha, mst
On Wed, Jun 26, 2013 at 11:54:20AM +0200, Markus Armbruster wrote:
> Amos Kong <akong@redhat.com> 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 <akong@redhat.com>
Thanks for your comments, some comments are out-of-data, I removed
them in this reply.
> > +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.
Good suggestion, added to my todolist.
> > +
> > + 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.
> > + 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?
We should notify in error path if n->mac_table is changed.
> > @@ -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/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 an error note is bettr than a NULL list. Management should not query
the rx-filter info if there is no net client supports it.
{
"return": [
]
}
NULL list might be misread to that it supports rx-filter querying, but the
rx-filter config has no item.
> > + }
> > + }
> > +
> > + 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.
Reasonable.
> > +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?
We will not.
It's ugly to check if nic is virtio-net nic in net/net.c, so I
register the query function to NetClientInfo. Traversal the net
client list in net/net.c, and execute query of each virtio-net
instance in virtio-net.c
> 2. If the former, are you reasonably sure this object will do for other
> NICs?
No.
--
Amos.
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
2013-07-02 6:33 ` Amos Kong
@ 2013-07-02 9:05 ` Markus Armbruster
2013-07-02 10:40 ` Amos Kong
0 siblings, 1 reply; 47+ messages in thread
From: Markus Armbruster @ 2013-07-02 9:05 UTC (permalink / raw)
To: Amos Kong; +Cc: lcapitulino, qemu-devel, stefanha, mst
Amos Kong <akong@redhat.com> writes:
> On Wed, Jun 26, 2013 at 11:54:20AM +0200, Markus Armbruster wrote:
>> Amos Kong <akong@redhat.com> 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 <akong@redhat.com>
>
> Thanks for your comments, some comments are out-of-data, I removed
> them in this reply.
Okay :)
>> > +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.
>
> Good suggestion, added to my todolist.
>
>> > +
>> > + 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.
>> > + 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?
>
> We should notify in error path if n->mac_table is changed.
>
>> > @@ -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/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 an error note is bettr than a NULL list. Management should not query
> the rx-filter info if there is no net client supports it.
I disagree. If a management application asks a question we can answer,
we should give it a straight answer, not an error.
ls of an empty directory prints nothing and succeeds. It doesn't
concern itself with whether I should or should not ask for the
directory's contents, say because nobody but me can access the
directory, and I therefore should know perfectly well that it's empty.
> {
> "return": [
> ]
> }
>
> NULL list might be misread to that it supports rx-filter querying, but the
> rx-filter config has no item.
If such a misunderstanding is possible, the command's specification
needs fixing.
If I read the specification correctly, the returned list has one element
per selected NIC that supports rx-filter querying, and no more.
Without the optional argument, all NICs are selected. With the optional
argument, just the NIC identified by the argument is selected.
Whether the "rx-filter config has no item" shouldn't matter. I don't
even understand what that means :)
>> > + }
>> > + }
>> > +
>> > + 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.
>
> Reasonable.
>
>> > +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?
>
> We will not.
>
> It's ugly to check if nic is virtio-net nic in net/net.c, so I
> register the query function to NetClientInfo. Traversal the net
> client list in net/net.c, and execute query of each virtio-net
> instance in virtio-net.c
Implementing the feature as an optional callback is fine.
Let me rephrase my question: could this feature be implemented for other
NICs? I'm *not* asking you to do that, just whether it would be
possible.
I'm asking because my review of the QAPI schema depends on the answer.
>> 2. If the former, are you reasonably sure this object will do for other
>> NICs?
>
> No.
I'm not sure I understand you. Do you mean to say that the feature
could be implemented for other NICs, but RxFilterInfo would probably not
fit for them?
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
2013-07-02 9:05 ` Markus Armbruster
@ 2013-07-02 10:40 ` Amos Kong
2013-07-02 13:27 ` Markus Armbruster
0 siblings, 1 reply; 47+ messages in thread
From: Amos Kong @ 2013-07-02 10:40 UTC (permalink / raw)
To: Markus Armbruster, Eric Blake; +Cc: lcapitulino, qemu-devel, stefanha, mst
On Tue, Jul 02, 2013 at 11:05:56AM +0200, Markus Armbruster wrote:
> Amos Kong <akong@redhat.com> writes:
> >> > 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 an error note is bettr than a NULL list. Management should not query
> > the rx-filter info if there is no net client supports it.
>
> I disagree. If a management application asks a question we can answer,
> we should give it a straight answer, not an error.
>
> ls of an empty directory prints nothing and succeeds. It doesn't
> concern itself with whether I should or should not ask for the
> directory's contents, say because nobody but me can access the
> directory, and I therefore should know perfectly well that it's empty.
Reasonable.
> > {
> > "return": [
> > ]
> > }
> >
> > NULL list might be misread to that it supports rx-filter querying, but the
> > rx-filter config has no item.
>
> If such a misunderstanding is possible, the command's specification
> needs fixing.
>
> If I read the specification correctly, the returned list has one element
> per selected NIC that supports rx-filter querying, and no more.
>
> Without the optional argument, all NICs are selected. With the optional
> argument, just the NIC identified by the argument is selected.
We don't support filter by net client name in latest version.
> Whether the "rx-filter config has no item" shouldn't matter. I don't
> even understand what that means :)
I'm wrong, if one nic doesn't have rx-filter config entry, a NULL dict
will also be returned.
"return": [
{ }
]
I will return NULL table in this case.
> >> > + }
> >> > + }
> >> > +
> >> > + return filter_list;
> >> > +}
> >> > +
> >> > +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?
> >
> > We will not.
> >
> > It's ugly to check if nic is virtio-net nic in net/net.c, so I
> > register the query function to NetClientInfo. Traversal the net
> > client list in net/net.c, and execute query of each virtio-net
> > instance in virtio-net.c
>
> Implementing the feature as an optional callback is fine.
>
> Let me rephrase my question: could this feature be implemented for other
> NICs? I'm *not* asking you to do that, just whether it would be
> possible.
>
> I'm asking because my review of the QAPI schema depends on the answer.
>
> >> 2. If the former, are you reasonably sure this object will do for other
> >> NICs?
> >
> > No.
>
> I'm not sure I understand you. Do you mean to say that the feature
> could be implemented for other NICs, but RxFilterInfo would probably not
> fit for them?
We will not implement the feature to other NICs, no request.
We notify the management of virtio-net rx-filter change, because
we want to sync the the rx-filter change to macvtap device.
--
Amos.
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
2013-07-02 10:40 ` Amos Kong
@ 2013-07-02 13:27 ` Markus Armbruster
2013-07-04 3:31 ` Amos Kong
0 siblings, 1 reply; 47+ messages in thread
From: Markus Armbruster @ 2013-07-02 13:27 UTC (permalink / raw)
To: Amos Kong; +Cc: lcapitulino, qemu-devel, stefanha, mst
Amos Kong <akong@redhat.com> writes:
> On Tue, Jul 02, 2013 at 11:05:56AM +0200, Markus Armbruster wrote:
>> Amos Kong <akong@redhat.com> writes:
[...]
>> >> 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?
>> >
>> > We will not.
>> >
>> > It's ugly to check if nic is virtio-net nic in net/net.c, so I
>> > register the query function to NetClientInfo. Traversal the net
>> > client list in net/net.c, and execute query of each virtio-net
>> > instance in virtio-net.c
>>
>> Implementing the feature as an optional callback is fine.
>>
>> Let me rephrase my question: could this feature be implemented for other
>> NICs? I'm *not* asking you to do that, just whether it would be
>> possible.
>>
>> I'm asking because my review of the QAPI schema depends on the answer.
>>
>> >> 2. If the former, are you reasonably sure this object will do for other
>> >> NICs?
>> >
>> > No.
>>
>> I'm not sure I understand you. Do you mean to say that the feature
>> could be implemented for other NICs, but RxFilterInfo would probably not
>> fit for them?
>
> We will not implement the feature to other NICs, no request.
>
> We notify the management of virtio-net rx-filter change, because
> we want to sync the the rx-filter change to macvtap device.
I understand there are no plans to implement this feature for other
NICs. But I'm not asking whether we *want* to implement it for other
NICs, I'm asking whether we *could*.
Or rephrased yet another way: what exactly makes this feature applicable
to virtio-net only?
If the answer is "nothing", then we *could* implement it for other NICs.
Else, implementing it for other NICs would be impossible.
Once again, I'm not asking because I want it implemented for other
NICs. I'm asking because the answer affects my review of the schema.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
2013-07-02 13:27 ` Markus Armbruster
@ 2013-07-04 3:31 ` Amos Kong
2013-07-04 6:28 ` Markus Armbruster
0 siblings, 1 reply; 47+ messages in thread
From: Amos Kong @ 2013-07-04 3:31 UTC (permalink / raw)
To: Markus Armbruster; +Cc: lcapitulino, qemu-devel, stefanha, mst
On Tue, Jul 02, 2013 at 03:27:12PM +0200, Markus Armbruster wrote:
> Amos Kong <akong@redhat.com> writes:
>
> > On Tue, Jul 02, 2013 at 11:05:56AM +0200, Markus Armbruster wrote:
> >> Amos Kong <akong@redhat.com> writes:
> [...]
> >> >> 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?
> >> >
> >> > We will not.
> >> >
> >> > It's ugly to check if nic is virtio-net nic in net/net.c, so I
> >> > register the query function to NetClientInfo. Traversal the net
> >> > client list in net/net.c, and execute query of each virtio-net
> >> > instance in virtio-net.c
> >>
> >> Implementing the feature as an optional callback is fine.
> >>
> >> Let me rephrase my question: could this feature be implemented for other
> >> NICs? I'm *not* asking you to do that, just whether it would be
> >> possible.
> >>
> >> I'm asking because my review of the QAPI schema depends on the answer.
> >>
> >> >> 2. If the former, are you reasonably sure this object will do for other
> >> >> NICs?
> >> >
> >> > No.
> >>
> >> I'm not sure I understand you. Do you mean to say that the feature
> >> could be implemented for other NICs, but RxFilterInfo would probably not
> >> fit for them?
> >
> > We will not implement the feature to other NICs, no request.
> >
> > We notify the management of virtio-net rx-filter change, because
> > we want to sync the the rx-filter change to macvtap device.
>
> I understand there are no plans to implement this feature for other
> NICs. But I'm not asking whether we *want* to implement it for other
> NICs, I'm asking whether we *could*.
In theory, we can.
> Or rephrased yet another way: what exactly makes this feature applicable
> to virtio-net only?
Macvtap can only be used by virtio-net, not other emulated nic.
It's meaningless for management to know the rx-filter change of
non-virtio-net NICs.
> If the answer is "nothing", then we *could* implement it for other NICs.
> Else, implementing it for other NICs would be impossible.
>
> Once again, I'm not asking because I want it implemented for other
> NICs. I'm asking because the answer affects my review of the schema.
--
Amos.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
2013-07-04 3:31 ` Amos Kong
@ 2013-07-04 6:28 ` Markus Armbruster
2013-07-11 14:05 ` Amos Kong
0 siblings, 1 reply; 47+ messages in thread
From: Markus Armbruster @ 2013-07-04 6:28 UTC (permalink / raw)
To: Amos Kong; +Cc: lcapitulino, qemu-devel, stefanha, mst
Amos Kong <akong@redhat.com> writes:
> On Tue, Jul 02, 2013 at 03:27:12PM +0200, Markus Armbruster wrote:
>> Amos Kong <akong@redhat.com> writes:
>>
>> > On Tue, Jul 02, 2013 at 11:05:56AM +0200, Markus Armbruster wrote:
>> >> Amos Kong <akong@redhat.com> writes:
>> [...]
>> >> >> 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?
>> >> >
>> >> > We will not.
>> >> >
>> >> > It's ugly to check if nic is virtio-net nic in net/net.c, so I
>> >> > register the query function to NetClientInfo. Traversal the net
>> >> > client list in net/net.c, and execute query of each virtio-net
>> >> > instance in virtio-net.c
>> >>
>> >> Implementing the feature as an optional callback is fine.
>> >>
>> >> Let me rephrase my question: could this feature be implemented for other
>> >> NICs? I'm *not* asking you to do that, just whether it would be
>> >> possible.
>> >>
>> >> I'm asking because my review of the QAPI schema depends on the answer.
>> >>
>> >> >> 2. If the former, are you reasonably sure this object will do for other
>> >> >> NICs?
>> >> >
>> >> > No.
>> >>
>> >> I'm not sure I understand you. Do you mean to say that the feature
>> >> could be implemented for other NICs, but RxFilterInfo would probably not
>> >> fit for them?
>> >
>> > We will not implement the feature to other NICs, no request.
>> >
>> > We notify the management of virtio-net rx-filter change, because
>> > we want to sync the the rx-filter change to macvtap device.
>>
>> I understand there are no plans to implement this feature for other
>> NICs. But I'm not asking whether we *want* to implement it for other
>> NICs, I'm asking whether we *could*.
>
> In theory, we can.
>
>> Or rephrased yet another way: what exactly makes this feature applicable
>> to virtio-net only?
>
> Macvtap can only be used by virtio-net, not other emulated nic.
> It's meaningless for management to know the rx-filter change of
> non-virtio-net NICs.
I'm having trouble squaring "in theory, we can" with "meaningless". So
I'm rephrasing my question yet again.
Do NICs other than virtio-net have rx-filters?
If yes, what have these NIC rx-filters in common, and how do they
differ?
Why would anybody want to query rx-filters? Use cases, please.
Why is querying rx-filters "meaningless" for anything but virtio-net?
The dictionary explains "meaningless" as "having no meaning; of no
value". Thus, for the query to be meaningless, the answer must carry no
information, or at least none of value. Is querying rx-filters really
meaningless? Or is it just something we don't need right now, and can't
see being needed in the future?
>> If the answer is "nothing", then we *could* implement it for other NICs.
>> Else, implementing it for other NICs would be impossible.
>>
>> Once again, I'm not asking because I want it implemented for other
>> NICs. I'm asking because the answer affects my review of the schema.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
2013-07-04 6:28 ` Markus Armbruster
@ 2013-07-11 14:05 ` Amos Kong
2013-07-12 6:32 ` Markus Armbruster
0 siblings, 1 reply; 47+ messages in thread
From: Amos Kong @ 2013-07-11 14:05 UTC (permalink / raw)
To: Markus Armbruster; +Cc: lcapitulino, qemu-devel, stefanha, mst
On Thu, Jul 04, 2013 at 08:28:59AM +0200, Markus Armbruster wrote:
> Amos Kong <akong@redhat.com> writes:
>
> > On Tue, Jul 02, 2013 at 03:27:12PM +0200, Markus Armbruster wrote:
> >> Amos Kong <akong@redhat.com> writes:
> >>
> >> > On Tue, Jul 02, 2013 at 11:05:56AM +0200, Markus Armbruster wrote:
> >> >> Amos Kong <akong@redhat.com> writes:
> >> [...]
> >> >> >> 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?
> >> >> >
> >> >> > We will not.
> >> >> >
> >> >> > It's ugly to check if nic is virtio-net nic in net/net.c, so I
> >> >> > register the query function to NetClientInfo. Traversal the net
> >> >> > client list in net/net.c, and execute query of each virtio-net
> >> >> > instance in virtio-net.c
> >> >>
> >> >> Implementing the feature as an optional callback is fine.
> >> >>
> >> >> Let me rephrase my question: could this feature be implemented for other
> >> >> NICs? I'm *not* asking you to do that, just whether it would be
> >> >> possible.
> >> >>
> >> >> I'm asking because my review of the QAPI schema depends on the answer.
> >> >>
> >> >> >> 2. If the former, are you reasonably sure this object will do for other
> >> >> >> NICs?
> >> >> >
> >> >> > No.
> >> >>
> >> >> I'm not sure I understand you. Do you mean to say that the feature
> >> >> could be implemented for other NICs, but RxFilterInfo would probably not
> >> >> fit for them?
> >> >
> >> > We will not implement the feature to other NICs, no request.
> >> >
> >> > We notify the management of virtio-net rx-filter change, because
> >> > we want to sync the the rx-filter change to macvtap device.
> >>
> >> I understand there are no plans to implement this feature for other
> >> NICs. But I'm not asking whether we *want* to implement it for other
> >> NICs, I'm asking whether we *could*.
> >
> > In theory, we can.
> >
> >> Or rephrased yet another way: what exactly makes this feature applicable
> >> to virtio-net only?
> >
> > Macvtap can only be used by virtio-net, not other emulated nic.
> > It's meaningless for management to know the rx-filter change of
> > non-virtio-net NICs.
>
> I'm having trouble squaring "in theory, we can" with "meaningless". So
> I'm rephrasing my question yet again.
>
> Do NICs other than virtio-net have rx-filters?
Yes.
Talked with Jason, upstream kernel fixed some bugs, then we can also
use e1000 with macvtap. In this case, we should also implement a
.query_rx_filter function for e1000. We can do it by another patchset.
> If yes, what have these NIC rx-filters in common, and how do they
> differ?
>
> Why would anybody want to query rx-filters? Use cases, please.
It's a way to tell management the rx-filter setup in guest nic.
Management query the rx-filter setup of guest, then change the
setup of macvtap device, macvtap uses same rx-filter setup as
virtual nic.
> Why is querying rx-filters "meaningless" for anything but virtio-net?
> The dictionary explains "meaningless" as "having no meaning; of no
> value". Thus, for the query to be meaningless, the answer must carry no
> information, or at least none of value. Is querying rx-filters really
> meaningless? Or is it just something we don't need right now, and can't
> see being needed in the future?
--
Amos.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
2013-07-11 14:05 ` Amos Kong
@ 2013-07-12 6:32 ` Markus Armbruster
2013-07-12 7:07 ` Amos Kong
0 siblings, 1 reply; 47+ messages in thread
From: Markus Armbruster @ 2013-07-12 6:32 UTC (permalink / raw)
To: Amos Kong; +Cc: lcapitulino, qemu-devel, stefanha, mst
Amos Kong <akong@redhat.com> writes:
> On Thu, Jul 04, 2013 at 08:28:59AM +0200, Markus Armbruster wrote:
>> Amos Kong <akong@redhat.com> writes:
>>
>> > On Tue, Jul 02, 2013 at 03:27:12PM +0200, Markus Armbruster wrote:
>> >> Amos Kong <akong@redhat.com> writes:
>> >>
>> >> > On Tue, Jul 02, 2013 at 11:05:56AM +0200, Markus Armbruster wrote:
>> >> >> Amos Kong <akong@redhat.com> writes:
>> >> [...]
>> >> >> >> 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?
>> >> >> >
>> >> >> > We will not.
>> >> >> >
>> >> >> > It's ugly to check if nic is virtio-net nic in net/net.c, so I
>> >> >> > register the query function to NetClientInfo. Traversal the net
>> >> >> > client list in net/net.c, and execute query of each virtio-net
>> >> >> > instance in virtio-net.c
>> >> >>
>> >> >> Implementing the feature as an optional callback is fine.
>> >> >>
>> >> >> Let me rephrase my question: could this feature be implemented for other
>> >> >> NICs? I'm *not* asking you to do that, just whether it would be
>> >> >> possible.
>> >> >>
>> >> >> I'm asking because my review of the QAPI schema depends on the answer.
>> >> >>
>> >> >> >> 2. If the former, are you reasonably sure this object will
>> >> >> >> do for other
>> >> >> >> NICs?
>> >> >> >
>> >> >> > No.
>> >> >>
>> >> >> I'm not sure I understand you. Do you mean to say that the feature
>> >> >> could be implemented for other NICs, but RxFilterInfo would probably not
>> >> >> fit for them?
>> >> >
>> >> > We will not implement the feature to other NICs, no request.
>> >> >
>> >> > We notify the management of virtio-net rx-filter change, because
>> >> > we want to sync the the rx-filter change to macvtap device.
>> >>
>> >> I understand there are no plans to implement this feature for other
>> >> NICs. But I'm not asking whether we *want* to implement it for other
>> >> NICs, I'm asking whether we *could*.
>> >
>> > In theory, we can.
>> >
>> >> Or rephrased yet another way: what exactly makes this feature applicable
>> >> to virtio-net only?
>> >
>> > Macvtap can only be used by virtio-net, not other emulated nic.
>> > It's meaningless for management to know the rx-filter change of
>> > non-virtio-net NICs.
>>
>> I'm having trouble squaring "in theory, we can" with "meaningless". So
>> I'm rephrasing my question yet again.
>>
>> Do NICs other than virtio-net have rx-filters?
>
> Yes.
>
> Talked with Jason, upstream kernel fixed some bugs, then we can also
> use e1000 with macvtap. In this case, we should also implement a
> .query_rx_filter function for e1000. We can do it by another patchset.
Yes. Just to avoid misunderstandings: I'm not asking you for that. I
merely asked whether it's possible, and you answered that.
>> If yes, what have these NIC rx-filters in common, and how do they
>> differ?
>>
>> Why would anybody want to query rx-filters? Use cases, please.
>
> It's a way to tell management the rx-filter setup in guest nic.
> Management query the rx-filter setup of guest, then change the
> setup of macvtap device, macvtap uses same rx-filter setup as
> virtual nic.
So this use case is "mirror the virtual NIC's rx-filter setup to
macvtap". Makes sense.
This leads me to the question I've been aiming for all along: will your
definition of RxFilterInfo do for devices other than virtio-net?
It should do if it contains only stuff that all NICs with an rx-filter
have. Is that the case?
[...]
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
2013-07-12 6:32 ` Markus Armbruster
@ 2013-07-12 7:07 ` Amos Kong
0 siblings, 0 replies; 47+ messages in thread
From: Amos Kong @ 2013-07-12 7:07 UTC (permalink / raw)
To: Markus Armbruster; +Cc: lcapitulino, qemu-devel, stefanha, mst
On Fri, Jul 12, 2013 at 08:32:29AM +0200, Markus Armbruster wrote:
> Amos Kong <akong@redhat.com> writes:
>
> > On Thu, Jul 04, 2013 at 08:28:59AM +0200, Markus Armbruster wrote:
> >> Amos Kong <akong@redhat.com> writes:
> >>
> >> > On Tue, Jul 02, 2013 at 03:27:12PM +0200, Markus Armbruster wrote:
> >> >> Amos Kong <akong@redhat.com> writes:
> >> >>
> >> >> > On Tue, Jul 02, 2013 at 11:05:56AM +0200, Markus Armbruster wrote:
> >> >> >> Amos Kong <akong@redhat.com> writes:
> >> >> [...]
> >> >> >> >> 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?
> >> >> >> >
> >> >> >> > We will not.
> >> >> >> >
> >> >> >> > It's ugly to check if nic is virtio-net nic in net/net.c, so I
> >> >> >> > register the query function to NetClientInfo. Traversal the net
> >> >> >> > client list in net/net.c, and execute query of each virtio-net
> >> >> >> > instance in virtio-net.c
> >> >> >>
> >> >> >> Implementing the feature as an optional callback is fine.
> >> >> >>
> >> >> >> Let me rephrase my question: could this feature be implemented for other
> >> >> >> NICs? I'm *not* asking you to do that, just whether it would be
> >> >> >> possible.
> >> >> >>
> >> >> >> I'm asking because my review of the QAPI schema depends on the answer.
> >> >> >>
> >> >> >> >> 2. If the former, are you reasonably sure this object will
> >> >> >> >> do for other
> >> >> >> >> NICs?
> >> >> >> >
> >> >> >> > No.
> >> >> >>
> >> >> >> I'm not sure I understand you. Do you mean to say that the feature
> >> >> >> could be implemented for other NICs, but RxFilterInfo would probably not
> >> >> >> fit for them?
> >> >> >
> >> >> > We will not implement the feature to other NICs, no request.
> >> >> >
> >> >> > We notify the management of virtio-net rx-filter change, because
> >> >> > we want to sync the the rx-filter change to macvtap device.
> >> >>
> >> >> I understand there are no plans to implement this feature for other
> >> >> NICs. But I'm not asking whether we *want* to implement it for other
> >> >> NICs, I'm asking whether we *could*.
> >> >
> >> > In theory, we can.
> >> >
> >> >> Or rephrased yet another way: what exactly makes this feature applicable
> >> >> to virtio-net only?
> >> >
> >> > Macvtap can only be used by virtio-net, not other emulated nic.
> >> > It's meaningless for management to know the rx-filter change of
> >> > non-virtio-net NICs.
> >>
> >> I'm having trouble squaring "in theory, we can" with "meaningless". So
> >> I'm rephrasing my question yet again.
> >>
> >> Do NICs other than virtio-net have rx-filters?
> >
> > Yes.
> >
> > Talked with Jason, upstream kernel fixed some bugs, then we can also
> > use e1000 with macvtap. In this case, we should also implement a
> > .query_rx_filter function for e1000. We can do it by another patchset.
>
> Yes. Just to avoid misunderstandings: I'm not asking you for that. I
> merely asked whether it's possible, and you answered that.
>
> >> If yes, what have these NIC rx-filters in common, and how do they
> >> differ?
> >>
> >> Why would anybody want to query rx-filters? Use cases, please.
> >
> > It's a way to tell management the rx-filter setup in guest nic.
> > Management query the rx-filter setup of guest, then change the
> > setup of macvtap device, macvtap uses same rx-filter setup as
> > virtual nic.
>
> So this use case is "mirror the virtual NIC's rx-filter setup to
> macvtap". Makes sense.
>
> This leads me to the question I've been aiming for all along: will your
> definition of RxFilterInfo do for devices other than virtio-net?
query_rx_filter() returns the address of nic's RxFilterInfo.
RxFilterInfo is general.
> It should do if it contains only stuff that all NICs with an rx-filter
> have. Is that the case?
Yes.
--
Amos.
^ permalink raw reply [flat|nested] 47+ messages in thread