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

[Daniel P . Berrangé]

On Mon, Jan 09, 2023 at 12:20:56PM +0100, 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.

qdev_new() unconditionally tries loading the module, regardless
of whether that particular device type can be built as a module.
Not an issue, as we should only hit the error code of missing
object type for devices which can be loadable, or in case of
programmer error in typename. The latter shouldn't ever happen.

If we want to push this logic down into QOM, this suggests
not introducing a new object_try_new() helper at all. Instead
modify  'object_new' to unconditionally try to load modules.

Or even take it one step further, make 'object_class_by_name'
try to load the module in its error path.

Can anyone think of other scenarios where object_class_by_name
would be expected to fail, that aren't a case of the typename
being a loadable module, or a programmer error ? If not, then
modifying object_class_by_name should be ok.

> 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         | 23 +++++++++++++++++++++++
>  3 files changed, 37 insertions(+), 21 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));
>  }
>  
>  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));
>  }
>  
>  static QTAILQ_HEAD(, DeviceListener) device_listeners
> diff --git a/include/qom/object.h b/include/qom/object.h
> index ef7258a5e1..9cc5bf30ec 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
> + * @name: 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 @name
> + * does not exist, rather than asserting.
> + *
> + * Returns: The newly allocated and instantiated object, or %NULL.
> + */
> +Object *object_try_new(const char *name, 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..6d3faaeb6e 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -755,6 +755,29 @@ Object *object_new(const char *typename)
>  }
>  
>  
> +Object *object_try_new(const char *name, Error **errp)
> +{
> +    TypeImpl *ti = type_get_by_name(name);
> +
> +    if (!ti) {
> +#ifdef CONFIG_MODULES
> +        int rv = module_load_qom(name, errp);
> +        if (rv <= 0) {
> +            error_report("could not find a module for type '%s'", name);
> +            exit(1);
> +        }
> +        ti = type_get_by_name(name);
> +#endif
> +    }
> +    if (!ti) {
> +        error_setg(errp, "unknown type '%s'", name);
> +        return NULL;
> +    }
> +
> +    return object_new_with_type(ti);
> +}
> +
> +
>  Object *object_new_with_props(const char *typename,
>                                Object *parent,
>                                const char *id,
> -- 
> 2.38.1
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|