| Message ID | 1434115575-7214-3-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
Peter Maydell <peter.maydell@linaro.org> writes: > Instead of having set_pointer() call a parse callback which returns > an error number that we then convert to an Error string with > error_set_from_qdev_prop_error(), make the parse callback take an > Error** and set the error itself. This will allow parse routines > to provide more helpful error messages than the generic ones. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > hw/core/qdev-properties-system.c | 33 ++++++++++++++++++++------------- > 1 file changed, 20 insertions(+), 13 deletions(-) > > diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c > index 0309fe5..56954b4 100644 > --- a/hw/core/qdev-properties-system.c > +++ b/hw/core/qdev-properties-system.c > @@ -35,15 +35,15 @@ static void get_pointer(Object *obj, Visitor *v, Property *prop, > } > > static void set_pointer(Object *obj, Visitor *v, Property *prop, > - int (*parse)(DeviceState *dev, const char *str, > - void **ptr), > + void (*parse)(DeviceState *dev, const char *str, > + void **ptr, const char *propname, > + Error **errp), > const char *name, Error **errp) > { > DeviceState *dev = DEVICE(obj); > Error *local_err = NULL; > void **ptr = qdev_get_prop_ptr(dev, prop); > char *str; > - int ret; > > if (dev->realized) { > qdev_prop_set_after_realize(dev, name, errp); > @@ -60,26 +60,29 @@ static void set_pointer(Object *obj, Visitor *v, Property *prop, > *ptr = NULL; > return; > } > - ret = parse(dev, str, ptr); > - error_set_from_qdev_prop_error(errp, ret, dev, prop, str); > + parse(dev, str, ptr, prop->name, errp); > g_free(str); > } > > /* --- drive --- */ > > -static int parse_drive(DeviceState *dev, const char *str, void **ptr) > +static void parse_drive(DeviceState *dev, const char *str, void **ptr, > + const char *propname, Error **errp) > { > BlockBackend *blk; > > blk = blk_by_name(str); > if (!blk) { > - return -ENOENT; > + error_setg(errp, "Property '%s.%s' can't find value '%s'", > + object_get_typename(OBJECT(dev)), propname, str); > + return; > } > if (blk_attach_dev(blk, dev) < 0) { > - return -EEXIST; > + error_setg(errp, "Property '%s.%s' can't take value '%s', it's in use", > + object_get_typename(OBJECT(dev)), propname, str); > + return; > } > *ptr = blk; > - return 0; > } > > static void release_drive(Object *obj, const char *name, void *opaque) > @@ -121,17 +124,21 @@ PropertyInfo qdev_prop_drive = { > > /* --- character device --- */ > > -static int parse_chr(DeviceState *dev, const char *str, void **ptr) > +static void parse_chr(DeviceState *dev, const char *str, void **ptr, > + const char *propname, Error **errp) > { > CharDriverState *chr = qemu_chr_find(str); > if (chr == NULL) { > - return -ENOENT; > + error_setg(errp, "Property '%s.%s' can't find value '%s'", > + object_get_typename(OBJECT(dev)), propname, str); > + return; > } > if (qemu_chr_fe_claim(chr) != 0) { > - return -EEXIST; > + error_setg(errp, "Property '%s.%s' can't take value '%s', it's in use", > + object_get_typename(OBJECT(dev)), propname, str); > + return; > } > *ptr = chr; > - return 0; > } > > static void release_chr(Object *obj, const char *name, void *opaque) The error messages are faithfully copied from error_set_from_qdev_prop_error(). The next patch will improve parse_drive()'s messages, and that's why you got this patch. The next patch takes special care of the if=T, T!=none case. That case only exists for drives, because only for drives did we mix up old-style (one option to configure front- and backend) and new-style (one option to configure backend, -device to configure frontend) configuration. It also changes the if=none case, from Property 'TYPE.PROP' can't take value 'VAL', it's in use to "Property 'TYPE.PROP' can't be set to drive ID 'VAL'; that drive has already been connected to another device." More explicit. Rather long, though. If this change is deemed an improvement for drive=, then it surely is one for the other backend references, isn't it? These are parse_chr() for chardev= (shown above), and set_netdev() for netdev= (not shown). Property 'TYPE.PROP' can't take value 'VAL', it's in use vs. Property 'TYPE.PROP' can't take value 'VAL'; that chardev has already been connected to another device. Same as above, except s/drive/chardev/. Likewise for netdev. Other users of error_set_from_qdev_prop_error() remain: set_vlan(), set_mac(), set_pci_devfn(), set_pci_host_devaddr(). Okay. I figure the error message duplication could be avoided by calling error_set_from_qdev_prop_error() here. No biggie either way.
On 22 June 2015 at 10:39, Markus Armbruster <armbru@redhat.com> wrote: > Peter Maydell <peter.maydell@linaro.org> writes: > >> Instead of having set_pointer() call a parse callback which returns >> an error number that we then convert to an Error string with >> error_set_from_qdev_prop_error(), make the parse callback take an >> Error** and set the error itself. This will allow parse routines >> to provide more helpful error messages than the generic ones. > The error messages are faithfully copied from > error_set_from_qdev_prop_error(). The next patch will improve > parse_drive()'s messages, and that's why you got this patch. That's right. > The next patch takes special care of the if=T, T!=none case. That case > only exists for drives, because only for drives did we mix up old-style > (one option to configure front- and backend) and new-style (one option > to configure backend, -device to configure frontend) configuration. > > It also changes the if=none case, from > > Property 'TYPE.PROP' can't take value 'VAL', it's in use > > to > > "Property 'TYPE.PROP' can't be set to drive ID 'VAL'; that drive has already been connected to another device." > > More explicit. Rather long, though. Do you have a proposed better phrasing? In general I feel that the problem with the current error message is that: (1) it is phrased from the point of view of QEMU's internal implementation (2) it is extremely terse So the lengthiness is in my opinion an improvement. Better not to leave the user in the dark about why we just refused to run... > If this change is deemed an improvement for drive=, then it surely is > one for the other backend references, isn't it? These are parse_chr() > for chardev= (shown above), and set_netdev() for netdev= (not shown). > > Property 'TYPE.PROP' can't take value 'VAL', it's in use > > vs. > > Property 'TYPE.PROP' can't take value 'VAL'; that chardev has already been connected to another device. > > Same as above, except s/drive/chardev/. Likewise for netdev. > > Other users of error_set_from_qdev_prop_error() remain: set_vlan(), > set_mac(), set_pci_devfn(), set_pci_host_devaddr(). Okay. I figure the > error message duplication could be avoided by calling > error_set_from_qdev_prop_error() here. No biggie either way. Yes. My feeling is that error_set_from_qdev_prop_error() should probably be removed entirely; but with 2.4 looming I didn't want to bulk this patchset out with that change. -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On 22 June 2015 at 10:39, Markus Armbruster <armbru@redhat.com> wrote: >> Peter Maydell <peter.maydell@linaro.org> writes: >> >>> Instead of having set_pointer() call a parse callback which returns >>> an error number that we then convert to an Error string with >>> error_set_from_qdev_prop_error(), make the parse callback take an >>> Error** and set the error itself. This will allow parse routines >>> to provide more helpful error messages than the generic ones. > >> The error messages are faithfully copied from >> error_set_from_qdev_prop_error(). The next patch will improve >> parse_drive()'s messages, and that's why you got this patch. > > That's right. > >> The next patch takes special care of the if=T, T!=none case. That case >> only exists for drives, because only for drives did we mix up old-style >> (one option to configure front- and backend) and new-style (one option >> to configure backend, -device to configure frontend) configuration. >> >> It also changes the if=none case, from >> >> Property 'TYPE.PROP' can't take value 'VAL', it's in use >> >> to >> >> "Property 'TYPE.PROP' can't be set to drive ID 'VAL'; that drive >> has already been connected to another device." >> >> More explicit. Rather long, though. > > Do you have a proposed better phrasing? In general I feel that the problem > with the current error message is that: > (1) it is phrased from the point of view of QEMU's internal implementation > (2) it is extremely terse > > So the lengthiness is in my opinion an improvement. Better not to > leave the user in the dark about why we just refused to run... Perhaps Drive VAL is already used by another device Relies on the fact that the context is either blatantly obvious (monitor command), or the actual message makes it obvious, e.g. qemu-system-arm: -device virtio-blk-pci,drive=foo: Drive foo is already used by another device [...]
diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c index 0309fe5..56954b4 100644 --- a/hw/core/qdev-properties-system.c +++ b/hw/core/qdev-properties-system.c @@ -35,15 +35,15 @@ static void get_pointer(Object *obj, Visitor *v, Property *prop, } static void set_pointer(Object *obj, Visitor *v, Property *prop, - int (*parse)(DeviceState *dev, const char *str, - void **ptr), + void (*parse)(DeviceState *dev, const char *str, + void **ptr, const char *propname, + Error **errp), const char *name, Error **errp) { DeviceState *dev = DEVICE(obj); Error *local_err = NULL; void **ptr = qdev_get_prop_ptr(dev, prop); char *str; - int ret; if (dev->realized) { qdev_prop_set_after_realize(dev, name, errp); @@ -60,26 +60,29 @@ static void set_pointer(Object *obj, Visitor *v, Property *prop, *ptr = NULL; return; } - ret = parse(dev, str, ptr); - error_set_from_qdev_prop_error(errp, ret, dev, prop, str); + parse(dev, str, ptr, prop->name, errp); g_free(str); } /* --- drive --- */ -static int parse_drive(DeviceState *dev, const char *str, void **ptr) +static void parse_drive(DeviceState *dev, const char *str, void **ptr, + const char *propname, Error **errp) { BlockBackend *blk; blk = blk_by_name(str); if (!blk) { - return -ENOENT; + error_setg(errp, "Property '%s.%s' can't find value '%s'", + object_get_typename(OBJECT(dev)), propname, str); + return; } if (blk_attach_dev(blk, dev) < 0) { - return -EEXIST; + error_setg(errp, "Property '%s.%s' can't take value '%s', it's in use", + object_get_typename(OBJECT(dev)), propname, str); + return; } *ptr = blk; - return 0; } static void release_drive(Object *obj, const char *name, void *opaque) @@ -121,17 +124,21 @@ PropertyInfo qdev_prop_drive = { /* --- character device --- */ -static int parse_chr(DeviceState *dev, const char *str, void **ptr) +static void parse_chr(DeviceState *dev, const char *str, void **ptr, + const char *propname, Error **errp) { CharDriverState *chr = qemu_chr_find(str); if (chr == NULL) { - return -ENOENT; + error_setg(errp, "Property '%s.%s' can't find value '%s'", + object_get_typename(OBJECT(dev)), propname, str); + return; } if (qemu_chr_fe_claim(chr) != 0) { - return -EEXIST; + error_setg(errp, "Property '%s.%s' can't take value '%s', it's in use", + object_get_typename(OBJECT(dev)), propname, str); + return; } *ptr = chr; - return 0; } static void release_chr(Object *obj, const char *name, void *opaque)
Instead of having set_pointer() call a parse callback which returns an error number that we then convert to an Error string with error_set_from_qdev_prop_error(), make the parse callback take an Error** and set the error itself. This will allow parse routines to provide more helpful error messages than the generic ones. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/core/qdev-properties-system.c | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-)