| Message ID | 1373552895-17349-1-git-send-email-akong@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 07/11/2013 08:28 AM, 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 <akong@redhat.com> > --- > 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(-) Reviewed-by: Eric Blake <eblake@redhat.com> > @@ -3724,13 +3724,14 @@ > # > # @all: receive all assigned packets > # > +# Since: 1.6 > ## > { 'enum': 'RxState', 'data': [ 'normal', 'none', 'all' ] } I don't see RxState in the latest qemu.git (c170a23c); is it worth trying to squash this in place to the pull request of the earlier series, instead of as a followup patch?
On Thu, Jul 11, 2013 at 12:52:56PM -0600, Eric Blake wrote: > On 07/11/2013 08:28 AM, 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 <akong@redhat.com> > > --- > > 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(-) > > Reviewed-by: Eric Blake <eblake@redhat.com> > > > @@ -3724,13 +3724,14 @@ > > # > > # @all: receive all assigned packets > > # > > +# Since: 1.6 > > ## > > { 'enum': 'RxState', 'data': [ 'normal', 'none', 'all' ] } > > I don't see RxState in the latest qemu.git (c170a23c); is it worth > trying to squash this in place to the pull request of the earlier > series, instead of as a followup patch? > > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org > It's easy for me to squash when I apply. Marcus - you were the second persons with comments here, are you happy with follow-up patches or do you prefer a full repost?
Amos Kong <akong@redhat.com> writes: > 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 <akong@redhat.com> Addresses those parts of my review that have made it into the PCI tree, thus: Reviewed-by: Markus Armbruster <armbru@redhat.com> One suggestion inline. > --- > 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); entry->next = str_list; str_list = entry; } info->unicast_table = str_list; str_list = NULL; for (i = n->mac_table.first_multi; i < n->mac_table.in_use; i++) { entry = g_malloc0(sizeof(*entry)); entry->value = mac_strdup_printf(n->mac_table.macs + i * ETH_ALEN); entry->next = str_list; str_list = entry; } info->multicast_table = str_list; Suggest a helper function to build str_list. Can be done as a follow-up patch, no need to respin this one. [...]
"Michael S. Tsirkin" <mst@redhat.com> writes: > On Thu, Jul 11, 2013 at 12:52:56PM -0600, Eric Blake wrote: >> On 07/11/2013 08:28 AM, 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 <akong@redhat.com> >> > --- >> > 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(-) >> >> Reviewed-by: Eric Blake <eblake@redhat.com> >> >> > @@ -3724,13 +3724,14 @@ >> > # >> > # @all: receive all assigned packets >> > # >> > +# Since: 1.6 >> > ## >> > { 'enum': 'RxState', 'data': [ 'normal', 'none', 'all' ] } >> >> I don't see RxState in the latest qemu.git (c170a23c); is it worth >> trying to squash this in place to the pull request of the earlier >> series, instead of as a followup patch? >> >> -- >> Eric Blake eblake redhat com +1-919-301-3266 >> Libvirt virtualization library http://libvirt.org >> > > > It's easy for me to squash when I apply. Yes, please squash. > Marcus - you were the second persons with comments here, are you happy > with follow-up patches or do you prefer a full repost? I'm fine, thanks.
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 <akong@redhat.com> 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
On Mon, Jul 15, 2013 at 09:28:28PM +0300, Michael S. Tsirkin wrote: > 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 <akong@redhat.com> > > 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? I tested your latest pci branch, it's correct. I didn't find other issue in my test. Thanks, Amos > Thanks,
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:
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 <akong@redhat.com> --- 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(-)