[Qemu-devel] [PATCH] qdev: Catch attempt to attach more than one device to a netdev

[Qemu-devel] [PATCH] qdev: Catch attempt to attach more than one device to a netdev

[Markus Armbruster]

Guest device and host netdev are peers, i.e. it's a 1:1 relation.
However, we fail to enforce that:

    $ qemu -nodefaults --nographic -netdev user,id=net0 -device e1000,netdev=net0 -device virtio-net-pci,netdev=net0 -monitor stdio
    QEMU 0.12.50 monitor - type 'help' for more information
    (qemu) info network
    Devices not on any VLAN:
      net0: net=10.0.2.0, restricted=n peer=virtio-net-pci.0
      e1000.0: model=e1000,macaddr=52:54:00:12:34:56 peer=net0
      virtio-net-pci.0: model=virtio-net-pci,macaddr=52:54:00:12:34:57 peer=net0

It's all downhill from there.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/qdev-properties.c |    3 +++
 net.c                |    1 +
 2 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index 277ff9e..89efd91 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -341,6 +341,9 @@ static int parse_netdev(DeviceState *dev, Property *prop, const char *str)
     *ptr = qemu_find_netdev(str);
     if (*ptr == NULL)
         return -1;
+    if ((*ptr)->peer) {
+        return -1;
+    }
     return 0;
 }
 
diff --git a/net.c b/net.c
index a1bf49f..e6c96d3 100644
--- a/net.c
+++ b/net.c
@@ -245,6 +245,7 @@ VLANClientState *qemu_new_net_client(NetClientInfo *info,
         QTAILQ_INSERT_TAIL(&vc->vlan->clients, vc, next);
     } else {
         if (peer) {
+            assert(!peer->peer);
             vc->peer = peer;
             peer->peer = vc;
         }
-- 
1.6.6

[Qemu-devel] Re: [PATCH] qdev: Catch attempt to attach more than one device to a netdev

[Michael S. Tsirkin]

On Thu, Feb 25, 2010 at 11:10:15AM +0100, Markus Armbruster wrote:
> Guest device and host netdev are peers, i.e. it's a 1:1 relation.
> However, we fail to enforce that:
> 
>     $ qemu -nodefaults --nographic -netdev user,id=net0 -device e1000,netdev=net0 -device virtio-net-pci,netdev=net0 -monitor stdio
>     QEMU 0.12.50 monitor - type 'help' for more information
>     (qemu) info network
>     Devices not on any VLAN:
>       net0: net=10.0.2.0, restricted=n peer=virtio-net-pci.0
>       e1000.0: model=e1000,macaddr=52:54:00:12:34:56 peer=net0
>       virtio-net-pci.0: model=virtio-net-pci,macaddr=52:54:00:12:34:57 peer=net0
> 
> It's all downhill from there.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/qdev-properties.c |    3 +++
>  net.c                |    1 +
>  2 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
> index 277ff9e..89efd91 100644
> --- a/hw/qdev-properties.c
> +++ b/hw/qdev-properties.c
> @@ -341,6 +341,9 @@ static int parse_netdev(DeviceState *dev, Property *prop, const char *str)
>      *ptr = qemu_find_netdev(str);
>      if (*ptr == NULL)
>          return -1;
> +    if ((*ptr)->peer) {
> +        return -1;
> +    }

Not even -EBUSY?
Can this produce a helpful diagnostic message?

>      return 0;
>  }
>  
> diff --git a/net.c b/net.c
> index a1bf49f..e6c96d3 100644
> --- a/net.c
> +++ b/net.c
> @@ -245,6 +245,7 @@ VLANClientState *qemu_new_net_client(NetClientInfo *info,
>          QTAILQ_INSERT_TAIL(&vc->vlan->clients, vc, next);
>      } else {
>          if (peer) {
> +            assert(!peer->peer);

Do we ever get herE?

>              vc->peer = peer;
>              peer->peer = vc;
>          }
> -- 
> 1.6.6
> 
> 

[Qemu-devel] Re: [PATCH] qdev: Catch attempt to attach more than one device to a netdev

[Markus Armbruster]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Thu, Feb 25, 2010 at 11:10:15AM +0100, Markus Armbruster wrote:
>> Guest device and host netdev are peers, i.e. it's a 1:1 relation.
>> However, we fail to enforce that:
>> 
>>     $ qemu -nodefaults --nographic -netdev user,id=net0 -device e1000,netdev=net0 -device virtio-net-pci,netdev=net0 -monitor stdio
>>     QEMU 0.12.50 monitor - type 'help' for more information
>>     (qemu) info network
>>     Devices not on any VLAN:
>>       net0: net=10.0.2.0, restricted=n peer=virtio-net-pci.0
>>       e1000.0: model=e1000,macaddr=52:54:00:12:34:56 peer=net0
>>       virtio-net-pci.0: model=virtio-net-pci,macaddr=52:54:00:12:34:57 peer=net0
>> 
>> It's all downhill from there.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hw/qdev-properties.c |    3 +++
>>  net.c                |    1 +
>>  2 files changed, 4 insertions(+), 0 deletions(-)
>> 
>> diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
>> index 277ff9e..89efd91 100644
>> --- a/hw/qdev-properties.c
>> +++ b/hw/qdev-properties.c
>> @@ -341,6 +341,9 @@ static int parse_netdev(DeviceState *dev, Property *prop, const char *str)
>>      *ptr = qemu_find_netdev(str);
>>      if (*ptr == NULL)
>>          return -1;
>> +    if ((*ptr)->peer) {
>> +        return -1;
>> +    }
>
> Not even -EBUSY?
> Can this produce a helpful diagnostic message?

Callers take care of that.  It's how property parse methods work.
In this case:

    property "virtio-net-pci.netdev": failed to parse "net0"
    can't set property "netdev" to "net0" for "virtio-net-pci"

>>      return 0;
>>  }
>>  
>> diff --git a/net.c b/net.c
>> index a1bf49f..e6c96d3 100644
>> --- a/net.c
>> +++ b/net.c
>> @@ -245,6 +245,7 @@ VLANClientState *qemu_new_net_client(NetClientInfo *info,
>>          QTAILQ_INSERT_TAIL(&vc->vlan->clients, vc, next);
>>      } else {
>>          if (peer) {
>> +            assert(!peer->peer);
>
> Do we ever get herE?

If "get here" means "conditional entered": yes.  For instance, with the
command line from the commit message, we get here for each of the
-device, like this:

#0  qemu_new_net_client (info=0x8b2ce0, vlan=0x0, peer=0xcd83f0, 
    model=0x5ea826 "e1000", name=0x0) at /work/armbru/qemu/net.c:227
#1  0x000000000047682f in qemu_new_nic (info=0x8b2ce0, conf=0x7fffece4a270, 
    model=0x5ea826 "e1000", name=0x0, opaque=0x7fffece4a010)
    at /work/armbru/qemu/net.c:273
