Re: [Qemu-devel] [PATCH MST/PCI] additional fixes for mac-programming feature

[Michael S. Tsirkin]

On Thu, Jul 11, 2013 at 10:28:15PM +0800, Amos Kong wrote:
> Markus added some comments on old patchset, this patch contains
> some additional fixes, it's based on MST's PCI tree.
> 
> * Fix typos (missed 1.6, NIC)
> * Don't initialize list point at its declaration
> * Always notify QMP client if mactable is changed
> * Returns NULL list if no net client supports rx-filter querying.
> 
> BTW, we can also use e1000 with macvtap device, so we can implement
> a query_rx_filter function for e1000 in future.
> 
> Signed-off-by: Amos Kong <address@hidden>

I applied this and folded into your previous
patch. Pushed on my pci branch.
Amos, could you please verify (and test)
that the result is correct?

Thanks,


> ---
>  hw/net/virtio-net.c | 24 ++++++++++++++----------
>  net/net.c           | 19 ++++++++++---------
>  qapi-schema.json    | 11 ++++++-----
>  qmp-commands.hx     | 10 +++++-----
>  4 files changed, 35 insertions(+), 29 deletions(-)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index c88403a..679f50c 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -226,10 +226,8 @@ static RxFilterInfo 
> *virtio_net_query_rxfilter(NetClientState *nc)
>  {
>      VirtIONet *n = qemu_get_nic_opaque(nc);
>      RxFilterInfo *info;
> -    strList *str_list = NULL;
> -    strList *entry;
> -    intList *int_list = NULL;
> -    intList *int_entry;
> +    strList *str_list, *entry;
> +    intList *int_list, *int_entry;
>      int i, j;
>  
>      info = g_malloc0(sizeof(*info));
> @@ -258,6 +256,7 @@ static RxFilterInfo 
> *virtio_net_query_rxfilter(NetClientState *nc)
>  
>      info->main_mac = mac_strdup_printf(n->mac);
>  
> +    str_list = NULL;
>      for (i = 0; i < n->mac_table.first_multi; i++) {
>          entry = g_malloc0(sizeof(*entry));
>          entry->value = mac_strdup_printf(n->mac_table.macs + i * ETH_ALEN);
> @@ -275,6 +274,7 @@ static RxFilterInfo 
> *virtio_net_query_rxfilter(NetClientState *nc)
>      }
>      info->multicast_table = str_list;
>  
> +    int_list = NULL;
>      for (i = 0; i < MAX_VLAN >> 5; i++) {
>          for (j = 0; n->vlans[i] && j < 0x1f; j++) {
>              if (n->vlans[i] & (1U << j)) {
> @@ -619,19 +619,19 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t 
> cmd,
>                     sizeof(mac_data.entries));
>      mac_data.entries = ldl_p(&mac_data.entries);
>      if (s != sizeof(mac_data.entries)) {
> -        return VIRTIO_NET_ERR;
> +        goto error;
>      }
>      iov_discard_front(&iov, &iov_cnt, s);
>  
>      if (mac_data.entries * ETH_ALEN > iov_size(iov, iov_cnt)) {
> -        return VIRTIO_NET_ERR;
> +        goto error;
>      }
>  
>      if (mac_data.entries <= MAC_TABLE_ENTRIES) {
>          s = iov_to_buf(iov, iov_cnt, 0, n->mac_table.macs,
>                         mac_data.entries * ETH_ALEN);
>          if (s != mac_data.entries * ETH_ALEN) {
> -            return VIRTIO_NET_ERR;
> +            goto error;
>          }
>          n->mac_table.in_use += mac_data.entries;
>      } else {
> @@ -646,20 +646,20 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t 
> cmd,
>                     sizeof(mac_data.entries));
>      mac_data.entries = ldl_p(&mac_data.entries);
>      if (s != sizeof(mac_data.entries)) {
> -        return VIRTIO_NET_ERR;
> +        goto error;
>      }
>  
>      iov_discard_front(&iov, &iov_cnt, s);
>  
>      if (mac_data.entries * ETH_ALEN != iov_size(iov, iov_cnt)) {
> -        return VIRTIO_NET_ERR;
> +        goto error;
>      }
>  
>      if (n->mac_table.in_use + mac_data.entries <= MAC_TABLE_ENTRIES) {
>          s = iov_to_buf(iov, iov_cnt, 0, n->mac_table.macs,
>                         mac_data.entries * ETH_ALEN);
>          if (s != mac_data.entries * ETH_ALEN) {
> -            return VIRTIO_NET_ERR;
> +            goto error;
>          }
>          n->mac_table.in_use += mac_data.entries;
>      } else {
> @@ -669,6 +669,10 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t 
> cmd,
>      rxfilter_notify(nc);
>  
>      return VIRTIO_NET_OK;
> +
> +error:
> +    rxfilter_notify(nc);
> +    return VIRTIO_NET_ERR;
>  }
>  
>  static int virtio_net_handle_vlan_table(VirtIONet *n, uint8_t cmd,
> diff --git a/net/net.c b/net/net.c
> index 33abffe..c0d61bf 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -971,11 +971,16 @@ RxFilterInfoList *qmp_query_rx_filter(bool has_name, 
> const char *name,
>          RxFilterInfoList *entry;
>          RxFilterInfo *info;
>  
> -        /* only query rx-filter information of nic */
> -        if (nc->info->type != NET_CLIENT_OPTIONS_KIND_NIC) {
> +        if (has_name && strcmp(nc->name, name) != 0) {
>              continue;
>          }
> -        if (has_name && strcmp(nc->name, name) != 0) {
> +
> +        /* only query rx-filter information of NIC */
> +        if (nc->info->type != NET_CLIENT_OPTIONS_KIND_NIC) {
> +            if (has_name) {
> +                error_setg(errp, "net client(%s) isn't a NIC", name);
> +                break;
> +            }
>              continue;
>          }
>  
> @@ -997,12 +1002,8 @@ RxFilterInfoList *qmp_query_rx_filter(bool has_name, 
> const char *name,
>          }
>      }
>  
> -    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");
> -        }
> +    if (filter_list == NULL && !error_is_set(errp) && has_name) {
> +        error_setg(errp, "invalid net client name: %s", name);
>      }
>  
>      return filter_list;
> diff --git a/qapi-schema.json b/qapi-schema.json
> index da45980..95ba5a6 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3724,13 +3724,14 @@
>  #
>  # @all: receive all assigned packets
>  #
> +# Since: 1.6
>  ##
>  { 'enum': 'RxState', 'data': [ 'normal', 'none', 'all' ] }
>  
>  ##
>  # @RxFilterInfo:
>  #
> -# Rx-filter information for a nic.
> +# Rx-filter information for a NIC.
>  #
>  # @name: net client name
>  #
> @@ -3774,14 +3775,14 @@
>  ##
>  # @query-rx-filter:
>  #
> -# Return rx-filter information for all nics (or for the given nic).
> +# 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: 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
> +#          NIC doesn't support rx-filter querying, or given net client
> +#          isn't a NIC.
>  #
>  # Since: 1.6
>  ##
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 37113e5..aee3f96 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -3055,12 +3055,12 @@ query-rx-filter
>  
>  Show rx-filter information.
>  
> -Returns a json-array of rx-filter information for all nics (or for the
> -given nic), returning an error if the given nic doesn't exist, or
> -given nic doesn't support rx-filter querying, or no net client
> -supports rx-filter querying.
> +Returns a json-array of rx-filter information for all NICs (or for the
> +given NIC), returning an error if the given NIC doesn't exist, or
> +given NIC doesn't support rx-filter querying, or given net client
> +isn't a NIC.
>  
> -The query will clear the event notification flag of each nic, then qemu
> +The query will clear the event notification flag of each NIC, then qemu
>  will start to emit event to QMP monitor.
>  
>  Each array entry contains the following:
> -- 
> 1.8.3.1