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

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         | 23 +++++++++++++++++++++++
 3 files changed, 37 insertions(+), 21 deletions(-)

On 9/1/23 12:20, 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         | 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);

Wrong patch, sorry, v2 coming.

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

On 1/9/23 13:39, Daniel P. Berrangé wrote:
> 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.

As far as I understand, when we want this behavior this is what we currently achieve with module_object_class_by_name().

If we are to make module_object_class_by_name() the only way to do object_class_by_name I would just recommend a lot of care and a lot of testing
given the amount of calls to object_class_by_name in the code, studying the underlying assumptions of the code in each case,

especially testing with/without loadable modules, with the modules builtin, loadable but not present, etc.

> 
> 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.

Currently optional AccelCPUClass cpu interfaces can be used to extend a CPUClass with additional acceelerator-specific initializations.
In this context, object_class_by_name in accel-common.c can fail if no such extension is needed,
and since we don't have full target-specific accelerator loadable modules in QEMU, this isn't always a case of the typename being a loadable module, or a programmer error.

Thanks,

Claudio


> 
>> 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