Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information

[Amos Kong]

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