| Message ID | 1392604023-20142-1-git-send-email-akong@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 02/16/2014 09:27 PM, Amos Kong wrote: > Stefan Fritsch just fixed a virtio-net driver bug [1], virtio-net won't > filter out VLAN-tagged packets if VIRTIO_NET_F_CTRL_VLAN isn't negotiated. > > We should also not send the vlan table to management, this patch makes > the vlan-talbe optional. ^^^^^^^^^^ table. One question I have from the API perspective is can we suddenly change something to be optional? If there are any users of this , wouldn't they have to change now to check the existence of this list? Thanks -vlad > [1] http://lists.nongnu.org/archive/html/qemu-devel/2014-02/msg02604.html > > Signed-off-by: Amos Kong <akong@redhat.com> > --- > hw/net/virtio-net.c | 38 +++++++++++++++++++++++++------------- > qapi-schema.json | 4 ++-- > qmp-commands.hx | 2 +- > 3 files changed, 28 insertions(+), 16 deletions(-) > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > index 3626608..0b32e6a 100644 > --- a/hw/net/virtio-net.c > +++ b/hw/net/virtio-net.c > @@ -222,13 +222,33 @@ static char *mac_strdup_printf(const uint8_t *mac) > mac[1], mac[2], mac[3], mac[4], mac[5]); > } > > +static intList *get_vlan_table(VirtIONet *n) > +{ > + intList *list, *entry; > + int i, j; > + > + 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)) { > + entry = g_malloc0(sizeof(*entry)); > + entry->value = (i << 5) + j; > + entry->next = list; > + list = entry; > + } > + } > + } > + > + return list; > +} > + > static RxFilterInfo *virtio_net_query_rxfilter(NetClientState *nc) > { > VirtIONet *n = qemu_get_nic_opaque(nc); > + VirtIODevice *vdev = VIRTIO_DEVICE(n); > RxFilterInfo *info; > strList *str_list, *entry; > - intList *int_list, *int_entry; > - int i, j; > + int i; > > info = g_malloc0(sizeof(*info)); > info->name = g_strdup(nc->name); > @@ -274,18 +294,10 @@ 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)) { > - int_entry = g_malloc0(sizeof(*int_entry)); > - int_entry->value = (i << 5) + j; > - int_entry->next = int_list; > - int_list = int_entry; > - } > - } > + if ((1 << VIRTIO_NET_F_CTRL_VLAN) & vdev->guest_features) { > + info->has_vlan_table = true; > + info->vlan_table = get_vlan_table(n); > } > - info->vlan_table = int_list; > > /* enable event notification after query */ > nc->rxfilter_notify_enabled = 1; > diff --git a/qapi-schema.json b/qapi-schema.json > index 7cfb5e5..5d48fa9 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -4034,7 +4034,7 @@ > # > # @main-mac: the main macaddr string > # > -# @vlan-table: a list of active vlan id > +# @vlan-table: #optional a list of active vlan id > # > # @unicast-table: a list of unicast macaddr string > # > @@ -4053,7 +4053,7 @@ > 'multicast-overflow': 'bool', > 'unicast-overflow': 'bool', > 'main-mac': 'str', > - 'vlan-table': ['int'], > + '*vlan-table': ['int'], > 'unicast-table': ['str'], > 'multicast-table': ['str'] }} > > diff --git a/qmp-commands.hx b/qmp-commands.hx > index cce6b81..a1c1dfa 100644 > --- a/qmp-commands.hx > +++ b/qmp-commands.hx > @@ -3308,7 +3308,7 @@ Each array entry contains the following: > - "multicast-overflow": multicast table is overflowed (json-bool) > - "unicast-overflow": unicast table is overflowed (json-bool) > - "main-mac": main macaddr string (json-string) > -- "vlan-table": a json-array of active vlan id > +- "vlan-table": a json-array of active vlan id (optoinal) > - "unicast-table": a json-array of unicast macaddr string > - "multicast-table": a json-array of multicast macaddr string > >
On 02/16/2014 07:27 PM, Amos Kong wrote: > Stefan Fritsch just fixed a virtio-net driver bug [1], virtio-net won't > filter out VLAN-tagged packets if VIRTIO_NET_F_CTRL_VLAN isn't negotiated. > > We should also not send the vlan table to management, this patch makes > the vlan-talbe optional. s/talbe/table/ > > [1] http://lists.nongnu.org/archive/html/qemu-devel/2014-02/msg02604.html > > Signed-off-by: Amos Kong <akong@redhat.com> > --- > hw/net/virtio-net.c | 38 +++++++++++++++++++++++++------------- > qapi-schema.json | 4 ++-- > qmp-commands.hx | 2 +- > 3 files changed, 28 insertions(+), 16 deletions(-) > > +++ b/qapi-schema.json > @@ -4034,7 +4034,7 @@ > # > # @main-mac: the main macaddr string > # > -# @vlan-table: a list of active vlan id > +# @vlan-table: #optional a list of active vlan id > # > # @unicast-table: a list of unicast macaddr string > # > @@ -4053,7 +4053,7 @@ > 'multicast-overflow': 'bool', > 'unicast-overflow': 'bool', > 'main-mac': 'str', > - 'vlan-table': ['int'], > + '*vlan-table': ['int'], Indentation is now off. > - "main-mac": main macaddr string (json-string) > -- "vlan-table": a json-array of active vlan id > +- "vlan-table": a json-array of active vlan id (optoinal) s/optoinal/optional/ Those fixes are trivial enough, so I'm okay if you correct them then add: Reviewed-by: Eric Blake <eblake@redhat.com>
On 02/17/2014 09:52 AM, Eric Blake wrote: > On 02/16/2014 07:27 PM, Amos Kong wrote: >> Stefan Fritsch just fixed a virtio-net driver bug [1], virtio-net won't >> filter out VLAN-tagged packets if VIRTIO_NET_F_CTRL_VLAN isn't negotiated. >> >> We should also not send the vlan table to management, this patch makes >> the vlan-talbe optional. > > s/talbe/table/ > >> @@ -4053,7 +4053,7 @@ >> 'multicast-overflow': 'bool', >> 'unicast-overflow': 'bool', >> 'main-mac': 'str', >> - 'vlan-table': ['int'], >> + '*vlan-table': ['int'], > > Indentation is now off. > >> - "main-mac": main macaddr string (json-string) >> -- "vlan-table": a json-array of active vlan id >> +- "vlan-table": a json-array of active vlan id (optoinal) > > s/optoinal/optional/ > > Those fixes are trivial enough, so I'm okay if you correct them then add: Scratch that. I revoke my R-b, on the following grounds: On 02/17/2014 08:27 AM, Vlad Yasevich wrote: > On 02/16/2014 09:27 PM, Amos Kong wrote: >> Stefan Fritsch just fixed a virtio-net driver bug [1], virtio-net won't >> filter out VLAN-tagged packets if VIRTIO_NET_F_CTRL_VLAN isn't negotiated. >> >> We should also not send the vlan table to management, this patch makes >> the vlan-talbe optional. > ^^^^^^^^^^ > table. > > One question I have from the API perspective is can we suddenly change > something to be optional? If there are any users of this , > wouldn't they have to change now to check the existence of this > list? You are correct. Since the parameter is an output field, older clients may be depending on it existing. It is okay to generate an empty array, but you must not entirely omit the array unless you add further justification in your commit message that you are 100% positive that there are no clients of 1.6 that will be broken when the array no longer appears in the output. Can you rework the patch to just leave the array empty in the case where the bit does not indicate it is used? Or do we need to add a new bool field to the output for new enough management to know whether to use the array?
On 02/17/2014 11:56 AM, Eric Blake wrote: > On 02/17/2014 09:52 AM, Eric Blake wrote: >> On 02/16/2014 07:27 PM, Amos Kong wrote: >>> Stefan Fritsch just fixed a virtio-net driver bug [1], virtio-net won't >>> filter out VLAN-tagged packets if VIRTIO_NET_F_CTRL_VLAN isn't negotiated. >>> >>> We should also not send the vlan table to management, this patch makes >>> the vlan-talbe optional. >> >> s/talbe/table/ >> > >>> @@ -4053,7 +4053,7 @@ >>> 'multicast-overflow': 'bool', >>> 'unicast-overflow': 'bool', >>> 'main-mac': 'str', >>> - 'vlan-table': ['int'], >>> + '*vlan-table': ['int'], >> >> Indentation is now off. >> >>> - "main-mac": main macaddr string (json-string) >>> -- "vlan-table": a json-array of active vlan id >>> +- "vlan-table": a json-array of active vlan id (optoinal) >> >> s/optoinal/optional/ >> >> Those fixes are trivial enough, so I'm okay if you correct them then add: > > Scratch that. I revoke my R-b, on the following grounds: > > On 02/17/2014 08:27 AM, Vlad Yasevich wrote: >> On 02/16/2014 09:27 PM, Amos Kong wrote: >>> Stefan Fritsch just fixed a virtio-net driver bug [1], virtio-net won't >>> filter out VLAN-tagged packets if VIRTIO_NET_F_CTRL_VLAN isn't > negotiated. >>> >>> We should also not send the vlan table to management, this patch makes >>> the vlan-talbe optional. >> ^^^^^^^^^^ >> table. >> >> One question I have from the API perspective is can we suddenly change >> something to be optional? If there are any users of this , >> wouldn't they have to change now to check the existence of this >> list? > > You are correct. Since the parameter is an output field, older clients > may be depending on it existing. It is okay to generate an empty array, > but you must not entirely omit the array unless you add further > justification in your commit message that you are 100% positive that > there are no clients of 1.6 that will be broken when the array no longer > appears in the output. > > Can you rework the patch to just leave the array empty in the case where > the bit does not indicate it is used? Or do we need to add a new bool > field to the output for new enough management to know whether to use the > array? > I think a completely empty array should be sufficient. -vlad
On Mon, Feb 17, 2014 at 11:58:11AM -0500, Vlad Yasevich wrote: > On 02/17/2014 11:56 AM, Eric Blake wrote: > > On 02/17/2014 09:52 AM, Eric Blake wrote: > >> On 02/16/2014 07:27 PM, Amos Kong wrote: > >>> Stefan Fritsch just fixed a virtio-net driver bug [1], virtio-net won't > >>> filter out VLAN-tagged packets if VIRTIO_NET_F_CTRL_VLAN isn't negotiated. > >>> > >>> We should also not send the vlan table to management, this patch makes > >>> the vlan-talbe optional. > >> > >> s/talbe/table/ > >> > > > >>> @@ -4053,7 +4053,7 @@ > >>> 'multicast-overflow': 'bool', > >>> 'unicast-overflow': 'bool', > >>> 'main-mac': 'str', > >>> - 'vlan-table': ['int'], > >>> + '*vlan-table': ['int'], > >> > >> Indentation is now off. > >> > >>> - "main-mac": main macaddr string (json-string) > >>> -- "vlan-table": a json-array of active vlan id > >>> +- "vlan-table": a json-array of active vlan id (optoinal) > >> > >> s/optoinal/optional/ > >> > >> Those fixes are trivial enough, so I'm okay if you correct them then add: > > > > Scratch that. I revoke my R-b, on the following grounds: > > > > On 02/17/2014 08:27 AM, Vlad Yasevich wrote: > >> On 02/16/2014 09:27 PM, Amos Kong wrote: > >>> Stefan Fritsch just fixed a virtio-net driver bug [1], virtio-net won't > >>> filter out VLAN-tagged packets if VIRTIO_NET_F_CTRL_VLAN isn't > > negotiated. > >>> > >>> We should also not send the vlan table to management, this patch makes > >>> the vlan-talbe optional. > >> ^^^^^^^^^^ > >> table. > >> > >> One question I have from the API perspective is can we suddenly change > >> something to be optional? If there are any users of this , > >> wouldn't they have to change now to check the existence of this > >> list? > > > > You are correct. Since the parameter is an output field, older clients > > may be depending on it existing. It is okay to generate an empty array, > > but you must not entirely omit the array unless you add further > > justification in your commit message that you are 100% positive that > > there are no clients of 1.6 that will be broken when the array no longer > > appears in the output. > > > > Can you rework the patch to just leave the array empty in the case where > > the bit does not indicate it is used? Or do we need to add a new bool > > field to the output for new enough management to know whether to use the > > array? > > > > I think a completely empty array should be sufficient. > > -vlad An empty array could mean either no vlans allowed or all vlans allowed. Also it's nice if users can detect an old buggy qemu. Let's just add an rx state.
On 02/18/2014 05:06 AM, Michael S. Tsirkin wrote: > On Mon, Feb 17, 2014 at 11:58:11AM -0500, Vlad Yasevich wrote: >> On 02/17/2014 11:56 AM, Eric Blake wrote: >>> On 02/17/2014 09:52 AM, Eric Blake wrote: >>>> On 02/16/2014 07:27 PM, Amos Kong wrote: >>>>> Stefan Fritsch just fixed a virtio-net driver bug [1], virtio-net won't >>>>> filter out VLAN-tagged packets if VIRTIO_NET_F_CTRL_VLAN isn't negotiated. >>>>> >>>>> We should also not send the vlan table to management, this patch makes >>>>> the vlan-talbe optional. >>>> >>>> s/talbe/table/ >>>> >>> >>>>> @@ -4053,7 +4053,7 @@ >>>>> 'multicast-overflow': 'bool', >>>>> 'unicast-overflow': 'bool', >>>>> 'main-mac': 'str', >>>>> - 'vlan-table': ['int'], >>>>> + '*vlan-table': ['int'], >>>> >>>> Indentation is now off. >>>> >>>>> - "main-mac": main macaddr string (json-string) >>>>> -- "vlan-table": a json-array of active vlan id >>>>> +- "vlan-table": a json-array of active vlan id (optoinal) >>>> >>>> s/optoinal/optional/ >>>> >>>> Those fixes are trivial enough, so I'm okay if you correct them then add: >>> >>> Scratch that. I revoke my R-b, on the following grounds: >>> >>> On 02/17/2014 08:27 AM, Vlad Yasevich wrote: >>>> On 02/16/2014 09:27 PM, Amos Kong wrote: >>>>> Stefan Fritsch just fixed a virtio-net driver bug [1], virtio-net won't >>>>> filter out VLAN-tagged packets if VIRTIO_NET_F_CTRL_VLAN isn't >>> negotiated. >>>>> >>>>> We should also not send the vlan table to management, this patch makes >>>>> the vlan-talbe optional. >>>> ^^^^^^^^^^ >>>> table. >>>> >>>> One question I have from the API perspective is can we suddenly change >>>> something to be optional? If there are any users of this , >>>> wouldn't they have to change now to check the existence of this >>>> list? >>> >>> You are correct. Since the parameter is an output field, older clients >>> may be depending on it existing. It is okay to generate an empty array, >>> but you must not entirely omit the array unless you add further >>> justification in your commit message that you are 100% positive that >>> there are no clients of 1.6 that will be broken when the array no longer >>> appears in the output. >>> >>> Can you rework the patch to just leave the array empty in the case where >>> the bit does not indicate it is used? Or do we need to add a new bool >>> field to the output for new enough management to know whether to use the >>> array? >>> >> >> I think a completely empty array should be sufficient. >> >> -vlad > > An empty array could mean either no vlans allowed or > all vlans allowed. Also it's nice if users can detect an old > buggy qemu. > > Let's just add an rx state. > Fine with me. -vlad
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index 3626608..0b32e6a 100644 --- a/hw/net/virtio-net.c +++ b/hw/net/virtio-net.c @@ -222,13 +222,33 @@ static char *mac_strdup_printf(const uint8_t *mac) mac[1], mac[2], mac[3], mac[4], mac[5]); } +static intList *get_vlan_table(VirtIONet *n) +{ + intList *list, *entry; + int i, j; + + 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)) { + entry = g_malloc0(sizeof(*entry)); + entry->value = (i << 5) + j; + entry->next = list; + list = entry; + } + } + } + + return list; +} + static RxFilterInfo *virtio_net_query_rxfilter(NetClientState *nc) { VirtIONet *n = qemu_get_nic_opaque(nc); + VirtIODevice *vdev = VIRTIO_DEVICE(n); RxFilterInfo *info; strList *str_list, *entry; - intList *int_list, *int_entry; - int i, j; + int i; info = g_malloc0(sizeof(*info)); info->name = g_strdup(nc->name); @@ -274,18 +294,10 @@ 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)) { - int_entry = g_malloc0(sizeof(*int_entry)); - int_entry->value = (i << 5) + j; - int_entry->next = int_list; - int_list = int_entry; - } - } + if ((1 << VIRTIO_NET_F_CTRL_VLAN) & vdev->guest_features) { + info->has_vlan_table = true; + info->vlan_table = get_vlan_table(n); } - info->vlan_table = int_list; /* enable event notification after query */ nc->rxfilter_notify_enabled = 1; diff --git a/qapi-schema.json b/qapi-schema.json index 7cfb5e5..5d48fa9 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -4034,7 +4034,7 @@ # # @main-mac: the main macaddr string # -# @vlan-table: a list of active vlan id +# @vlan-table: #optional a list of active vlan id # # @unicast-table: a list of unicast macaddr string # @@ -4053,7 +4053,7 @@ 'multicast-overflow': 'bool', 'unicast-overflow': 'bool', 'main-mac': 'str', - 'vlan-table': ['int'], + '*vlan-table': ['int'], 'unicast-table': ['str'], 'multicast-table': ['str'] }} diff --git a/qmp-commands.hx b/qmp-commands.hx index cce6b81..a1c1dfa 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -3308,7 +3308,7 @@ Each array entry contains the following: - "multicast-overflow": multicast table is overflowed (json-bool) - "unicast-overflow": unicast table is overflowed (json-bool) - "main-mac": main macaddr string (json-string) -- "vlan-table": a json-array of active vlan id +- "vlan-table": a json-array of active vlan id (optoinal) - "unicast-table": a json-array of unicast macaddr string - "multicast-table": a json-array of multicast macaddr string
Stefan Fritsch just fixed a virtio-net driver bug [1], virtio-net won't filter out VLAN-tagged packets if VIRTIO_NET_F_CTRL_VLAN isn't negotiated. We should also not send the vlan table to management, this patch makes the vlan-talbe optional. [1] http://lists.nongnu.org/archive/html/qemu-devel/2014-02/msg02604.html Signed-off-by: Amos Kong <akong@redhat.com> --- hw/net/virtio-net.c | 38 +++++++++++++++++++++++++------------- qapi-schema.json | 4 ++-- qmp-commands.hx | 2 +- 3 files changed, 28 insertions(+), 16 deletions(-)