Peter Maydell <peter.mayd...@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.mayd...@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.