#2  0x0000000000447c74 in pci_e1000_init (pci_dev=0x7fffece4a010)
    at /work/armbru/qemu/hw/e1000.c:1125
#3  0x0000000000422335 in pci_qdev_init (qdev=0x7fffece4a010, base=0x8b2dc0)
    at /work/armbru/qemu/hw/pci.c:1647
#4  0x00000000004bbb29 in qdev_init (dev=0x7fffece4a010)
    at /work/armbru/qemu/hw/qdev.c:265

If "get here" means "assertion fails": only if somebody breaks something
elsewhere.  And then assertion will slap his wrist.

>>              vc->peer = peer;
>>              peer->peer = vc;
>>          }
>> -- 
>> 1.6.6
>> 
>> 

[Qemu-devel] Re: [PATCH] qdev: Catch attempt to attach more than one device to a netdev

[Michael S. Tsirkin]

On Thu, Feb 25, 2010 at 01:07:54PM +0100, Markus Armbruster wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Thu, Feb 25, 2010 at 11:10:15AM +0100, Markus Armbruster wrote:
> >> Guest device and host netdev are peers, i.e. it's a 1:1 relation.
> >> However, we fail to enforce that:
> >> 
> >>     $ qemu -nodefaults --nographic -netdev user,id=net0 -device e1000,netdev=net0 -device virtio-net-pci,netdev=net0 -monitor stdio
> >>     QEMU 0.12.50 monitor - type 'help' for more information
> >>     (qemu) info network
> >>     Devices not on any VLAN:
> >>       net0: net=10.0.2.0, restricted=n peer=virtio-net-pci.0
> >>       e1000.0: model=e1000,macaddr=52:54:00:12:34:56 peer=net0
> >>       virtio-net-pci.0: model=virtio-net-pci,macaddr=52:54:00:12:34:57 peer=net0
> >> 
> >> It's all downhill from there.
> >> 
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> ---
> >>  hw/qdev-properties.c |    3 +++
> >>  net.c                |    1 +
> >>  2 files changed, 4 insertions(+), 0 deletions(-)
> >> 
> >> diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
> >> index 277ff9e..89efd91 100644
> >> --- a/hw/qdev-properties.c
> >> +++ b/hw/qdev-properties.c
> >> @@ -341,6 +341,9 @@ static int parse_netdev(DeviceState *dev, Property *prop, const char *str)
> >>      *ptr = qemu_find_netdev(str);
> >>      if (*ptr == NULL)
> >>          return -1;
> >> +    if ((*ptr)->peer) {
> >> +        return -1;
> >> +    }
> >
> > Not even -EBUSY?
> > Can this produce a helpful diagnostic message?
> 
> Callers take care of that.  It's how property parse methods work.
> In this case:
> 
>     property "virtio-net-pci.netdev": failed to parse "net0"
>     can't set property "netdev" to "net0" for "virtio-net-pci"

