Re: [RFC PATCH v2] qom: Extract object_try_new() from qdev_new()

[Claudio Fontana]

Hi Philippe,

On 1/9/23 12:31, Philippe Mathieu-Daudé wrote:
> The module resolving in qdev_new() is specific to QOM, not to
> QDev. Extract the code as a new QOM object_try_new() helper so
> it can be reused by non-QDev code.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> RFC because I'm wonder if we can't find a better name...
> 
> Also, should we refactor object_initialize() similarly,
> having object_try_initialize(..., Error *)?
> ---
>  hw/core/qdev.c       | 23 ++---------------------
>  include/qom/object.h | 12 ++++++++++++
>  qom/object.c         | 25 ++++++++++++++++++++++++-
>  3 files changed, 38 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index d759c4602c..3a076dcc7f 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -147,31 +147,12 @@ bool qdev_set_parent_bus(DeviceState *dev, BusState 
> *bus, Error **errp)
>  
>  DeviceState *qdev_new(const char *name)
>  {
> -    ObjectClass *oc = object_class_by_name(name);
> -#ifdef CONFIG_MODULES
> -    if (!oc) {
> -        int rv = module_load_qom(name, &error_fatal);
> -        if (rv > 0) {
> -            oc = object_class_by_name(name);
> -        } else {
> -            error_report("could not find a module for type '%s'", name);
> -            exit(1);
> -        }
> -    }
> -#endif
> -    if (!oc) {
> -        error_report("unknown type '%s'", name);
> -        abort();
> -    }
> -    return DEVICE(object_new(name));
> +    return DEVICE(object_try_new(name, &error_fatal));

for clarity, why this last line change?

The new object_new() implementation does exactly this, ie: {
   return object_try_new(typename, &error_fatal);
}

and from a reader perspective, when calling qdev_new() - as opposed to 
qdev_try_new() - 
we want the object to be created, there is no "try".

It is not a functional thing, just something that would seem clearer to me.


>  }
>  
>  DeviceState *qdev_try_new(const char *name)
>  {
> -    if (!module_object_class_by_name(name)) {
> -        return NULL;
> -    }
> -    return DEVICE(object_new(name));
> +    return DEVICE(object_try_new(name, NULL));
>  }

Ok. Here it make sense for me to see "try_new", as we are trying to create a 
qdev.

>  
>  static QTAILQ_HEAD(, DeviceListener) device_listeners
> diff --git a/include/qom/object.h b/include/qom/object.h
> index ef7258a5e1..27059cafb7 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -565,6 +565,18 @@ Object *object_new_with_class(ObjectClass *klass);
>   */
>  Object *object_new(const char *typename);
>  
> +/**
> + * object_try_new: Try to create an object on the heap
> + * @typename: The name of the type of the object to instantiate.
> + * @errp: pointer to Error object.
> + *
> + * This is like object_new(), except it returns %NULL when type @typename
> + * does not exist, rather than asserting.
> + *
> + * Returns: The newly allocated and instantiated object, or %NULL.

Does it make sense to warn or document that in order to make any error fatal,
those special values can be passed? I know it is documented in 
include/qapi/error.h in pages of text,

but from reading this function alone, one would think that try_new is never 
fatal, which in some way
is true, but also isn't..

When is the actual error propagation and check for the special value 
error_fatal and error_abort happening?
Is it worthwhile documenting it here, especially for someone investigating the 
use of qdev_new() ?


> + */
> +Object *object_try_new(const char *typename, Error **errp);
> +
>  /**
>   * object_new_with_props:
>   * @typename:  The name of the type of the object to instantiate.
> diff --git a/qom/object.c b/qom/object.c
> index e25f1e96db..13070393ef 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -747,14 +747,37 @@ Object *object_new_with_class(ObjectClass *klass)
>      return object_new_with_type(klass->type);
>  }
>  
> -Object *object_new(const char *typename)
> +
> +Object *object_try_new(const char *typename, Error **errp)
>  {
>      TypeImpl *ti = type_get_by_name(typename);
>  
> +#ifdef CONFIG_MODULES
> +    if (!ti) {
> +        int rv = module_load_qom(typename, errp);
> +        if (rv) {
> +            error_prepend(errp, "could not find a module for type '%s': ",
> +                          typename);
> +            return NULL;
> +        }
> +        ti = type_get_by_name(typename);
> +    }
> +#endif
> +    if (!ti) {
> +        error_setg(errp, "unknown type '%s'", typename);
> +        return NULL;
> +    }
> +
>      return object_new_with_type(ti);
>  }
>  
>  
> +Object *object_new(const char *typename)
> +{
> +    return object_try_new(typename, &error_fatal);
> +}
> +
> +
>  Object *object_new_with_props(const char *typename,
>                                Object *parent,
>                                const char *id,