Patchwork [v4,06/16] net: Remove vlan qdev property

login
register
mail settings
Submitter Zhiyong Wu
Date June 4, 2012, 5:29 a.m.
Message ID <1338787767-3443-7-git-send-email-zwu.kernel@gmail.com>
Download mbox | patch
Permalink /patch/162645/
State New
Headers show

Comments

Zhiyong Wu - June 4, 2012, 5:29 a.m.
From: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

The vlan feature is implemented using hubs and no longer uses
special-purpose VLANState structs that are accessible as qdev
properties.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
---
 hw/qdev-properties.c |   72 --------------------------------------------------
 hw/qdev.c            |    2 -
 hw/qdev.h            |    4 ---
 net.h                |    3 --
 4 files changed, 0 insertions(+), 81 deletions(-)
Stefan Hajnoczi - June 8, 2012, 1:23 p.m.
On Mon, Jun 4, 2012 at 6:29 AM,  <zwu.kernel@gmail.com> wrote:
> From: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
>
> The vlan feature is implemented using hubs and no longer uses
> special-purpose VLANState structs that are accessible as qdev
> properties.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
> ---
>  hw/qdev-properties.c |   72 --------------------------------------------------
>  hw/qdev.c            |    2 -
>  hw/qdev.h            |    4 ---
>  net.h                |    3 --
>  4 files changed, 0 insertions(+), 81 deletions(-)

This commit looks suspicious because it removes a user-visible qdev
property but we're trying to preserve backward compatibility.  This
command-line will break:

x86_64-softmmu/qemu-system-x86_64 -net user,vlan=1 -device virtio-net-pci,vlan=1

Instead of dropping the qdev_prop_vlan completely the
hw/qdev-properties.c code needs to call net/hub.h external functions
to implement equivalent functionality:

1. Setting the vlan=<id> property looks up the hub port and assigns
the NICConf->peer field.
2. Getting the vlan property looks up the hub id (i.e. vlan id) given
the peer.  If the peer is not a hub port the result is -1.

