[v5,02/10] object: qom module support
diff mbox series

Message ID 20200624131045.14512-3-kraxel@redhat.com
State New
Headers show
Series
  • build some devices as modules.
Related show

Commit Message

Gerd Hoffmann June 24, 2020, 1:10 p.m. UTC
Little helper function to load modules on demand.  In most cases adding
module loading support for devices and other objects is just
s/object_class_by_name/module_object_class_by_name/ in the right spot.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/qom/object.h | 12 ++++++++++++
 qom/object.c         | 14 ++++++++++++++
 2 files changed, 26 insertions(+)

Comments

Christophe de Dinechin July 20, 2020, 2:20 p.m. UTC | #1
On 2020-06-24 at 15:10 CEST, Gerd Hoffmann wrote...
> Little helper function to load modules on demand.  In most cases adding
> module loading support for devices and other objects is just
> s/object_class_by_name/module_object_class_by_name/ in the right spot.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/qom/object.h | 12 ++++++++++++
>  qom/object.c         | 14 ++++++++++++++
>  2 files changed, 26 insertions(+)
>
> diff --git a/include/qom/object.h b/include/qom/object.h
> index 94a61ccc3fe8..51f188137f1f 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -994,6 +994,18 @@ bool object_class_is_abstract(ObjectClass *klass);
>   */
>  ObjectClass *object_class_by_name(const char *typename);
>
> +/**
> + * module_object_class_by_name:
> + * @typename: The QOM typename to obtain the class for.
> + *
> + * For objects which might be provided by a module.  Behaves like
> + * object_class_by_name, but additionally tries to load the module
> + * needed in case the class is not available.
> + *
> + * Returns: The class for @typename or %NULL if not found.
> + */
> +ObjectClass *module_object_class_by_name(const char *typename);
> +
>  void object_class_foreach(void (*fn)(ObjectClass *klass, void *opaque),
>                            const char *implements_type, bool include_abstract,
>                            void *opaque);
> diff --git a/qom/object.c b/qom/object.c
> index 6ece96bc2bfc..34daaf1280f5 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -985,6 +985,20 @@ ObjectClass *object_class_by_name(const char *typename)
>      return type->class;
>  }
>
> +ObjectClass *module_object_class_by_name(const char *typename)
> +{
> +    ObjectClass *oc;
> +
> +    oc = object_class_by_name(typename);
> +#ifdef CONFIG_MODULES
> +    if (!oc) {
> +        module_load_qom_one(typename);
> +        oc = object_class_by_name(typename);
> +    }
> +#endif

I'm wondering if there is any reason to only trigger the module load when
you don't find the object class. You could simply call module_load_qom_one
under #ifdef CONFIG_MODULES.

Performance wise, I don't think this makes much of a difference, and it
simplifies the logical flow IMO.

> +    return oc;
> +}
> +
>  ObjectClass *object_class_get_parent(ObjectClass *class)
>  {
>      TypeImpl *type = type_get_parent(class->type);


--
Cheers,
Christophe de Dinechin (IRC c3d)
Gerd Hoffmann July 21, 2020, 2:26 p.m. UTC | #2
Hi,

> > +ObjectClass *module_object_class_by_name(const char *typename)
> > +{
> > +    ObjectClass *oc;
> > +
> > +    oc = object_class_by_name(typename);
> > +#ifdef CONFIG_MODULES
> > +    if (!oc) {
> > +        module_load_qom_one(typename);
> > +        oc = object_class_by_name(typename);
> > +    }
> > +#endif
> 
> I'm wondering if there is any reason to only trigger the module load when
> you don't find the object class. You could simply call module_load_qom_one
> under #ifdef CONFIG_MODULES.
> 
> Performance wise, I don't think this makes much of a difference, and it
> simplifies the logical flow IMO.

I expect the common case is that the object class is found and there is
rarely a need to actually load a module.

take care,
  Gerd
Daniel P. Berrangé July 21, 2020, 2:35 p.m. UTC | #3
On Mon, Jul 20, 2020 at 04:20:55PM +0200, Christophe de Dinechin wrote:
> 
> On 2020-06-24 at 15:10 CEST, Gerd Hoffmann wrote...
> > Little helper function to load modules on demand.  In most cases adding
> > module loading support for devices and other objects is just
> > s/object_class_by_name/module_object_class_by_name/ in the right spot.
> >
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > ---
> >  include/qom/object.h | 12 ++++++++++++
> >  qom/object.c         | 14 ++++++++++++++
> >  2 files changed, 26 insertions(+)
> >
> > diff --git a/include/qom/object.h b/include/qom/object.h
> > index 94a61ccc3fe8..51f188137f1f 100644
> > --- a/include/qom/object.h
> > +++ b/include/qom/object.h
> > @@ -994,6 +994,18 @@ bool object_class_is_abstract(ObjectClass *klass);
> >   */
> >  ObjectClass *object_class_by_name(const char *typename);
> >
> > +/**
> > + * module_object_class_by_name:
> > + * @typename: The QOM typename to obtain the class for.
> > + *
> > + * For objects which might be provided by a module.  Behaves like
> > + * object_class_by_name, but additionally tries to load the module
> > + * needed in case the class is not available.
> > + *
> > + * Returns: The class for @typename or %NULL if not found.
> > + */
> > +ObjectClass *module_object_class_by_name(const char *typename);
> > +
> >  void object_class_foreach(void (*fn)(ObjectClass *klass, void *opaque),
> >                            const char *implements_type, bool include_abstract,
> >                            void *opaque);
> > diff --git a/qom/object.c b/qom/object.c
> > index 6ece96bc2bfc..34daaf1280f5 100644
> > --- a/qom/object.c
> > +++ b/qom/object.c
> > @@ -985,6 +985,20 @@ ObjectClass *object_class_by_name(const char *typename)
> >      return type->class;
> >  }
> >
> > +ObjectClass *module_object_class_by_name(const char *typename)
> > +{
> > +    ObjectClass *oc;
> > +
> > +    oc = object_class_by_name(typename);
> > +#ifdef CONFIG_MODULES
> > +    if (!oc) {
> > +        module_load_qom_one(typename);
> > +        oc = object_class_by_name(typename);
> > +    }
> > +#endif
> 
> I'm wondering if there is any reason to only trigger the module load when
> you don't find the object class. You could simply call module_load_qom_one
> under #ifdef CONFIG_MODULES.
> 
> Performance wise, I don't think this makes much of a difference, and it
> simplifies the logical flow IMO.

I wouldn't make that assumption about performance - module_load_qom_one()
does alot of work, and there are places where object / class creation is
performance critical. We might not happen to trigger them now with the
current set of modules, but we can easily do so in future.


Regards,
Daniel
Christophe de Dinechin July 22, 2020, 8:06 a.m. UTC | #4
On 2020-06-24 at 15:10 CEST, Gerd Hoffmann wrote...
> Little helper function to load modules on demand.  In most cases adding
> module loading support for devices and other objects is just
> s/object_class_by_name/module_object_class_by_name/ in the right spot.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/qom/object.h | 12 ++++++++++++
>  qom/object.c         | 14 ++++++++++++++
>  2 files changed, 26 insertions(+)
>
> diff --git a/include/qom/object.h b/include/qom/object.h
> index 94a61ccc3fe8..51f188137f1f 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -994,6 +994,18 @@ bool object_class_is_abstract(ObjectClass *klass);
>   */
>  ObjectClass *object_class_by_name(const char *typename);
>
> +/**
> + * module_object_class_by_name:
> + * @typename: The QOM typename to obtain the class for.
> + *
> + * For objects which might be provided by a module.  Behaves like
> + * object_class_by_name, but additionally tries to load the module
> + * needed in case the class is not available.
> + *
> + * Returns: The class for @typename or %NULL if not found.
> + */
> +ObjectClass *module_object_class_by_name(const char *typename);
> +
>  void object_class_foreach(void (*fn)(ObjectClass *klass, void *opaque),
>                            const char *implements_type, bool include_abstract,
>                            void *opaque);
> diff --git a/qom/object.c b/qom/object.c
> index 6ece96bc2bfc..34daaf1280f5 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -985,6 +985,20 @@ ObjectClass *object_class_by_name(const char *typename)
>      return type->class;
>  }
>
> +ObjectClass *module_object_class_by_name(const char *typename)

Re-reading this, it occurred to me that typename is a keyword in C++.

I don't know if we want to make it a rule to not use C++ keywords just in
case we would some day compile that code with a C++ compiler.

> +{
> +    ObjectClass *oc;
> +
> +    oc = object_class_by_name(typename);
> +#ifdef CONFIG_MODULES
> +    if (!oc) {
> +        module_load_qom_one(typename);
> +        oc = object_class_by_name(typename);
> +    }
> +#endif
> +    return oc;
> +}
> +
>  ObjectClass *object_class_get_parent(ObjectClass *class)
>  {
>      TypeImpl *type = type_get_parent(class->type);


--
Cheers,
Christophe de Dinechin (IRC c3d)
Gerd Hoffmann July 22, 2020, 11:07 a.m. UTC | #5
> > +ObjectClass *module_object_class_by_name(const char *typename)
> 
> Re-reading this, it occurred to me that typename is a keyword in C++.
> 
> I don't know if we want to make it a rule to not use C++ keywords just in
> case we would some day compile that code with a C++ compiler.

If we want do this we have to touch lots of places anyway, using
typename as variable is pretty common in qom code ...

take care,
  Gerd

Patch
diff mbox series

diff --git a/include/qom/object.h b/include/qom/object.h
index 94a61ccc3fe8..51f188137f1f 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -994,6 +994,18 @@  bool object_class_is_abstract(ObjectClass *klass);
  */
 ObjectClass *object_class_by_name(const char *typename);
 
+/**
+ * module_object_class_by_name:
+ * @typename: The QOM typename to obtain the class for.
+ *
+ * For objects which might be provided by a module.  Behaves like
+ * object_class_by_name, but additionally tries to load the module
+ * needed in case the class is not available.
+ *
+ * Returns: The class for @typename or %NULL if not found.
+ */
+ObjectClass *module_object_class_by_name(const char *typename);
+
 void object_class_foreach(void (*fn)(ObjectClass *klass, void *opaque),
                           const char *implements_type, bool include_abstract,
                           void *opaque);
diff --git a/qom/object.c b/qom/object.c
index 6ece96bc2bfc..34daaf1280f5 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -985,6 +985,20 @@  ObjectClass *object_class_by_name(const char *typename)
     return type->class;
 }
 
+ObjectClass *module_object_class_by_name(const char *typename)
+{
+    ObjectClass *oc;
+
+    oc = object_class_by_name(typename);
+#ifdef CONFIG_MODULES
+    if (!oc) {
+        module_load_qom_one(typename);
+        oc = object_class_by_name(typename);
+    }
+#endif
+    return oc;
+}
+
 ObjectClass *object_class_get_parent(ObjectClass *class)
 {
     TypeImpl *type = type_get_parent(class->type);