Re: [Qemu-devel] [PATCH 2/4] qdev-properties-system: Change set_pointer's parse callback to use Error

[Markus Armbruster]

Peter Maydell <address@hidden> 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 <address@hidden>
> ---
>  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.