Amos Kong <ak...@redhat.com> writes: > 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.
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?