| Message ID | 20230109113158.1500-1-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,v2] qom: Extract object_try_new() from qdev_new() | expand |
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,
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..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. + */ +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,
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(-)