Yes, but does not tell you why. If parse_netdev even just returned a meaningful
error code, we could see:
    can't set property "netdev" to "net0" for "virtio-net-pci":
    device or resource busy.

which is at least a hint that something else uses it.

As it is, it looks like qemu could not parse "net0"
which is not really right.


> >>      return 0;
> >>  }
> >>  
> >> diff --git a/net.c b/net.c
> >> index a1bf49f..e6c96d3 100644
> >> --- a/net.c
> >> +++ b/net.c
> >> @@ -245,6 +245,7 @@ VLANClientState *qemu_new_net_client(NetClientInfo *info,
> >>          QTAILQ_INSERT_TAIL(&vc->vlan->clients, vc, next);
> >>      } else {
> >>          if (peer) {
> >> +            assert(!peer->peer);
> >
> > Do we ever get herE?
> 
> If "get here" means "conditional entered": yes.  For instance, with the
> command line from the commit message, we get here for each of the
> -device, like this:
> 
> #0  qemu_new_net_client (info=0x8b2ce0, vlan=0x0, peer=0xcd83f0, 
>     model=0x5ea826 "e1000", name=0x0) at /work/armbru/qemu/net.c:227
> #1  0x000000000047682f in qemu_new_nic (info=0x8b2ce0, conf=0x7fffece4a270, 
>     model=0x5ea826 "e1000", name=0x0, opaque=0x7fffece4a010)
>     at /work/armbru/qemu/net.c:273
> #2  0x0000000000447c74 in pci_e1000_init (pci_dev=0x7fffece4a010)
>     at /work/armbru/qemu/hw/e1000.c:1125
> #3  0x0000000000422335 in pci_qdev_init (qdev=0x7fffece4a010, base=0x8b2dc0)
>     at /work/armbru/qemu/hw/pci.c:1647
> #4  0x00000000004bbb29 in qdev_init (dev=0x7fffece4a010)
>     at /work/armbru/qemu/hw/qdev.c:265
> 
> If "get here" means "assertion fails": only if somebody breaks something
> elsewhere.  And then assertion will slap his wrist.

I see.

> >>              vc->peer = peer;
> >>              peer->peer = vc;
> >>          }
> >> -- 
> >> 1.6.6
> >> 
> >> 

[Qemu-devel] Re: [PATCH] qdev: Catch attempt to attach more than one device to a netdev

