Re: [PATCH v0 1/2] qdev-properties-system: extend set_pionter for unrealized devices

[Eduardo Habkost]

On Sun, Nov 10, 2019 at 10:03:09PM +0300, Denis Plotnikov wrote:
> Some device's property can be changed if the device has been already
> realized. For example, it could be "drive" property of a scsi disk device.
> 
> So far, set_pointer could operate only on a relized device. The patch
> extends its interface for operation on an unrealized device.
> 
> Signed-off-by: Denis Plotnikov <address@hidden>
> ---
>  hw/core/qdev-properties-system.c | 32 +++++++++++++++++++++-----------
>  1 file changed, 21 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/core/qdev-properties-system.c 
> b/hw/core/qdev-properties-system.c
> index ba412dd2ca..c534590dcd 100644
> --- a/hw/core/qdev-properties-system.c
> +++ b/hw/core/qdev-properties-system.c
> @@ -38,9 +38,14 @@ static void get_pointer(Object *obj, Visitor *v, Property 
> *prop,
>  }
>  
>  static void set_pointer(Object *obj, Visitor *v, Property *prop,
> -                        void (*parse)(DeviceState *dev, const char *str,
> -                                      void **ptr, const char *propname,
> -                                      Error **errp),
> +                        void (*parse_realized)(DeviceState *dev,
> +                                               const char *str, void **ptr,
> +                                               const char *propname,
> +                                               Error **errp),
> +                        void (*parse_unrealized)(DeviceState *dev,
> +                                                 const char *str, void **ptr,
> +                                                 const char *propname,
> +                                                 Error **errp),
>                          const char *name, Error **errp)

Wouldn't it be simpler to just add a PropertyInfo::allow_set_after_realize
bool field, and call the same setter function?  Then you can
simply change do_parse_drive() to check if realized is true.

>  {
>      DeviceState *dev = DEVICE(obj);
> @@ -48,11 +53,6 @@ static void set_pointer(Object *obj, Visitor *v, Property 
> *prop,
>      void **ptr = qdev_get_prop_ptr(dev, prop);
>      char *str;
>  
> -    if (dev->realized) {
> -        qdev_prop_set_after_realize(dev, name, errp);
> -        return;
> -    }
> -
>      visit_type_str(v, name, &str, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
> @@ -63,7 +63,17 @@ static void set_pointer(Object *obj, Visitor *v, Property 
> *prop,
>          *ptr = NULL;
>          return;
>      }
> -    parse(dev, str, ptr, prop->name, errp);
> +
> +    if (dev->realized) {
> +        if (parse_realized) {
> +            parse_realized(dev, str, ptr, prop->name, errp);
> +        } else {
> +            qdev_prop_set_after_realize(dev, name, errp);
> +        }
> +    } else {
> +        parse_unrealized(dev, str, ptr, prop->name, errp);
> +    }
> +
>      g_free(str);
>  }
>  
> @@ -178,13 +188,13 @@ static void get_drive(Object *obj, Visitor *v, const 
> char *name, void *opaque,
>  static void set_drive(Object *obj, Visitor *v, const char *name, void 
> *opaque,
>                        Error **errp)
>  {
> -    set_pointer(obj, v, opaque, parse_drive, name, errp);
> +    set_pointer(obj, v, opaque, NULL, parse_drive, name, errp);
>  }
>  
>  static void set_drive_iothread(Object *obj, Visitor *v, const char *name,
>                                 void *opaque, Error **errp)
>  {
> -    set_pointer(obj, v, opaque, parse_drive_iothread, name, errp);
> +    set_pointer(obj, v, opaque, NULL, parse_drive_iothread, name, errp);
>  }
>  
>  const PropertyInfo qdev_prop_drive = {
> -- 
> 2.17.0
> 

-- 
Eduardo