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

[Claudio Fontana]

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