| Message ID | 20200624131045.14512-2-kraxel@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | build some devices as modules. | expand |
On 2020-06-24 at 15:10 CEST, Gerd Hoffmann wrote... > Add support for qom types provided by modules. For starters use a > manually maintained list which maps qom type to module and prefix. > > Two load functions are added: One to load the module for a specific > type, and one to load all modules (needed for object/device lists as > printed by -- for example -- qemu -device help). > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > --- > include/qemu/module.h | 2 ++ > util/module.c | 55 +++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 57 insertions(+) > > diff --git a/include/qemu/module.h b/include/qemu/module.h > index 011ae1ae7605..9121a475c1b6 100644 > --- a/include/qemu/module.h > +++ b/include/qemu/module.h > @@ -70,5 +70,7 @@ void register_dso_module_init(void (*fn)(void), module_init_type type); > > void module_call_init(module_init_type type); > bool module_load_one(const char *prefix, const char *lib_name); > +void module_load_qom_one(const char *type); > +void module_load_qom_all(void); > > #endif > diff --git a/util/module.c b/util/module.c > index e48d9aacc05a..ee560a4b4269 100644 > --- a/util/module.c > +++ b/util/module.c > @@ -245,3 +245,58 @@ bool module_load_one(const char *prefix, const char *lib_name) > #endif > return success; > } > + > +/* > + * Building devices and other qom objects modular is mostly useful in > + * case they have dependencies to external shared libraries, so we can > + * cut down the core qemu library dependencies. Which is the case for > + * only a very few devices & objects. > + * > + * So with the expectation that this will be rather the exception than > + * to rule and the list will not gain that many entries go with a Nit: Add a comma after "entries" I would also indicate that this list needs to be sorted by prefix/module > + * simple manually maintained list for now. > + */ > +static struct { > + const char *type; > + const char *prefix; > + const char *module; Because of the sorting rule, it is more intuitive to put the module first and the type last, for cases where you have mod1 prefix1 type1 mod1 prefix1 type2 mod1 prefix1 type3 mod2 prefix2 type1 Visually, I expect the constants to come first. > +} const qom_modules[] = { > +}; > + > +static bool module_loaded_qom_all; > + > +void module_load_qom_one(const char *type) > +{ > + int i; > + > + if (module_loaded_qom_all) { > + return; > + } > + for (i = 0; i < ARRAY_SIZE(qom_modules); i++) { > + if (strcmp(qom_modules[i].type, type) == 0) { > + module_load_one(qom_modules[i].prefix, > + qom_modules[i].module); > + return; > + } > + } > +} > + > +void module_load_qom_all(void) > +{ > + int i; > + > + if (module_loaded_qom_all) { > + return; > + } > + for (i = 0; i < ARRAY_SIZE(qom_modules); i++) { > + if (i > 0 && (strcmp(qom_modules[i - 1].module, > + qom_modules[i].module) == 0 && > + strcmp(qom_modules[i - 1].prefix, > + qom_modules[i].prefix) == 0)) { > + /* one module implementing multiple types -> load only once */ This is the sorting requirement I'm talking about > + continue; > + } > + module_load_one(qom_modules[i].prefix, qom_modules[i].module); > + } > + module_loaded_qom_all = true; > +} -- Cheers, Christophe de Dinechin (IRC c3d)
> > +/* > > + * Building devices and other qom objects modular is mostly useful in > > + * case they have dependencies to external shared libraries, so we can > > + * cut down the core qemu library dependencies. Which is the case for > > + * only a very few devices & objects. > > + * > > + * So with the expectation that this will be rather the exception than > > + * to rule and the list will not gain that many entries go with a > > Nit: Add a comma after "entries" > > I would also indicate that this list needs to be sorted by prefix/module Done (incremental patch, series is merged already). > > + const char *type; > > + const char *prefix; > > + const char *module; > > Because of the sorting rule, it is more intuitive to put the module first > and the type last, for cases where you have > > mod1 prefix1 type1 > mod1 prefix1 type2 > mod1 prefix1 type3 > mod2 prefix2 type1 > > Visually, I expect the constants to come first. I see it more as a object-type -> module lookup table, thats why the type comes first ... take care, Gerd
Hello Gerd, Paolo,
this commit from 2020, together with others in the same series, contains a flaw that now comes back to bite.
From commit 28457744c345ca4ccb58c984c9552e9c5955a9de ("module: qom module support") :
On 6/24/20 15:10, Gerd Hoffmann wrote:
> Add support for qom types provided by modules. For starters use a
> manually maintained list which maps qom type to module and prefix.
>
> Two load functions are added: One to load the module for a specific
> type, and one to load all modules (needed for object/device lists as
> printed by -- for example -- qemu -device help).
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
> include/qemu/module.h | 2 ++
> util/module.c | 55 +++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 57 insertions(+)
>
> diff --git a/include/qemu/module.h b/include/qemu/module.h
> index 011ae1ae7605..9121a475c1b6 100644
> --- a/include/qemu/module.h
> +++ b/include/qemu/module.h
> @@ -70,5 +70,7 @@ void register_dso_module_init(void (*fn)(void), module_init_type type);
>
> void module_call_init(module_init_type type);
> bool module_load_one(const char *prefix, const char *lib_name);
> +void module_load_qom_one(const char *type);
> +void module_load_qom_all(void);
See the "+void" just below the "bool". We have now hidden all the return values of module_load_one,
so now we cannot figure out if a load was successful or not, and the fact that module_load_one returns bool is now pointless, at least for qom.
As a pattern, I think this is rarely a good idea.
>
> #endif
> diff --git a/util/module.c b/util/module.c
> index e48d9aacc05a..ee560a4b4269 100644
> --- a/util/module.c
> +++ b/util/module.c
> @@ -245,3 +245,58 @@ bool module_load_one(const char *prefix, const char *lib_name)
> #endif
> return success;
> }
> +
> +/*
> + * Building devices and other qom objects modular is mostly useful in
> + * case they have dependencies to external shared libraries, so we can
> + * cut down the core qemu library dependencies. Which is the case for
> + * only a very few devices & objects.
> + *
> + * So with the expectation that this will be rather the exception than
> + * to rule and the list will not gain that many entries go with a
> + * simple manually maintained list for now.
> + */
> +static struct {
> + const char *type;
> + const char *prefix;
> + const char *module;
> +} const qom_modules[] = {
> +};
> +
> +static bool module_loaded_qom_all;
> +
> +void module_load_qom_one(const char *type)
> +{
> + int i;
> +
> + if (module_loaded_qom_all) {
> + return;
> + }
> + for (i = 0; i < ARRAY_SIZE(qom_modules); i++) {
> + if (strcmp(qom_modules[i].type, type) == 0) {
> + module_load_one(qom_modules[i].prefix,
> + qom_modules[i].module);
> + return;
return value lost.
Should we not at least trace something, warn something?
Maybe we want to do it in module_load_one?
> + }
> + }
> +}
> +
> +void module_load_qom_all(void)
> +{
> + int i;
> +
> + if (module_loaded_qom_all) {
> + return;
> + }
> + for (i = 0; i < ARRAY_SIZE(qom_modules); i++) {
> + if (i > 0 && (strcmp(qom_modules[i - 1].module,
> + qom_modules[i].module) == 0 &&
> + strcmp(qom_modules[i - 1].prefix,
> + qom_modules[i].prefix) == 0)) {
> + /* one module implementing multiple types -> load only once */
> + continue;
> + }
> + module_load_one(qom_modules[i].prefix, qom_modules[i].module);
return value lost.
> + }
> + module_loaded_qom_all = true;
> +}
Pair this with the next commit,
commit 0f8198f1b2f3c33df2381c412ad8d8fd219b90b2 ("object: qom module support") :
which goes:
> commit 0f8198f1b2f3c33df2381c412ad8d8fd219b90b2
> Author: Gerd Hoffmann <kraxel@redhat.com>
> Date: Wed Jun 24 15:10:37 2020 +0200
>
> object: qom module support
>
> 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>
> Message-id: 20200624131045.14512-3-kraxel@redhat.com
>
> diff --git a/include/qom/object.h b/include/qom/object.h
> index 94a61ccc3f..51f188137f 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
Note the use of the word "tries" in the comment.
> + * 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 6ece96bc2b..34daaf1280 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);
now here we fail during object_class_by_name() with oc that is NULL. So I guess we can evince that module_load_qom_one failed from here.
> + }
> +#endif
> + return oc;
> +}
> +
> ObjectClass *object_class_get_parent(ObjectClass *class)
> {
> TypeImpl *type = type_get_parent(class->type);
We should be able to tell if a load is successful or not I think, but before that,
I think we are currently inconsistent, and do not report enough in terms of tracing and errors to the user
as to the reason that a module load fails.
Either we add tracing to module_load_one, and make it so that module_load_one also returns void,
or we change module_load_qom_one to also return bool.
Note, qdev also makes use of this facility.
Thoughts?
Claudio
On 5/9/22 11:17, Claudio Fontana wrote: > Hello Gerd, Paolo, > > this commit from 2020, together with others in the same series, contains a flaw that now comes back to bite. > > From commit 28457744c345ca4ccb58c984c9552e9c5955a9de ("module: qom module support") : > > On 6/24/20 15:10, Gerd Hoffmann wrote: >> Add support for qom types provided by modules. For starters use a >> manually maintained list which maps qom type to module and prefix. >> >> Two load functions are added: One to load the module for a specific >> type, and one to load all modules (needed for object/device lists as >> printed by -- for example -- qemu -device help). >> >> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> >> --- >> include/qemu/module.h | 2 ++ >> util/module.c | 55 +++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 57 insertions(+) >> >> diff --git a/include/qemu/module.h b/include/qemu/module.h >> index 011ae1ae7605..9121a475c1b6 100644 >> --- a/include/qemu/module.h >> +++ b/include/qemu/module.h >> @@ -70,5 +70,7 @@ void register_dso_module_init(void (*fn)(void), module_init_type type); >> >> void module_call_init(module_init_type type); >> bool module_load_one(const char *prefix, const char *lib_name); >> +void module_load_qom_one(const char *type); >> +void module_load_qom_all(void); > > > See the "+void" just below the "bool". We have now hidden all the return values of module_load_one, > so now we cannot figure out if a load was successful or not, and the fact that module_load_one returns bool is now pointless, at least for qom. > > As a pattern, I think this is rarely a good idea. > > >> >> #endif >> diff --git a/util/module.c b/util/module.c >> index e48d9aacc05a..ee560a4b4269 100644 >> --- a/util/module.c >> +++ b/util/module.c >> @@ -245,3 +245,58 @@ bool module_load_one(const char *prefix, const char *lib_name) >> #endif >> return success; >> } >> + >> +/* >> + * Building devices and other qom objects modular is mostly useful in >> + * case they have dependencies to external shared libraries, so we can >> + * cut down the core qemu library dependencies. Which is the case for >> + * only a very few devices & objects. >> + * >> + * So with the expectation that this will be rather the exception than >> + * to rule and the list will not gain that many entries go with a >> + * simple manually maintained list for now. >> + */ >> +static struct { >> + const char *type; >> + const char *prefix; >> + const char *module; >> +} const qom_modules[] = { >> +}; >> + >> +static bool module_loaded_qom_all; >> + >> +void module_load_qom_one(const char *type) >> +{ >> + int i; >> + >> + if (module_loaded_qom_all) { >> + return; >> + } >> + for (i = 0; i < ARRAY_SIZE(qom_modules); i++) { >> + if (strcmp(qom_modules[i].type, type) == 0) { >> + module_load_one(qom_modules[i].prefix, >> + qom_modules[i].module); >> + return; > > return value lost. > > Should we not at least trace something, warn something? > > Maybe we want to do it in module_load_one? > > >> + } >> + } >> +} >> + >> +void module_load_qom_all(void) >> +{ >> + int i; >> + >> + if (module_loaded_qom_all) { >> + return; >> + } >> + for (i = 0; i < ARRAY_SIZE(qom_modules); i++) { >> + if (i > 0 && (strcmp(qom_modules[i - 1].module, >> + qom_modules[i].module) == 0 && >> + strcmp(qom_modules[i - 1].prefix, >> + qom_modules[i].prefix) == 0)) { >> + /* one module implementing multiple types -> load only once */ >> + continue; >> + } >> + module_load_one(qom_modules[i].prefix, qom_modules[i].module); > > return value lost. > >> + } >> + module_loaded_qom_all = true; >> +} > > Pair this with the next commit, > > commit 0f8198f1b2f3c33df2381c412ad8d8fd219b90b2 ("object: qom module support") : > > which goes: > >> commit 0f8198f1b2f3c33df2381c412ad8d8fd219b90b2 >> Author: Gerd Hoffmann <kraxel@redhat.com> >> Date: Wed Jun 24 15:10:37 2020 +0200 >> >> object: qom module support >> >> 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> >> Message-id: 20200624131045.14512-3-kraxel@redhat.com >> >> diff --git a/include/qom/object.h b/include/qom/object.h >> index 94a61ccc3f..51f188137f 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 > > Note the use of the word "tries" in the comment. > >> + * 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 6ece96bc2b..34daaf1280 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); > > now here we fail during object_class_by_name() with oc that is NULL. So I guess we can evince that module_load_qom_one failed from here. > > >> + } >> +#endif >> + return oc; >> +} >> + >> ObjectClass *object_class_get_parent(ObjectClass *class) >> { >> TypeImpl *type = type_get_parent(class->type); > > > We should be able to tell if a load is successful or not I think, but before that, > > I think we are currently inconsistent, and do not report enough in terms of tracing and errors to the user > as to the reason that a module load fails. > > Either we add tracing to module_load_one, and make it so that module_load_one also returns void, > or we change module_load_qom_one to also return bool. > > Note, qdev also makes use of this facility. > > Thoughts? You are correct, module_load_qom_*() should return a boolean and ideally take an Error* parameter.
diff --git a/include/qemu/module.h b/include/qemu/module.h index 011ae1ae7605..9121a475c1b6 100644 --- a/include/qemu/module.h +++ b/include/qemu/module.h @@ -70,5 +70,7 @@ void register_dso_module_init(void (*fn)(void), module_init_type type); void module_call_init(module_init_type type); bool module_load_one(const char *prefix, const char *lib_name); +void module_load_qom_one(const char *type); +void module_load_qom_all(void); #endif diff --git a/util/module.c b/util/module.c index e48d9aacc05a..ee560a4b4269 100644 --- a/util/module.c +++ b/util/module.c @@ -245,3 +245,58 @@ bool module_load_one(const char *prefix, const char *lib_name) #endif return success; } + +/* + * Building devices and other qom objects modular is mostly useful in + * case they have dependencies to external shared libraries, so we can + * cut down the core qemu library dependencies. Which is the case for + * only a very few devices & objects. + * + * So with the expectation that this will be rather the exception than + * to rule and the list will not gain that many entries go with a + * simple manually maintained list for now. + */ +static struct { + const char *type; + const char *prefix; + const char *module; +} const qom_modules[] = { +}; + +static bool module_loaded_qom_all; + +void module_load_qom_one(const char *type) +{ + int i; + + if (module_loaded_qom_all) { + return; + } + for (i = 0; i < ARRAY_SIZE(qom_modules); i++) { + if (strcmp(qom_modules[i].type, type) == 0) { + module_load_one(qom_modules[i].prefix, + qom_modules[i].module); + return; + } + } +} + +void module_load_qom_all(void) +{ + int i; + + if (module_loaded_qom_all) { + return; + } + for (i = 0; i < ARRAY_SIZE(qom_modules); i++) { + if (i > 0 && (strcmp(qom_modules[i - 1].module, + qom_modules[i].module) == 0 && + strcmp(qom_modules[i - 1].prefix, + qom_modules[i].prefix) == 0)) { + /* one module implementing multiple types -> load only once */ + continue; + } + module_load_one(qom_modules[i].prefix, qom_modules[i].module); + } + module_loaded_qom_all = true; +}
Add support for qom types provided by modules. For starters use a manually maintained list which maps qom type to module and prefix. Two load functions are added: One to load the module for a specific type, and one to load all modules (needed for object/device lists as printed by -- for example -- qemu -device help). Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- include/qemu/module.h | 2 ++ util/module.c | 55 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+)