On Tue, Jul 02, 2013 at 11:05:56AM +0200, Markus Armbruster wrote: > Amos Kong <ak...@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.