[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filte
From: |
Amos Kong |
Subject: |
Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information |
Date: |
Tue, 2 Jul 2013 14:33:01 +0800 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
On Wed, Jun 26, 2013 at 11:54:20AM +0200, Markus Armbruster wrote:
> Amos Kong <address@hidden> writes:
>
> > We want to implement mac programming over macvtap through Libvirt,
> > related rx-filter configuration contains main mac, some of rx-mode
> > and mac-table.
> >
> > The previous patch adds QMP event to notify management of rx-filter
> > change. This patch adds a monitor command to query rx-filter
> > information.
> >
> > A flag is used to avoid events flooding, if user don't query
> > rx-filter after receives one event, new events won't be sent
> > to qmp monitor.
> >
> > (qemu) info rx-filter vnet0
> > vnet0:
> > \ promiscuous: on
> > \ multicast: normal
> > \ unicast: normal
> > \ broadcast-allowed: off
> > \ multicast-overflow: off
> > \ unicast-overflow: off
> > \ main-mac: 52:54:00:12:34:56
> > \ unicast-table:
> > \ multicast-table:
> > 01:00:5e:00:00:01
> > 33:33:00:00:00:01
> > 33:33:ff:12:34:56
> >
> > Signed-off-by: Amos Kong <address@hidden>
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.
- Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information,
Amos Kong <=
- Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information, Markus Armbruster, 2013/07/02
- Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information, Amos Kong, 2013/07/02
- Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information, Markus Armbruster, 2013/07/02
- Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information, Amos Kong, 2013/07/03
- Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information, Markus Armbruster, 2013/07/04
- Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information, Amos Kong, 2013/07/11
- Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information, Markus Armbruster, 2013/07/12
- Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information, Amos Kong, 2013/07/12