When I wrote this patch I missed the big picture and forgot about
backwards compatibility :(.

Do you feel comfortable rewriting this commit?

Stefan
Zhiyong Wu - June 8, 2012, 2:48 p.m.
On Fri, Jun 8, 2012 at 9:23 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Mon, Jun 4, 2012 at 6:29 AM,  <zwu.kernel@gmail.com> wrote:
>> From: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
>>
>> The vlan feature is implemented using hubs and no longer uses
>> special-purpose VLANState structs that are accessible as qdev
>> properties.
>>
>> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
>> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>> ---
>>  hw/qdev-properties.c |   72 --------------------------------------------------
>>  hw/qdev.c            |    2 -
>>  hw/qdev.h            |    4 ---
>>  net.h                |    3 --
>>  4 files changed, 0 insertions(+), 81 deletions(-)
>
> This commit looks suspicious because it removes a user-visible qdev
> property but we're trying to preserve backward compatibility.  This
> command-line will break:
>
> x86_64-softmmu/qemu-system-x86_64 -net user,vlan=1 -device virtio-net-pci,vlan=1
Should this type of syntax be supported? i know this at the first time
>
> Instead of dropping the qdev_prop_vlan completely the
> hw/qdev-properties.c code needs to call net/hub.h external functions
> to implement equivalent functionality:
>
> 1. Setting the vlan=<id> property looks up the hub port and assigns
> the NICConf->peer field.
> 2. Getting the vlan property looks up the hub id (i.e. vlan id) given
> the peer.  If the peer is not a hub port the result is -1.
>
> When I wrote this patch I missed the big picture and forgot about
> backwards compatibility :(.
>
> Do you feel comfortable rewriting this commit?
Do you mean that you would like to rewrite this by yourself?
>
> Stefan
Stefan Hajnoczi - June 8, 2012, 4:09 p.m.
On Fri, Jun 8, 2012 at 3:48 PM, Zhi Yong Wu <zwu.kernel@gmail.com> wrote:
> On Fri, Jun 8, 2012 at 9:23 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> On Mon, Jun 4, 2012 at 6:29 AM,  <zwu.kernel@gmail.com> wrote:
>>> From: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
>>>
>>> The vlan feature is implemented using hubs and no longer uses
>>> special-purpose VLANState structs that are accessible as qdev
>>> properties.
>>>
>>> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
>>> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>>> ---
>>>  hw/qdev-properties.c |   72 --------------------------------------------------
>>>  hw/qdev.c            |    2 -
>>>  hw/qdev.h            |    4 ---
>>>  net.h                |    3 --
>>>  4 files changed, 0 insertions(+), 81 deletions(-)
>>
>> This commit looks suspicious because it removes a user-visible qdev
>> property but we're trying to preserve backward compatibility.  This
>> command-line will break:
>>
>> x86_64-softmmu/qemu-system-x86_64 -net user,vlan=1 -device virtio-net-pci,vlan=1
> Should this type of syntax be supported? i know this at the first time
>>
>> Instead of dropping the qdev_prop_vlan completely the
>> hw/qdev-properties.c code needs to call net/hub.h external functions
>> to implement equivalent functionality:
>>
>> 1. Setting the vlan=<id> property looks up the hub port and assigns
>> the NICConf->peer field.
>> 2. Getting the vlan property looks up the hub id (i.e. vlan id) given
>> the peer.  If the peer is not a hub port the result is -1.
>>
>> When I wrote this patch I missed the big picture and forgot about
>> backwards compatibility :(.
>>
>> Do you feel comfortable rewriting this commit?
> Do you mean that you would like to rewrite this by yourself?

No, I meant do you agree with the changes that I suggested?  I wanted
to make sure that you understand the problem that I'm describing and
how it could be solved.

Stefan
Zhiyong Wu - June 9, 2012, 3:04 a.m.
On Sat, Jun 9, 2012 at 12:09 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Fri, Jun 8, 2012 at 3:48 PM, Zhi Yong Wu <zwu.kernel@gmail.com> wrote:
>> On Fri, Jun 8, 2012 at 9:23 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>>> On Mon, Jun 4, 2012 at 6:29 AM,  <zwu.kernel@gmail.com> wrote:
>>>> From: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
>>>>
>>>> The vlan feature is implemented using hubs and no longer uses
>>>> special-purpose VLANState structs that are accessible as qdev
>>>> properties.
>>>>
>>>> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
>>>> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>>>> ---
>>>>  hw/qdev-properties.c |   72 --------------------------------------------------
>>>>  hw/qdev.c            |    2 -
>>>>  hw/qdev.h            |    4 ---
>>>>  net.h                |    3 --
>>>>  4 files changed, 0 insertions(+), 81 deletions(-)
>>>
>>> This commit looks suspicious because it removes a user-visible qdev
>>> property but we're trying to preserve backward compatibility.  This
>>> command-line will break:
>>>
>>> x86_64-softmmu/qemu-system-x86_64 -net user,vlan=1 -device virtio-net-pci,vlan=1
>> Should this type of syntax be supported? i know this at the first time
>>>
>>> Instead of dropping the qdev_prop_vlan completely the
>>> hw/qdev-properties.c code needs to call net/hub.h external functions
>>> to implement equivalent functionality:
>>>
>>> 1. Setting the vlan=<id> property looks up the hub port and assigns
>>> the NICConf->peer field.
>>> 2. Getting the vlan property looks up the hub id (i.e. vlan id) given
>>> the peer.  If the peer is not a hub port the result is -1.
>>>
>>> When I wrote this patch I missed the big picture and forgot about
>>> backwards compatibility :(.
>>>
>>> Do you feel comfortable rewriting this commit?
>> Do you mean that you would like to rewrite this by yourself?
>
> No, I meant do you agree with the changes that I suggested?  I wanted
> to make sure that you understand the problem that I'm describing and
> how it could be solved.
To be honest, i am concerned if anyone uses this syntax. Since the
feature will finally be discarded, i suggest that we don't support
this now. If someone complains this later, we can fix it. If nobody
complains, that is what we hope.
>
> Stefan
Paolo Bonzini - June 11, 2012, 6:28 a.m.
Il 09/06/2012 05:04, Zhi Yong Wu ha scritto:
>>>> This commit looks suspicious because it removes a user-visible qdev
>>>> property but we're trying to preserve backward compatibility.  This
>>>> command-line will break:
>>>>
>>>> x86_64-softmmu/qemu-system-x86_64 -net user,vlan=1 -device virtio-net-pci,vlan=1
>>>>
>>>> Instead of dropping the qdev_prop_vlan completely the
>>>> hw/qdev-properties.c code needs to call net/hub.h external functions
>>>> to implement equivalent functionality:
>>>>
>>>> 1. Setting the vlan=<id> property looks up the hub port and assigns
>>>> the NICConf->peer field.
>>>> 2. Getting the vlan property looks up the hub id (i.e. vlan id) given
>>>> the peer.  If the peer is not a hub port the result is -1.
>>>>
>>>> When I wrote this patch I missed the big picture and forgot about
>>>> backwards compatibility :(.
>>>>
> To be honest, i am concerned if anyone uses this syntax. Since the
> feature will finally be discarded, i suggest that we don't support
> this now. If someone complains this later, we can fix it. If nobody
> complains, that is what we hope.

I think you're missing the big picture of this series, which is exactly
_not_ to discard the VLAN feature, but just to rewrite it in a better way.

That said, I agree that this is a somewhat fringe usage; most people
will use -net nic,model=virtio,vlan=1 rather than "-device".  We may get
by with dropping it.  I have no strong opinion either way.

Paolo
Stefan Hajnoczi - June 11, 2012, 8:57 a.m.
On Mon, Jun 11, 2012 at 08:28:57AM +0200, Paolo Bonzini wrote:
> Il 09/06/2012 05:04, Zhi Yong Wu ha scritto:
> >>>> This commit looks suspicious because it removes a user-visible qdev
> >>>> property but we're trying to preserve backward compatibility.  This
> >>>> command-line will break:
> >>>>
> >>>> x86_64-softmmu/qemu-system-x86_64 -net user,vlan=1 -device virtio-net-pci,vlan=1
> >>>>
> >>>> Instead of dropping the qdev_prop_vlan completely the
> >>>> hw/qdev-properties.c code needs to call net/hub.h external functions
> >>>> to implement equivalent functionality:
> >>>>
> >>>> 1. Setting the vlan=<id> property looks up the hub port and assigns
> >>>> the NICConf->peer field.
> >>>> 2. Getting the vlan property looks up the hub id (i.e. vlan id) given
> >>>> the peer.  If the peer is not a hub port the result is -1.
> >>>>
> >>>> When I wrote this patch I missed the big picture and forgot about
> >>>> backwards compatibility :(.
> >>>>
> > To be honest, i am concerned if anyone uses this syntax. Since the
> > feature will finally be discarded, i suggest that we don't support
> > this now. If someone complains this later, we can fix it. If nobody
> > complains, that is what we hope.
> 
> I think you're missing the big picture of this series, which is exactly
> _not_ to discard the VLAN feature, but just to rewrite it in a better way.
> 
> That said, I agree that this is a somewhat fringe usage; most people
> will use -net nic,model=virtio,vlan=1 rather than "-device".  We may get
> by with dropping it.  I have no strong opinion either way.

Either we keep backwards compatibility or we don't.  Taking a middle
path where we preserve only some of the "VLAN" syntax is confusing and
inconsistent.

Like Paolo said, the point here is not to drop "VLAN" support.  We're
simply moving this feature into a regular net client (the hub) so that
the special case "VLAN" code in net.c can be removed.

Stefan
Zhiyong Wu - June 11, 2012, 2:15 p.m.
On Mon, Jun 11, 2012 at 2:28 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 09/06/2012 05:04, Zhi Yong Wu ha scritto:
>>>>> This commit looks suspicious because it removes a user-visible qdev
>>>>> property but we're trying to preserve backward compatibility.  This
>>>>> command-line will break:
>>>>>
>>>>> x86_64-softmmu/qemu-system-x86_64 -net user,vlan=1 -device virtio-net-pci,vlan=1
>>>>>
>>>>> Instead of dropping the qdev_prop_vlan completely the
>>>>> hw/qdev-properties.c code needs to call net/hub.h external functions
>>>>> to implement equivalent functionality:
>>>>>
>>>>> 1. Setting the vlan=<id> property looks up the hub port and assigns
>>>>> the NICConf->peer field.
>>>>> 2. Getting the vlan property looks up the hub id (i.e. vlan id) given
>>>>> the peer.  If the peer is not a hub port the result is -1.
>>>>>
>>>>> When I wrote this patch I missed the big picture and forgot about
>>>>> backwards compatibility :(.
>>>>>
>> To be honest, i am concerned if anyone uses this syntax. Since the
>> feature will finally be discarded, i suggest that we don't support
>> this now. If someone complains this later, we can fix it. If nobody
>> complains, that is what we hope.
>
> I think you're missing the big picture of this series, which is exactly
> _not_ to discard the VLAN feature, but just to rewrite it in a better way.
Yeah, i know that this series are rewriting VLAN feature in one better way.

What i mean was that luiz and other some guys think that the -net
syntax should be completely removed.

>
> That said, I agree that this is a somewhat fringe usage; most people
> will use -net nic,model=virtio,vlan=1 rather than "-device".  We may get
Yes.
> by with dropping it.  I have no strong opinion either way.
>
> Paolo
>
Zhiyong Wu - June 11, 2012, 2:24 p.m.
On Mon, Jun 11, 2012 at 4:57 PM, Stefan Hajnoczi
<stefanha@linux.vnet.ibm.com> wrote:
> On Mon, Jun 11, 2012 at 08:28:57AM +0200, Paolo Bonzini wrote:
>> Il 09/06/2012 05:04, Zhi Yong Wu ha scritto:
>> >>>> This commit looks suspicious because it removes a user-visible qdev
>> >>>> property but we're trying to preserve backward compatibility.  This
>> >>>> command-line will break:
>> >>>>
>> >>>> x86_64-softmmu/qemu-system-x86_64 -net user,vlan=1 -device virtio-net-pci,vlan=1
>> >>>>
>> >>>> Instead of dropping the qdev_prop_vlan completely the
>> >>>> hw/qdev-properties.c code needs to call net/hub.h external functions
>> >>>> to implement equivalent functionality:
>> >>>>
>> >>>> 1. Setting the vlan=<id> property looks up the hub port and assigns
>> >>>> the NICConf->peer field.
>> >>>> 2. Getting the vlan property looks up the hub id (i.e. vlan id) given
>> >>>> the peer.  If the peer is not a hub port the result is -1.
>> >>>>
>> >>>> When I wrote this patch I missed the big picture and forgot about
>> >>>> backwards compatibility :(.
>> >>>>
>> > To be honest, i am concerned if anyone uses this syntax. Since the
>> > feature will finally be discarded, i suggest that we don't support
>> > this now. If someone complains this later, we can fix it. If nobody
>> > complains, that is what we hope.
>>
>> I think you're missing the big picture of this series, which is exactly
>> _not_ to discard the VLAN feature, but just to rewrite it in a better way.
>>
>> That said, I agree that this is a somewhat fringe usage; most people
>> will use -net nic,model=virtio,vlan=1 rather than "-device".  We may get
>> by with dropping it.  I have no strong opinion either way.
>
> Either we keep backwards compatibility or we don't.  Taking a middle
> path where we preserve only some of the "VLAN" syntax is confusing and
> inconsistent.
in terms of technology, i fully agree with you, but in terms of
usefulness and our business, it will waste our effort, time and is
meaningless if nobody or no customers use this syntax. As i said, if
someone complain this later, we can fix it.

>
> Like Paolo said, the point here is not to drop "VLAN" support.  We're
> simply moving this feature into a regular net client (the hub) so that
> the special case "VLAN" code in net.c can be removed.
>
> Stefan
>
Stefan Hajnoczi - June 11, 2012, 2:42 p.m.
On Mon, Jun 11, 2012 at 3:24 PM, Zhi Yong Wu <zwu.kernel@gmail.com> wrote:
> On Mon, Jun 11, 2012 at 4:57 PM, Stefan Hajnoczi
> <stefanha@linux.vnet.ibm.com> wrote:
>> On Mon, Jun 11, 2012 at 08:28:57AM +0200, Paolo Bonzini wrote:
>>> Il 09/06/2012 05:04, Zhi Yong Wu ha scritto:
>>> >>>> This commit looks suspicious because it removes a user-visible qdev
>>> >>>> property but we're trying to preserve backward compatibility.  This
>>> >>>> command-line will break:
>>> >>>>
>>> >>>> x86_64-softmmu/qemu-system-x86_64 -net user,vlan=1 -device virtio-net-pci,vlan=1
>>> >>>>
>>> >>>> Instead of dropping the qdev_prop_vlan completely the
>>> >>>> hw/qdev-properties.c code needs to call net/hub.h external functions
>>> >>>> to implement equivalent functionality:
>>> >>>>
>>> >>>> 1. Setting the vlan=<id> property looks up the hub port and assigns
>>> >>>> the NICConf->peer field.
>>> >>>> 2. Getting the vlan property looks up the hub id (i.e. vlan id) given
>>> >>>> the peer.  If the peer is not a hub port the result is -1.
>>> >>>>
>>> >>>> When I wrote this patch I missed the big picture and forgot about
>>> >>>> backwards compatibility :(.
>>> >>>>
>>> > To be honest, i am concerned if anyone uses this syntax. Since the
>>> > feature will finally be discarded, i suggest that we don't support
>>> > this now. If someone complains this later, we can fix it. If nobody
>>> > complains, that is what we hope.
>>>
>>> I think you're missing the big picture of this series, which is exactly
>>> _not_ to discard the VLAN feature, but just to rewrite it in a better way.
>>>
>>> That said, I agree that this is a somewhat fringe usage; most people
>>> will use -net nic,model=virtio,vlan=1 rather than "-device".  We may get
>>> by with dropping it.  I have no strong opinion either way.
>>
>> Either we keep backwards compatibility or we don't.  Taking a middle
>> path where we preserve only some of the "VLAN" syntax is confusing and
>> inconsistent.
> in terms of technology, i fully agree with you, but in terms of
> usefulness and our business, it will waste our effort, time and is
> meaningless if nobody or no customers use this syntax. As i said, if
> someone complain this later, we can fix it.

When users upgrade QEMU versions and find their setup is now broken
QEMU's reputation will be damaged.  You can't build critical systems
on top of software which keeps changing and breaking.  Fixing it after
a user hits the problem is not okay, users won't trust us if we do
that.

We need to be disciplined when it comes to backwards compatibility.
Either we support the "VLAN" feature or we drop it.  We already had
this discussion in another thread, here's what Anthony had to say:

"Dropping features is only something that should be approached lightly and
certainly not something that should be done just because you don't like a
particular bit of code."

http://permalink.gmane.org/gmane.comp.emulators.qemu/153600

Stefan
Luiz Capitulino - June 11, 2012, 8:05 p.m.
On Mon, 11 Jun 2012 22:15:55 +0800
Zhi Yong Wu <zwu.kernel@gmail.com> wrote:

> On Mon, Jun 11, 2012 at 2:28 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > Il 09/06/2012 05:04, Zhi Yong Wu ha scritto:
> >>>>> This commit looks suspicious because it removes a user-visible qdev
> >>>>> property but we're trying to preserve backward compatibility.  This
> >>>>> command-line will break:
> >>>>>
> >>>>> x86_64-softmmu/qemu-system-x86_64 -net user,vlan=1 -device virtio-net-pci,vlan=1
> >>>>>
> >>>>> Instead of dropping the qdev_prop_vlan completely the
> >>>>> hw/qdev-properties.c code needs to call net/hub.h external functions
> >>>>> to implement equivalent functionality:
> >>>>>
> >>>>> 1. Setting the vlan=<id> property looks up the hub port and assigns
> >>>>> the NICConf->peer field.
> >>>>> 2. Getting the vlan property looks up the hub id (i.e. vlan id) given
> >>>>> the peer.  If the peer is not a hub port the result is -1.
> >>>>>
> >>>>> When I wrote this patch I missed the big picture and forgot about
> >>>>> backwards compatibility :(.
> >>>>>
> >> To be honest, i am concerned if anyone uses this syntax. Since the
> >> feature will finally be discarded, i suggest that we don't support
> >> this now. If someone complains this later, we can fix it. If nobody
> >> complains, that is what we hope.
> >
> > I think you're missing the big picture of this series, which is exactly
> > _not_ to discard the VLAN feature, but just to rewrite it in a better way.
> Yeah, i know that this series are rewriting VLAN feature in one better way.
> 
> What i mean was that luiz and other some guys think that the -net
> syntax should be completely removed.

One of the motivations for having the hub feature is to preserve the vlan
functionality, in that case we shouldn't break cmd-line backwards compatibility.
Anthony Liguori - June 11, 2012, 8:49 p.m.
On 06/11/2012 01:28 AM, Paolo Bonzini wrote:
> Il 09/06/2012 05:04, Zhi Yong Wu ha scritto:
>>>>> This commit looks suspicious because it removes a user-visible qdev
>>>>> property but we're trying to preserve backward compatibility.  This
>>>>> command-line will break:
>>>>>
>>>>> x86_64-softmmu/qemu-system-x86_64 -net user,vlan=1 -device virtio-net-pci,vlan=1
>>>>>
>>>>> Instead of dropping the qdev_prop_vlan completely the
>>>>> hw/qdev-properties.c code needs to call net/hub.h external functions
>>>>> to implement equivalent functionality:
>>>>>
>>>>> 1. Setting the vlan=<id>  property looks up the hub port and assigns
>>>>> the NICConf->peer field.
>>>>> 2. Getting the vlan property looks up the hub id (i.e. vlan id) given
>>>>> the peer.  If the peer is not a hub port the result is -1.
>>>>>
>>>>> When I wrote this patch I missed the big picture and forgot about
>>>>> backwards compatibility :(.
>>>>>
>> To be honest, i am concerned if anyone uses this syntax. Since the
>> feature will finally be discarded, i suggest that we don't support
>> this now. If someone complains this later, we can fix it. If nobody
>> complains, that is what we hope.
>
> I think you're missing the big picture of this series, which is exactly
> _not_ to discard the VLAN feature, but just to rewrite it in a better way.
>
> That said, I agree that this is a somewhat fringe usage; most people
> will use -net nic,model=virtio,vlan=1 rather than "-device".  We may get
> by with dropping it.  I have no strong opinion either way.

I have a strong opinion :-)

We shipped a version with it, so now we have to support it.

Regards,

Anthony Liguori
Zhiyong Wu - June 12, 2012, 12:54 a.m.
On Mon, Jun 11, 2012 at 10:42 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Mon, Jun 11, 2012 at 3:24 PM, Zhi Yong Wu <zwu.kernel@gmail.com> wrote:
>> On Mon, Jun 11, 2012 at 4:57 PM, Stefan Hajnoczi
>> <stefanha@linux.vnet.ibm.com> wrote:
>>> On Mon, Jun 11, 2012 at 08:28:57AM +0200, Paolo Bonzini wrote:
>>>> Il 09/06/2012 05:04, Zhi Yong Wu ha scritto:
>>>> >>>> This commit looks suspicious because it removes a user-visible qdev
>>>> >>>> property but we're trying to preserve backward compatibility.  This
>>>> >>>> command-line will break:
>>>> >>>>
>>>> >>>> x86_64-softmmu/qemu-system-x86_64 -net user,vlan=1 -device virtio-net-pci,vlan=1
>>>> >>>>
>>>> >>>> Instead of dropping the qdev_prop_vlan completely the
>>>> >>>> hw/qdev-properties.c code needs to call net/hub.h external functions
>>>> >>>> to implement equivalent functionality:
>>>> >>>>
>>>> >>>> 1. Setting the vlan=<id> property looks up the hub port and assigns
>>>> >>>> the NICConf->peer field.
>>>> >>>> 2. Getting the vlan property looks up the hub id (i.e. vlan id) given
>>>> >>>> the peer.  If the peer is not a hub port the result is -1.
>>>> >>>>
>>>> >>>> When I wrote this patch I missed the big picture and forgot about
>>>> >>>> backwards compatibility :(.
>>>> >>>>
>>>> > To be honest, i am concerned if anyone uses this syntax. Since the
>>>> > feature will finally be discarded, i suggest that we don't support
>>>> > this now. If someone complains this later, we can fix it. If nobody
>>>> > complains, that is what we hope.
>>>>
>>>> I think you're missing the big picture of this series, which is exactly
>>>> _not_ to discard the VLAN feature, but just to rewrite it in a better way.
>>>>
>>>> That said, I agree that this is a somewhat fringe usage; most people
>>>> will use -net nic,model=virtio,vlan=1 rather than "-device".  We may get
>>>> by with dropping it.  I have no strong opinion either way.
>>>
>>> Either we keep backwards compatibility or we don't.  Taking a middle
>>> path where we preserve only some of the "VLAN" syntax is confusing and
>>> inconsistent.
>> in terms of technology, i fully agree with you, but in terms of
>> usefulness and our business, it will waste our effort, time and is
>> meaningless if nobody or no customers use this syntax. As i said, if
>> someone complain this later, we can fix it.
>
> When users upgrade QEMU versions and find their setup is now broken
> QEMU's reputation will be damaged.  You can't build critical systems
> on top of software which keeps changing and breaking.  Fixing it after
> a user hits the problem is not okay, users won't trust us if we do
> that.
OK, i will try to work on this.
>
> We need to be disciplined when it comes to backwards compatibility.
> Either we support the "VLAN" feature or we drop it.  We already had
> this discussion in another thread, here's what Anthony had to say:
>
> "Dropping features is only something that should be approached lightly and
> certainly not something that should be done just because you don't like a
> particular bit of code."
>
> http://permalink.gmane.org/gmane.comp.emulators.qemu/153600
>
> Stefan
Zhiyong Wu - June 12, 2012, 12:55 a.m.
On Tue, Jun 12, 2012 at 4:05 AM, Luiz Capitulino <lcapitulino@redhat.com> wrote:
> On Mon, 11 Jun 2012 22:15:55 +0800
> Zhi Yong Wu <zwu.kernel@gmail.com> wrote:
>
>> On Mon, Jun 11, 2012 at 2:28 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> > Il 09/06/2012 05:04, Zhi Yong Wu ha scritto:
>> >>>>> This commit looks suspicious because it removes a user-visible qdev
>> >>>>> property but we're trying to preserve backward compatibility.  This
>> >>>>> command-line will break:
>> >>>>>
>> >>>>> x86_64-softmmu/qemu-system-x86_64 -net user,vlan=1 -device virtio-net-pci,vlan=1
>> >>>>>
>> >>>>> Instead of dropping the qdev_prop_vlan completely the
>> >>>>> hw/qdev-properties.c code needs to call net/hub.h external functions
>> >>>>> to implement equivalent functionality:
>> >>>>>
>> >>>>> 1. Setting the vlan=<id> property looks up the hub port and assigns
>> >>>>> the NICConf->peer field.
>> >>>>> 2. Getting the vlan property looks up the hub id (i.e. vlan id) given
>> >>>>> the peer.  If the peer is not a hub port the result is -1.
>> >>>>>
>> >>>>> When I wrote this patch I missed the big picture and forgot about
>> >>>>> backwards compatibility :(.
>> >>>>>
>> >> To be honest, i am concerned if anyone uses this syntax. Since the
>> >> feature will finally be discarded, i suggest that we don't support
>> >> this now. If someone complains this later, we can fix it. If nobody
>> >> complains, that is what we hope.
>> >
>> > I think you're missing the big picture of this series, which is exactly
>> > _not_ to discard the VLAN feature, but just to rewrite it in a better way.
>> Yeah, i know that this series are rewriting VLAN feature in one better way.
>>
>> What i mean was that luiz and other some guys think that the -net
>> syntax should be completely removed.
>
> One of the motivations for having the hub feature is to preserve the vlan
> functionality, in that case we shouldn't break cmd-line backwards compatibility.
 OK, thanks.

Patch

diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index b7b5597..d2e2afb 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -623,71 +623,6 @@  PropertyInfo qdev_prop_netdev = {
     .set   = set_netdev,
 };
 
-/* --- vlan --- */
-
-static int print_vlan(DeviceState *dev, Property *prop, char *dest, size_t len)
-{
-    VLANState **ptr = qdev_get_prop_ptr(dev, prop);
-
-    if (*ptr) {
-        return snprintf(dest, len, "%d", (*ptr)->id);
-    } else {
-        return snprintf(dest, len, "<null>");
-    }
-}
-
-static void get_vlan(Object *obj, Visitor *v, void *opaque,
-                     const char *name, Error **errp)
-{
-    DeviceState *dev = DEVICE(obj);
-    Property *prop = opaque;
-    VLANState **ptr = qdev_get_prop_ptr(dev, prop);
-    int64_t id;
-
-    id = *ptr ? (*ptr)->id : -1;
-    visit_type_int(v, &id, name, errp);
-}
-
-static void set_vlan(Object *obj, Visitor *v, void *opaque,
-                     const char *name, Error **errp)
-{
-    DeviceState *dev = DEVICE(obj);
-    Property *prop = opaque;
-    VLANState **ptr = qdev_get_prop_ptr(dev, prop);
-    Error *local_err = NULL;
-    int64_t id;
-    VLANState *vlan;
-
-    if (dev->state != DEV_STATE_CREATED) {
-        error_set(errp, QERR_PERMISSION_DENIED);
-        return;
-    }
-
-    visit_type_int(v, &id, name, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        return;
-    }
-    if (id == -1) {
-        *ptr = NULL;
-        return;
-    }
-    vlan = qemu_find_vlan(id, 1);
-    if (!vlan) {
-        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
-                  name, prop->info->name);
-        return;
-    }
-    *ptr = vlan;
-}
-
-PropertyInfo qdev_prop_vlan = {
-    .name  = "vlan",
-    .print = print_vlan,
-    .get   = get_vlan,
-    .set   = set_vlan,
-};
-
 /* --- pointer --- */
 
 /* Not a proper property, just for dirty hacks.  TODO Remove it!  */
@@ -1094,13 +1029,6 @@  void qdev_prop_set_netdev(DeviceState *dev, const char *name, VLANClientState *v
     assert_no_error(errp);
 }
 
-void qdev_prop_set_vlan(DeviceState *dev, const char *name, VLANState *value)
-{
-    Error *errp = NULL;
-    object_property_set_int(OBJECT(dev), value ? value->id : -1, name, &errp);
-    assert_no_error(errp);
-}
-
 void qdev_prop_set_macaddr(DeviceState *dev, const char *name, uint8_t *value)
 {
     Error *errp = NULL;
diff --git a/hw/qdev.c b/hw/qdev.c
index 6a8f6bd..49dd303 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -316,8 +316,6 @@  void qdev_connect_gpio_out(DeviceState * dev, int n, qemu_irq pin)
 void qdev_set_nic_properties(DeviceState *dev, NICInfo *nd)
 {
     qdev_prop_set_macaddr(dev, "mac", nd->macaddr.a);
-    if (nd->vlan)
-        qdev_prop_set_vlan(dev, "vlan", nd->vlan);
     if (nd->netdev)
         qdev_prop_set_netdev(dev, "netdev", nd->netdev);
     if (nd->nvectors != DEV_NVECTORS_UNSPECIFIED &&
diff --git a/hw/qdev.h b/hw/qdev.h
index 4e90119..0a50a40 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -222,7 +222,6 @@  extern PropertyInfo qdev_prop_macaddr;
 extern PropertyInfo qdev_prop_losttickpolicy;
 extern PropertyInfo qdev_prop_drive;
 extern PropertyInfo qdev_prop_netdev;
-extern PropertyInfo qdev_prop_vlan;
 extern PropertyInfo qdev_prop_pci_devfn;
 extern PropertyInfo qdev_prop_blocksize;
 
@@ -277,8 +276,6 @@  extern PropertyInfo qdev_prop_blocksize;
     DEFINE_PROP(_n, _s, _f, qdev_prop_string, char*)
 #define DEFINE_PROP_NETDEV(_n, _s, _f)             \
     DEFINE_PROP(_n, _s, _f, qdev_prop_netdev, VLANClientState*)
-#define DEFINE_PROP_VLAN(_n, _s, _f)             \
-    DEFINE_PROP(_n, _s, _f, qdev_prop_vlan, VLANState*)
 #define DEFINE_PROP_DRIVE(_n, _s, _f) \
     DEFINE_PROP(_n, _s, _f, qdev_prop_drive, BlockDriverState *)
 #define DEFINE_PROP_MACADDR(_n, _s, _f)         \
@@ -305,7 +302,6 @@  void qdev_prop_set_uint64(DeviceState *dev, const char *name, uint64_t value);
 void qdev_prop_set_string(DeviceState *dev, const char *name, char *value);
 void qdev_prop_set_chr(DeviceState *dev, const char *name, CharDriverState *value);
 void qdev_prop_set_netdev(DeviceState *dev, const char *name, VLANClientState *value);
-void qdev_prop_set_vlan(DeviceState *dev, const char *name, VLANState *value);
 int qdev_prop_set_drive(DeviceState *dev, const char *name, BlockDriverState *value) QEMU_WARN_UNUSED_RESULT;
 void qdev_prop_set_drive_nofail(DeviceState *dev, const char *name, BlockDriverState *value);
 void qdev_prop_set_macaddr(DeviceState *dev, const char *name, uint8_t *value);
diff --git a/net.h b/net.h
index d3d6e4c..7d18b10 100644
--- a/net.h
+++ b/net.h
@@ -16,14 +16,12 @@  struct MACAddr {
 
 typedef struct NICConf {
     MACAddr macaddr;
-    VLANState *vlan;
     VLANClientState *peer;
     int32_t bootindex;
 } NICConf;
 
 #define DEFINE_NIC_PROPERTIES(_state, _conf)                            \
     DEFINE_PROP_MACADDR("mac",   _state, _conf.macaddr),                \
-    DEFINE_PROP_VLAN("vlan",     _state, _conf.vlan),                   \
     DEFINE_PROP_NETDEV("netdev", _state, _conf.peer),                   \
     DEFINE_PROP_INT32("bootindex", _state, _conf.bootindex, -1)
 
@@ -134,7 +132,6 @@  struct NICInfo {
     char *model;
     char *name;
     char *devaddr;
-    VLANState *vlan;
     VLANClientState *netdev;
     int used;         /* is this slot in nd_table[] being used? */
     int instantiated; /* does this NICInfo correspond to an instantiated NIC? */