| Message ID | 1338787767-3443-7-git-send-email-zwu.kernel@gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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
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
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 >
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 >
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
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.
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
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
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.
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? */