On Wed, Jun 26, 2013 at 11:54:20AM +0200, Markus Armbruster wrote: > Amos Kong <ak...@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 <ak...@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.