| Message ID | m3fx4pwrqw.fsf@blackfin.pond.sub.org |
|---|---|
| State | New |
| Headers | show |
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 > >
"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 >> >>
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 > >> > >>
"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.
"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?
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
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().
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
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.
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; }
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(-)