[Markus Armbruster]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Thu, Feb 25, 2010 at 01:07:54PM +0100, Markus Armbruster wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> 
>> > On Thu, Feb 25, 2010 at 11:10:15AM +0100, Markus Armbruster wrote:
>> >> Guest device and host netdev are peers, i.e. it's a 1:1 relation.
>> >> However, we fail to enforce that:
>> >> 
>> >>     $ qemu -nodefaults --nographic -netdev user,id=net0 -device e1000,netdev=net0 -device virtio-net-pci,netdev=net0 -monitor stdio
>> >>     QEMU 0.12.50 monitor - type 'help' for more information
>> >>     (qemu) info network
>> >>     Devices not on any VLAN:
>> >>       net0: net=10.0.2.0, restricted=n peer=virtio-net-pci.0
>> >>       e1000.0: model=e1000,macaddr=52:54:00:12:34:56 peer=net0
>> >>       virtio-net-pci.0: model=virtio-net-pci,macaddr=52:54:00:12:34:57 peer=net0
>> >> 
>> >> It's all downhill from there.
>> >> 
>> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> >> ---
>> >>  hw/qdev-properties.c |    3 +++
>> >>  net.c                |    1 +
>> >>  2 files changed, 4 insertions(+), 0 deletions(-)
>> >> 
>> >> diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
>> >> index 277ff9e..89efd91 100644
>> >> --- a/hw/qdev-properties.c
>> >> +++ b/hw/qdev-properties.c
>> >> @@ -341,6 +341,9 @@ static int parse_netdev(DeviceState *dev, Property *prop, const char *str)
>> >>      *ptr = qemu_find_netdev(str);
>> >>      if (*ptr == NULL)
>> >>          return -1;
>> >> +    if ((*ptr)->peer) {
>> >> +        return -1;
>> >> +    }
>> >
>> > Not even -EBUSY?
>> > Can this produce a helpful diagnostic message?
>> 
>> Callers take care of that.  It's how property parse methods work.
>> In this case:
>> 
>>     property "virtio-net-pci.netdev": failed to parse "net0"
>>     can't set property "netdev" to "net0" for "virtio-net-pci"
>
> Yes, but does not tell you why. If parse_netdev even just returned a meaningful
> error code, we could see:
>     can't set property "netdev" to "net0" for "virtio-net-pci":
>     device or resource busy.
>
> which is at least a hint that something else uses it.
>
> As it is, it looks like qemu could not parse "net0"
> which is not really right.

Yes, but that's really a separate problem, namely that parse methods
need a way to indicate what went wrong.

[Qemu-devel] Re: [PATCH] qdev: Catch attempt to attach more than one device to a netdev

[Markus Armbruster]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Thu, Feb 25, 2010 at 01:07:54PM +0100, Markus Armbruster wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> 
>> > On Thu, Feb 25, 2010 at 11:10:15AM +0100, Markus Armbruster wrote:
>> >> Guest device and host netdev are peers, i.e. it's a 1:1 relation.
>> >> However, we fail to enforce that:
>> >> 
>> >>     $ qemu -nodefaults --nographic -netdev user,id=net0 -device e1000,netdev=net0 -device virtio-net-pci,netdev=net0 -monitor stdio
>> >>     QEMU 0.12.50 monitor - type 'help' for more information
>> >>     (qemu) info network
>> >>     Devices not on any VLAN:
>> >>       net0: net=10.0.2.0, restricted=n peer=virtio-net-pci.0
>> >>       e1000.0: model=e1000,macaddr=52:54:00:12:34:56 peer=net0
>> >>       virtio-net-pci.0: model=virtio-net-pci,macaddr=52:54:00:12:34:57 peer=net0
>> >> 
>> >> It's all downhill from there.
>> >> 
>> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> >> ---
>> >>  hw/qdev-properties.c |    3 +++
>> >>  net.c                |    1 +
>> >>  2 files changed, 4 insertions(+), 0 deletions(-)
>> >> 
>> >> diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
>> >> index 277ff9e..89efd91 100644
>> >> --- a/hw/qdev-properties.c
>> >> +++ b/hw/qdev-properties.c
>> >> @@ -341,6 +341,9 @@ static int parse_netdev(DeviceState *dev, Property *prop, const char *str)
>> >>      *ptr = qemu_find_netdev(str);
>> >>      if (*ptr == NULL)
>> >>          return -1;
>> >> +    if ((*ptr)->peer) {
>> >> +        return -1;
>> >> +    }
>> >
>> > Not even -EBUSY?
>> > Can this produce a helpful diagnostic message?
>> 
>> Callers take care of that.  It's how property parse methods work.
>> In this case:
>> 
>>     property "virtio-net-pci.netdev": failed to parse "net0"
>>     can't set property "netdev" to "net0" for "virtio-net-pci"
>
> Yes, but does not tell you why. If parse_netdev even just returned a meaningful
> error code, we could see:
>     can't set property "netdev" to "net0" for "virtio-net-pci":
>     device or resource busy.
>
> which is at least a hint that something else uses it.
>
> As it is, it looks like qemu could not parse "net0"
> which is not really right.

