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

[Markus Armbruster]

Amos Kong <address@hidden> writes:

> 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.

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?