Gerd, what do you think about changing the contract for property parse
methods to return -EINVAL, -EBUSY and such?

[Qemu-devel] Re: [PATCH] qdev: Catch attempt to attach more than one device to a netdev

[Gerd Hoffmann]

   Hi,

> Gerd, what do you think about changing the contract for property parse
> methods to return -EINVAL, -EBUSY and such?

Hmm, wouldn't it make more sense to integrate with qerror somehow?

cheers,
   Gerd

[Qemu-devel] Re: [PATCH] qdev: Catch attempt to attach more than one device to a netdev

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

>   Hi,
>
>> Gerd, what do you think about changing the contract for property parse
>> methods to return -EINVAL, -EBUSY and such?
>
> Hmm, wouldn't it make more sense to integrate with qerror somehow?

Depends.

If you want to move error reporting from qdev_prop_parse() and
set_property() into the parse methods, then simply use qemu_error() or
qemu_error_new() there:

+    qemu_error_new(QERR_SYNTAX, str);
     return -1;

This made a lot of sense if each parser threw largely different errors.
Which isn't the case: we have one so far ("failed to parse"), and we
just encountered a use for a second.

We have the same very few errors occuring in many parsers, and we report
them just the same, regardless of the parser used.  Michael's proposal
is optimized for that: it keeps the actual error reporting centralized
in one place, qdev_prop_parse(), and keeps the code occuring many times
to a bare minimum:

-    return -1;
+    return -EINVAL;

You could attempt to do the same with QError rather than errno.h codes.
Looks like this:

-    return -1;
+    return qemu_error_from_info2(__FILE__, __LINE__, __func__, QERR_SYNTAX, str);

where qemu_error_from_info2() is to qemu_error_from_info() what printf()
is to vprintf().  Naturally, you'd want sweep some of the ugliness under
a macro-rug, so it actually looks more like this:

-    return -1;
+    return qerror_new2(QERR_SYNTAX, str);

Except with a less unimaginative name, of course.

qdev_prop_parse() could then query the QError member error (a QDict) to
find out more about the error.  But there's no need for that.  All it
wants to know about the error is "was there one?".  We get:

-    if (prop->info->parse(dev, prop, value) != 0) {
+    qerr = prop->info->parse(dev, prop, value);
+    if (qerr) {
-        fprintf(stderr, "property \"%s.%s\": failed to parse \"%s\"\n",
-                dev->info->name, name, value);
+        qerror_emit(qerr);
         return -1;
     }

No improvement over letting the parsers call qemu_error_new().

[Qemu-devel] Re: [PATCH] qdev: Catch attempt to attach more than one device to a netdev

[Gerd Hoffmann]

On 02/26/10 10:30, Markus Armbruster wrote:
> This made a lot of sense if each parser threw largely different errors.
> Which isn't the case: we have one so far ("failed to parse"), and we
> just encountered a use for a second.

There are actually three:  parse error, not found 
(netdev=<does-not-exist>) and busy (netdev=<already-in-use>).

But, yes, given the small number of error cases it makes sense to 
generate the error one layer up and not in the parsers themself.

cheers,
   Gerd

[Qemu-devel] Re: [PATCH] qdev: Catch attempt to attach more than one device to a netdev

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

> On 02/26/10 10:30, Markus Armbruster wrote:
>> This made a lot of sense if each parser threw largely different errors.
>> Which isn't the case: we have one so far ("failed to parse"), and we
>> just encountered a use for a second.
>
> There are actually three:  parse error, not found
> (netdev=<does-not-exist>) and busy (netdev=<already-in-use>).
>
> But, yes, given the small number of error cases it makes sense to
> generate the error one layer up and not in the parsers themself.

Okay, I'll cook up v2.