Patchwork [v10,5/8] module: implement module loading

login
register
mail settings
Submitter Fam Zheng
Date Sept. 16, 2013, 6:50 a.m.
Message ID <1379314227-8855-6-git-send-email-famz@redhat.com>
Download mbox | patch
Permalink /patch/275119/
State New
Headers show

Comments

Fam Zheng - Sept. 16, 2013, 6:50 a.m.
Added three types of modules:

    typedef enum {
        MODULE_LOAD_BLOCK = 0,
        MODULE_LOAD_UI,
        MODULE_LOAD_NET,
        MODULE_LOAD_MAX,
    } module_load_type;

and their loading function:

    void module_load(module_load_type).

which loads whitelisted ".so" files of the given type under ${MODDIR}.

Modules of each type should be loaded in respective subsystem
initialization code.

The init function of dynamic module is no longer with
__attribute__((constructor)) as static linked version, and need to be
explicitly called once loaded. The function name is mangled with per
configure fingerprint as:

    init_$(date +%s$$$RANDOM)

Which is known to module_load function, and the loading fails if this
symbol is not there. With this, modules built from a different
tree/version/configure will not be loaded.

The module loading code requires gmodule-2.0.

Configure option "--enable-modules=L" can be used to restrict qemu to
only build/load some whitelisted modules.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 Makefile              |  3 ++
 block.c               |  1 +
 configure             | 44 +++++++++++++++++++++-------
 include/qemu/module.h | 23 +++++++++++++++
 rules.mak             |  9 ++++--
 scripts/create_config | 22 ++++++++++++++
 util/module.c         | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++
 vl.c                  |  2 ++
 8 files changed, 172 insertions(+), 13 deletions(-)
Daniel P. Berrange - Sept. 16, 2013, 8:59 a.m.
On Mon, Sep 16, 2013 at 02:50:24PM +0800, Fam Zheng wrote:
> Added three types of modules:
> 
>     typedef enum {
>         MODULE_LOAD_BLOCK = 0,
>         MODULE_LOAD_UI,
>         MODULE_LOAD_NET,
>         MODULE_LOAD_MAX,
>     } module_load_type;
> 
> and their loading function:
> 
>     void module_load(module_load_type).
> 
> which loads whitelisted ".so" files of the given type under ${MODDIR}.
> 
> Modules of each type should be loaded in respective subsystem
> initialization code.
> 
> The init function of dynamic module is no longer with
> __attribute__((constructor)) as static linked version, and need to be
> explicitly called once loaded. The function name is mangled with per
> configure fingerprint as:
> 
>     init_$(date +%s$$$RANDOM)
> 
> Which is known to module_load function, and the loading fails if this
> symbol is not there. With this, modules built from a different
> tree/version/configure will not be loaded.
> 
> The module loading code requires gmodule-2.0.
> 
> Configure option "--enable-modules=L" can be used to restrict qemu to
> only build/load some whitelisted modules.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  Makefile              |  3 ++
>  block.c               |  1 +
>  configure             | 44 +++++++++++++++++++++-------
>  include/qemu/module.h | 23 +++++++++++++++
>  rules.mak             |  9 ++++--
>  scripts/create_config | 22 ++++++++++++++
>  util/module.c         | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  vl.c                  |  2 ++
>  8 files changed, 172 insertions(+), 13 deletions(-)
> 
> +void module_load(module_load_type type)
> +{
> +#ifdef CONFIG_MODULES
> +    const char *prefix;
> +    char *fname = NULL;
> +    const char **mp;
> +    static const char *module_whitelist[] = {
> +        CONFIG_MODULE_WHITELIST
> +    };
> +
> +    if (!g_module_supported()) {
> +        return;
> +    }
> +
> +    switch (type) {
> +    case MODULE_LOAD_BLOCK:
> +        prefix = "block-";
> +        break;
> +    case MODULE_LOAD_UI:
> +        prefix = "ui-";
> +        break;
> +    case MODULE_LOAD_NET:
> +        prefix = "ui-";

Wrong prefix.

> +        break;
> +    default:
> +        return;
> +    }
> +
> +    for (mp = &module_whitelist[0]; *mp; mp++) {
> +        if (strncmp(prefix, *mp, strlen(prefix))) {
> +            continue;
> +        }
> +        fname = g_strdup_printf("%s/%s%s", CONFIG_MODDIR, *mp, HOST_DSOSUF);
> +        module_load_file(fname);
> +        g_free(fname);
> +    }
> +
> +#endif
> +}

IMHO this method design is really crazy. If you want to have a
situation where you call module_load() multiple times, then
have separate whitelists for block/net/ui, instead of having
one global whitelist which you then have to load a subset of
each time.  Alternatively just call module_load() once and
give rid of these enums for loading block/net/ui separately.

Daniel
Fam Zheng - Sept. 16, 2013, 9:28 a.m.
On Mon, 09/16 09:59, Daniel P. Berrange wrote:
> On Mon, Sep 16, 2013 at 02:50:24PM +0800, Fam Zheng wrote:
> > Added three types of modules:
> > 
> >     typedef enum {
> >         MODULE_LOAD_BLOCK = 0,
> >         MODULE_LOAD_UI,
> >         MODULE_LOAD_NET,
> >         MODULE_LOAD_MAX,
> >     } module_load_type;
> > 
> > and their loading function:
> > 
> >     void module_load(module_load_type).
> > 
> > which loads whitelisted ".so" files of the given type under ${MODDIR}.
> > 
> > Modules of each type should be loaded in respective subsystem
> > initialization code.
> > 
> > The init function of dynamic module is no longer with
> > __attribute__((constructor)) as static linked version, and need to be
> > explicitly called once loaded. The function name is mangled with per
> > configure fingerprint as:
> > 
> >     init_$(date +%s$$$RANDOM)
> > 
> > Which is known to module_load function, and the loading fails if this
> > symbol is not there. With this, modules built from a different
> > tree/version/configure will not be loaded.
> > 
> > The module loading code requires gmodule-2.0.
> > 
> > Configure option "--enable-modules=L" can be used to restrict qemu to
> > only build/load some whitelisted modules.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  Makefile              |  3 ++
> >  block.c               |  1 +
> >  configure             | 44 +++++++++++++++++++++-------
> >  include/qemu/module.h | 23 +++++++++++++++
> >  rules.mak             |  9 ++++--
> >  scripts/create_config | 22 ++++++++++++++
> >  util/module.c         | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  vl.c                  |  2 ++
> >  8 files changed, 172 insertions(+), 13 deletions(-)
> > 
> > +void module_load(module_load_type type)
> > +{
> > +#ifdef CONFIG_MODULES
> > +    const char *prefix;
> > +    char *fname = NULL;
> > +    const char **mp;
> > +    static const char *module_whitelist[] = {
> > +        CONFIG_MODULE_WHITELIST
> > +    };
> > +
> > +    if (!g_module_supported()) {
> > +        return;
> > +    }
> > +
> > +    switch (type) {
> > +    case MODULE_LOAD_BLOCK:
> > +        prefix = "block-";
> > +        break;
> > +    case MODULE_LOAD_UI:
> > +        prefix = "ui-";
> > +        break;
> > +    case MODULE_LOAD_NET:
> > +        prefix = "ui-";
> 
> Wrong prefix.
> 
> > +        break;
> > +    default:
> > +        return;
> > +    }
> > +
> > +    for (mp = &module_whitelist[0]; *mp; mp++) {
> > +        if (strncmp(prefix, *mp, strlen(prefix))) {
> > +            continue;
> > +        }
> > +        fname = g_strdup_printf("%s/%s%s", CONFIG_MODDIR, *mp, HOST_DSOSUF);
> > +        module_load_file(fname);
> > +        g_free(fname);
> > +    }
> > +
> > +#endif
> > +}
> 
> IMHO this method design is really crazy. If you want to have a
> situation where you call module_load() multiple times, then
> have separate whitelists for block/net/ui, instead of having
> one global whitelist which you then have to load a subset of
> each time.  Alternatively just call module_load() once and
> give rid of these enums for loading block/net/ui separately.
> 

It's pretty clear that we have different subsets of modules to load for target
emulator and qemu-img, so not having enums is not working.

What's the disadvantage of this, and why are separate lists better?

Thanks,
Fam
Paolo Bonzini - Sept. 16, 2013, 9:44 a.m.
Il 16/09/2013 10:59, Daniel P. Berrange ha scritto:
>> The init function of dynamic module is no longer with
>> __attribute__((constructor)) as static linked version, and need to be
>> explicitly called once loaded. The function name is mangled with per
>> configure fingerprint as:
>> 
>>     init_$(date +%s$$$RANDOM)

Does this work for a module that calls module_init multiple times?

Paolo
Fam Zheng - Sept. 16, 2013, 9:51 a.m.
On Mon, 09/16 11:44, Paolo Bonzini wrote:
> Il 16/09/2013 10:59, Daniel P. Berrange ha scritto:
> >> The init function of dynamic module is no longer with
> >> __attribute__((constructor)) as static linked version, and need to be
> >> explicitly called once loaded. The function name is mangled with per
> >> configure fingerprint as:
> >> 
> >>     init_$(date +%s$$$RANDOM)
> 
> Does this work for a module that calls module_init multiple times?
> 
Why should a module calls module_init, instead of the main function?

This name is generated per "./configure", not per object or per make, so it's
essentially the same with any fixed function name, except for two objects built
from two different "./configure" (which is the purpose for the mangling here).

Does this answer your question?

Fam
Paolo Bonzini - Sept. 16, 2013, 10:09 a.m.
Il 16/09/2013 11:51, Fam Zheng ha scritto:
> On Mon, 09/16 11:44, Paolo Bonzini wrote:
>> Il 16/09/2013 10:59, Daniel P. Berrange ha scritto:
>>>> The init function of dynamic module is no longer with
>>>> __attribute__((constructor)) as static linked version, and need to be
>>>> explicitly called once loaded. The function name is mangled with per
>>>> configure fingerprint as:
>>>>
>>>>     init_$(date +%s$$$RANDOM)
>>
>> Does this work for a module that calls module_init multiple times?
>
> Why should a module calls module_init, instead of the main function?

I think you mean "why should a module calls register_module_init", and I
agree that with this patch a module will not call register_module_init.

But a module is still using the module_init macro.

With this patch, a module will not be able to use the module_init macro
twice.  I am not sure this is an acceptable limitation, especially if we
do not have a dependency system within modules and/or load them with
G_MODULE_LOCAL/RTLD_LOCAL.

Paolo

> This name is generated per "./configure", not per object or per make, so it's
> essentially the same with any fixed function name, except for two objects built
> from two different "./configure" (which is the purpose for the mangling here).
> 
> Does this answer your question?
> 
> Fam
>
Daniel P. Berrange - Sept. 16, 2013, 10:14 a.m.
On Mon, Sep 16, 2013 at 12:09:47PM +0200, Paolo Bonzini wrote:
> Il 16/09/2013 11:51, Fam Zheng ha scritto:
> > On Mon, 09/16 11:44, Paolo Bonzini wrote:
> >> Il 16/09/2013 10:59, Daniel P. Berrange ha scritto:
> >>>> The init function of dynamic module is no longer with
> >>>> __attribute__((constructor)) as static linked version, and need to be
> >>>> explicitly called once loaded. The function name is mangled with per
> >>>> configure fingerprint as:
> >>>>
> >>>>     init_$(date +%s$$$RANDOM)
> >>
> >> Does this work for a module that calls module_init multiple times?
> >
> > Why should a module calls module_init, instead of the main function?
> 
> I think you mean "why should a module calls register_module_init", and I
> agree that with this patch a module will not call register_module_init.
> 
> But a module is still using the module_init macro.
> 
> With this patch, a module will not be able to use the module_init macro
> twice.  I am not sure this is an acceptable limitation, especially if we
> do not have a dependency system within modules and/or load them with
> G_MODULE_LOCAL/RTLD_LOCAL.

Why would a module ever want to use the module_init macro twice ? IIUC
this function is supposed todo one-time initialization work for the .so
module. Surely any place where a module wanted to use module_init twice
could be solved by having that module put all its init logic into just
one function. So I'm not sure I see where the problem is.

Daniel
Paolo Bonzini - Sept. 16, 2013, 10:18 a.m.
Il 16/09/2013 12:14, Daniel P. Berrange ha scritto:
> On Mon, Sep 16, 2013 at 12:09:47PM +0200, Paolo Bonzini wrote:
>> Il 16/09/2013 11:51, Fam Zheng ha scritto:
>>> On Mon, 09/16 11:44, Paolo Bonzini wrote:
>>>> Il 16/09/2013 10:59, Daniel P. Berrange ha scritto:
>>>>>> The init function of dynamic module is no longer with
>>>>>> __attribute__((constructor)) as static linked version, and need to be
>>>>>> explicitly called once loaded. The function name is mangled with per
>>>>>> configure fingerprint as:
>>>>>>
>>>>>>     init_$(date +%s$$$RANDOM)
>>>>
>>>> Does this work for a module that calls module_init multiple times?
>>>
>>> Why should a module calls module_init, instead of the main function?
>>
>> I think you mean "why should a module calls register_module_init", and I
>> agree that with this patch a module will not call register_module_init.
>>
>> But a module is still using the module_init macro.
>>
>> With this patch, a module will not be able to use the module_init macro
>> twice.  I am not sure this is an acceptable limitation, especially if we
>> do not have a dependency system within modules and/or load them with
>> G_MODULE_LOCAL/RTLD_LOCAL.
> 
> Why would a module ever want to use the module_init macro twice ?

Because our coding standard is to have each source file do its own
one-time initialization, using static functions and an invocation of
module_init per source file.

The reason is that otherwise you risk having function name conflicts in
the static-link case.

Paolo

> IIUC
> this function is supposed todo one-time initialization work for the .so
> module. Surely any place where a module wanted to use module_init twice
> could be solved by having that module put all its init logic into just
> one function. So I'm not sure I see where the problem is.
Daniel P. Berrange - Sept. 16, 2013, 10:21 a.m.
On Mon, Sep 16, 2013 at 12:18:54PM +0200, Paolo Bonzini wrote:
> Il 16/09/2013 12:14, Daniel P. Berrange ha scritto:
> > On Mon, Sep 16, 2013 at 12:09:47PM +0200, Paolo Bonzini wrote:
> >> Il 16/09/2013 11:51, Fam Zheng ha scritto:
> >>> On Mon, 09/16 11:44, Paolo Bonzini wrote:
> >>>> Il 16/09/2013 10:59, Daniel P. Berrange ha scritto:
> >>>>>> The init function of dynamic module is no longer with
> >>>>>> __attribute__((constructor)) as static linked version, and need to be
> >>>>>> explicitly called once loaded. The function name is mangled with per
> >>>>>> configure fingerprint as:
> >>>>>>
> >>>>>>     init_$(date +%s$$$RANDOM)
> >>>>
> >>>> Does this work for a module that calls module_init multiple times?
> >>>
> >>> Why should a module calls module_init, instead of the main function?
> >>
> >> I think you mean "why should a module calls register_module_init", and I
> >> agree that with this patch a module will not call register_module_init.
> >>
> >> But a module is still using the module_init macro.
> >>
> >> With this patch, a module will not be able to use the module_init macro
> >> twice.  I am not sure this is an acceptable limitation, especially if we
> >> do not have a dependency system within modules and/or load them with
> >> G_MODULE_LOCAL/RTLD_LOCAL.
> > 
> > Why would a module ever want to use the module_init macro twice ?
> 
> Because our coding standard is to have each source file do its own
> one-time initialization, using static functions and an invocation of
> module_init per source file.

Is there ever a case where two source files, each using module_init
will be compiled into the same .so loadable module. Looking at the
uses of block_init(), I don't see any obvious candidates for trouble,
all uses look like they'd be going into separate .so files.

Daniel
Alex Bligh - Sept. 16, 2013, 10:24 a.m.
On 16 Sep 2013, at 10:51, Fam Zheng wrote:

> On Mon, 09/16 11:44, Paolo Bonzini wrote:
>> Il 16/09/2013 10:59, Daniel P. Berrange ha scritto:
>>>> The init function of dynamic module is no longer with
>>>> __attribute__((constructor)) as static linked version, and need to be
>>>> explicitly called once loaded. The function name is mangled with per
>>>> configure fingerprint as:
>>>> 
>>>>    init_$(date +%s$$$RANDOM)
>> 
>> Does this work for a module that calls module_init multiple times?
>> 
> Why should a module calls module_init, instead of the main function?
> 
> This name is generated per "./configure", not per object or per make, so it's
> essentially the same with any fixed function name, except for two objects built
> from two different "./configure" (which is the purpose for the mangling here).

I think I must be missing something here.

We do not have a stable API/ABI and it seems generally acknowledged at this
stage that we don't need one. Therefore, to avoid API/ABI mismatch between
the executables and the modules, in ./configure you are generating a random
cookie (effectively) that you are calling the fingerprint.

The executable will then not load the module unless the module has the
right cookie. As far as I can tell, that means the module needs to be
built within the same build harness as the executables, or it won't
know what to call its init function.

And that's perfectly compatible with the stated objective:
> The main idea behind modules is to isolate dependencies on third party
> libraries from qemu executables, such as libglusterfs or librbd, so that the
> end users can install core qemu package with fewer dependencies.  And only for
> those who want to use particular modules, need they install qemu-foo
> sub-package, which in turn requires libbar and libbiz packages.

... this being to isolate dependencies, and not to enable third party
modules built outside the tree.

That's all well and good, but if the modules are all built within the
same build harness, why do we need a whitelist or a readdir() at all? We
know what the modules are, because they were the ones that were built
at the same time. Why not just process the list of modules it was
built with, and if you get EEXIST, move on?

The other issue I have with this approach is that it doesn't address
the situation where the distro builds the qemu version against its
distributed version of the development library for the plug in, but
on deployment, everyone wants to use a new development library. To
give a concrete example, qemu in Ubuntu Precise is build against
librbd 0.42 (from memory - don't shoot me if wrong), and that supports
fewer calls than librbd 0.62 which is what anyone sane will want to
run.

At risk of heresy, can I suggest a rather simpler scheme that requires
a total of zero infrastructure changes?

Here's a patch against qemu 1.0 (sorry) Ubuntu dist (sorry) that
uses weak binding to load and compile against any version of
librbd:
  https://github.com/flexiant/qemu/commit/6fa2e9c95bdaca7c814881e27f04424fb6cc2960

This requires librbd-dev headers of some sort in order to build. But
librbd does not need to be there on a deployment, and indeed multiple
versions work (and the patch does different things depending on
librbd version).

This would seem to achieve the stated objective (qemu package can
be installed without librbd dependencies) without any reliance
on modules at all. So we could scrap the whole modules infrastructure
and just tell people developing third party libraries to use weak
binding. As you can see, for librbd anyway, the changes required
to support weak binding are pretty minimal.
Paolo Bonzini - Sept. 16, 2013, 10:30 a.m.
Il 16/09/2013 12:21, Daniel P. Berrange ha scritto:
> On Mon, Sep 16, 2013 at 12:18:54PM +0200, Paolo Bonzini wrote:
>> Il 16/09/2013 12:14, Daniel P. Berrange ha scritto:
>>> On Mon, Sep 16, 2013 at 12:09:47PM +0200, Paolo Bonzini wrote:
>>>> Il 16/09/2013 11:51, Fam Zheng ha scritto:
>>>>> On Mon, 09/16 11:44, Paolo Bonzini wrote:
>>>>>> Il 16/09/2013 10:59, Daniel P. Berrange ha scritto:
>>>>>>>> The init function of dynamic module is no longer with
>>>>>>>> __attribute__((constructor)) as static linked version, and need to be
>>>>>>>> explicitly called once loaded. The function name is mangled with per
>>>>>>>> configure fingerprint as:
>>>>>>>>
>>>>>>>>     init_$(date +%s$$$RANDOM)
>>>>>>
>>>>>> Does this work for a module that calls module_init multiple times?
>>>>>
>>>>> Why should a module calls module_init, instead of the main function?
>>>>
>>>> I think you mean "why should a module calls register_module_init", and I
>>>> agree that with this patch a module will not call register_module_init.
>>>>
>>>> But a module is still using the module_init macro.
>>>>
>>>> With this patch, a module will not be able to use the module_init macro
>>>> twice.  I am not sure this is an acceptable limitation, especially if we
>>>> do not have a dependency system within modules and/or load them with
>>>> G_MODULE_LOCAL/RTLD_LOCAL.
>>>
>>> Why would a module ever want to use the module_init macro twice ?
>>
>> Because our coding standard is to have each source file do its own
>> one-time initialization, using static functions and an invocation of
>> module_init per source file.
> 
> Is there ever a case where two source files, each using module_init
> will be compiled into the same .so loadable module. Looking at the
> uses of block_init(), I don't see any obvious candidates for trouble,
> all uses look like they'd be going into separate .so files.

Without inter-module exports, all of SPICE probably would have to be in
a single .so file.  This includes spice-qemu-char.c and
hw/display/qxl.c, both of which use type_init.

If we use G_MODULE_GLOBAL as a primitive system for intermodule exports,
then indeed this is a much smaller problem, but then we need a
dependency system.  But I'm almost sure that Windows and maybe Darwin
lack support for the equivalent of G_MODULE_GLOBAL.

BTW, we need a buildbot for the static linking case, otherwise as
modules become more widespread, we'll have hard to detect bugs due to
duplicate symbols.

Paolo
Paolo Bonzini - Sept. 16, 2013, 10:38 a.m.
Il 16/09/2013 12:24, Alex Bligh ha scritto:
> At risk of heresy, can I suggest a rather simpler scheme that requires
> a total of zero infrastructure changes?
> 
> Here's a patch against qemu 1.0 (sorry) Ubuntu dist (sorry) that
> uses weak binding to load and compile against any version of
> librbd:
>   https://github.com/flexiant/qemu/commit/6fa2e9c95bdaca7c814881e27f04424fb6cc2960
> 
> This requires librbd-dev headers of some sort in order to build. But
> librbd does not need to be there on a deployment, and indeed multiple
> versions work (and the patch does different things depending on
> librbd version).

No, librbd does need to be there for the other symbols that are not weak
(e.g. rbd_aio_read).  This approach cannot be "taken to the limit", i.e.
removing the librbd dependency altogether.  For example:

xx.c:
int f(void)
{
	return 42;
}

yy.c:
#pragma weak f
extern int f(void);
int main()
{
	printf("%p %d", f, f ? f(): 67);
}

$ gcc xx.c -shared -o xx.so
$ LD_RUN_PATH=$PWD gcc yy.c xx.so -o yy
$ ./yy
0x4005b0 42
$ rm xx.so
$ ./yy
./yy: error while loading shared libraries: xx.so: cannot open shared
object file: No such file or directory


Also, the code _is_ ugly.  Do it once and it's perhaps acceptable.  Do
it for libiscsi, librbd, libcurl, libssh2, SPICE, GTK+, SDL etc. and it
becomes unmaintainable.

Paolo

> This would seem to achieve the stated objective (qemu package can
> be installed without librbd dependencies) without any reliance
> on modules at all. So we could scrap the whole modules infrastructure
> and just tell people developing third party libraries to use weak
> binding. As you can see, for librbd anyway, the changes required
> to support weak binding are pretty minimal.
Fam Zheng - Sept. 16, 2013, 10:43 a.m.
On Mon, 09/16 11:24, Alex Bligh wrote:
> 
> On 16 Sep 2013, at 10:51, Fam Zheng wrote:
> 
> > On Mon, 09/16 11:44, Paolo Bonzini wrote:
> >> Il 16/09/2013 10:59, Daniel P. Berrange ha scritto:
> >>>> The init function of dynamic module is no longer with
> >>>> __attribute__((constructor)) as static linked version, and need to be
> >>>> explicitly called once loaded. The function name is mangled with per
> >>>> configure fingerprint as:
> >>>> 
> >>>>    init_$(date +%s$$$RANDOM)
> >> 
> >> Does this work for a module that calls module_init multiple times?
> >> 
> > Why should a module calls module_init, instead of the main function?
> > 
> > This name is generated per "./configure", not per object or per make, so it's
> > essentially the same with any fixed function name, except for two objects built
> > from two different "./configure" (which is the purpose for the mangling here).
> 
> I think I must be missing something here.
> 
> We do not have a stable API/ABI and it seems generally acknowledged at this
> stage that we don't need one. Therefore, to avoid API/ABI mismatch between
> the executables and the modules, in ./configure you are generating a random
> cookie (effectively) that you are calling the fingerprint.
> 
> The executable will then not load the module unless the module has the
> right cookie. As far as I can tell, that means the module needs to be
> built within the same build harness as the executables, or it won't
> know what to call its init function.
> 
> And that's perfectly compatible with the stated objective:
> > The main idea behind modules is to isolate dependencies on third party
> > libraries from qemu executables, such as libglusterfs or librbd, so that the
> > end users can install core qemu package with fewer dependencies.  And only for
> > those who want to use particular modules, need they install qemu-foo
> > sub-package, which in turn requires libbar and libbiz packages.
> 
> ... this being to isolate dependencies, and not to enable third party
> modules built outside the tree.
> 
> That's all well and good, but if the modules are all built within the
> same build harness, why do we need a whitelist or a readdir() at all? We
> know what the modules are, because they were the ones that were built
> at the same time. Why not just process the list of modules it was
> built with, and if you get EEXIST, move on?
> 

Sounds good to me, I agree that "whitelist" is not necessary for user. We still
need a known_modules in the new module_load() code and it'll look very similar
to current whitelist. But I'll change the name.

Fam
Gerd Hoffmann - Sept. 16, 2013, 10:57 a.m.
Hi,

> With this patch, a module will not be able to use the module_init macro
> twice.  I am not sure this is an acceptable limitation, especially if we
> do not have a dependency system within modules and/or load them with
> G_MODULE_LOCAL/RTLD_LOCAL.

Exactly.  To modularize spice we need either inter-module dependencies,
so spice-audio.mo can depend on spice-core.mo etc, or allow multiple
module_init calls so we can link all spice components into one big
spice.mo module and each component can use module_init.

It's not mandatory for the initial revision, we'll need a bit more (like
registering monitor commands for 'info spice') so we can actually
modularize spice.  But it should definitively on the radar for the
planning ...

cheers,
  Gerd
Paolo Bonzini - Sept. 16, 2013, 11 a.m.
Il 16/09/2013 12:57, Gerd Hoffmann ha scritto:
>   Hi,
> 
>> With this patch, a module will not be able to use the module_init macro
>> twice.  I am not sure this is an acceptable limitation, especially if we
>> do not have a dependency system within modules and/or load them with
>> G_MODULE_LOCAL/RTLD_LOCAL.
> 
> Exactly.  To modularize spice we need either inter-module dependencies,
> so spice-audio.mo can depend on spice-core.mo etc, or allow multiple
> module_init calls so we can link all spice components into one big
> spice.mo module and each component can use module_init.

We could also have a huge web of shared objects like LibreOffice has
(spice-core.so depending on qemu-system.so, and spice.mo depending on
spice-core.so), but I'm not really suggesting that...

Paolo

> It's not mandatory for the initial revision, we'll need a bit more (like
> registering monitor commands for 'info spice') so we can actually
> modularize spice.  But it should definitively on the radar for the
> planning ...
> 
> cheers,
>   Gerd
> 
> 
>
Alex Bligh - Sept. 16, 2013, 11 a.m.
On 16 Sep 2013, at 11:38, Paolo Bonzini wrote:

> No, librbd does need to be there for the other symbols that are not weak
> (e.g. rbd_aio_read).  This approach cannot be "taken to the limit", i.e.
> removing the librbd dependency altogether.  For example:
> 
> xx.c:
> int f(void)
> {
> 	return 42;
> }
> 
> yy.c:
> #pragma weak f
> extern int f(void);
> int main()
> {
> 	printf("%p %d", f, f ? f(): 67);
> }
> 
> $ gcc xx.c -shared -o xx.so
> $ LD_RUN_PATH=$PWD gcc yy.c xx.so -o yy
> $ ./yy
> 0x4005b0 42
> $ rm xx.so
> $ ./yy
> ./yy: error while loading shared libraries: xx.so: cannot open shared
> object file: No such file or directory

I think you need to wrap f, i.e. take g as a pointer to f(), and
call g().

> Also, the code _is_ ugly.  Do it once and it's perhaps acceptable.  Do
> it for libiscsi, librbd, libcurl, libssh2, SPICE, GTK+, SDL etc. and it
> becomes unmaintainable.

I agree it's ugly. However, it's pretty much the only way to cope
with different versions of libraries.

However, even if you don't use weak symbols, we could simply dlopen()
a fixed list of modules known at compile time from a single directory
(because we also know at compile which executable needs what, e.g.
that qemu-img doesn't need spice or whatever).
Daniel P. Berrange - Sept. 16, 2013, 11:04 a.m.
On Mon, Sep 16, 2013 at 12:00:47PM +0100, Alex Bligh wrote:
> 
> However, even if you don't use weak symbols, we could simply dlopen()
> a fixed list of modules known at compile time from a single directory
> (because we also know at compile which executable needs what, e.g.
> that qemu-img doesn't need spice or whatever).

That's what this latest patch series already does, albeit with some
redundant complexity of trying to split the modules into UI/Net/Block
lists.

Daniel
Daniel P. Berrange - Sept. 16, 2013, 11:05 a.m.
On Mon, Sep 16, 2013 at 02:50:24PM +0800, Fam Zheng wrote:
> Added three types of modules:
> 
>     typedef enum {
>         MODULE_LOAD_BLOCK = 0,
>         MODULE_LOAD_UI,
>         MODULE_LOAD_NET,
>         MODULE_LOAD_MAX,
>     } module_load_type;
> 
> and their loading function:
> 
>     void module_load(module_load_type).
> 
> which loads whitelisted ".so" files of the given type under ${MODDIR}.
> 
> Modules of each type should be loaded in respective subsystem
> initialization code.

Based on Paolo's note that the SPICE .so module could likely
end up containing functionality that is spread across several
different sub-systems, this approach of loading per-type
seems even more flawed. I think I'd just have one flat list
of modules to load and ditch these  MODULE_LOAD_XXXX enums.

Daniel
Paolo Bonzini - Sept. 16, 2013, 11:08 a.m.
Il 16/09/2013 13:00, Alex Bligh ha scritto:
> 
> On 16 Sep 2013, at 11:38, Paolo Bonzini wrote:
> 
>> No, librbd does need to be there for the other symbols that are not weak
>> (e.g. rbd_aio_read).  This approach cannot be "taken to the limit", i.e.
>> removing the librbd dependency altogether.  For example:
>>
>> xx.c:
>> int f(void)
>> {
>> 	return 42;
>> }
>>
>> yy.c:
>> #pragma weak f
>> extern int f(void);
>> int main()
>> {
>> 	printf("%p %d", f, f ? f(): 67);
>> }
>>
>> $ gcc xx.c -shared -o xx.so
>> $ LD_RUN_PATH=$PWD gcc yy.c xx.so -o yy
>> $ ./yy
>> 0x4005b0 42
>> $ rm xx.so
>> $ ./yy
>> ./yy: error while loading shared libraries: xx.so: cannot open shared
>> object file: No such file or directory
> 
> I think you need to wrap f, i.e. take g as a pointer to f(), and
> call g().

No, you need to do dlopen("librbd.so"), which is bad because then
distros that can track .so dependencies will not do it anymore.
qemu-devel had a patch to do exactly that.

>> Also, the code _is_ ugly.  Do it once and it's perhaps acceptable.  Do
>> it for libiscsi, librbd, libcurl, libssh2, SPICE, GTK+, SDL etc. and it
>> becomes unmaintainable.
> 
> I agree it's ugly. However, it's pretty much the only way to cope
> with different versions of libraries.

But the reason to do modularization is not to "cope with different
versions of libraries".  In fact that's a problem that Fam's patches do
not solve at all.  The reason to do modularization is to make libraries
optional, i.e. let them be completely absent.

Paolo
Alex Bligh - Sept. 16, 2013, 11:27 a.m.
On 16 Sep 2013, at 12:04, Daniel P. Berrange wrote:

> On Mon, Sep 16, 2013 at 12:00:47PM +0100, Alex Bligh wrote:
>> 
>> However, even if you don't use weak symbols, we could simply dlopen()
>> a fixed list of modules known at compile time from a single directory
>> (because we also know at compile which executable needs what, e.g.
>> that qemu-img doesn't need spice or whatever).
> 
> That's what this latest patch series already does, albeit with some
> redundant complexity of trying to split the modules into UI/Net/Block
> lists.

Last time I looked (sorry if I'm out of date), it had whitelists
(which implies you can load things other than on the whitelists),
nested directories (as above), and readdir(). None of these are
necessary if the complete list of modules is known at build time.
And using the mangling method we're using at the moment, no
modules can be built other than at build time.
Fam Zheng - Sept. 16, 2013, 11:29 a.m.
On Mon, 09/16 12:30, Paolo Bonzini wrote:
> Il 16/09/2013 12:21, Daniel P. Berrange ha scritto:
> > On Mon, Sep 16, 2013 at 12:18:54PM +0200, Paolo Bonzini wrote:
> >> Il 16/09/2013 12:14, Daniel P. Berrange ha scritto:
> >>> On Mon, Sep 16, 2013 at 12:09:47PM +0200, Paolo Bonzini wrote:
> >>>> Il 16/09/2013 11:51, Fam Zheng ha scritto:
> >>>>> On Mon, 09/16 11:44, Paolo Bonzini wrote:
> >>>>>> Il 16/09/2013 10:59, Daniel P. Berrange ha scritto:
> >>>>>>>> The init function of dynamic module is no longer with
> >>>>>>>> __attribute__((constructor)) as static linked version, and need to be
> >>>>>>>> explicitly called once loaded. The function name is mangled with per
> >>>>>>>> configure fingerprint as:
> >>>>>>>>
> >>>>>>>>     init_$(date +%s$$$RANDOM)
> >>>>>>
> >>>>>> Does this work for a module that calls module_init multiple times?
> >>>>>
> >>>>> Why should a module calls module_init, instead of the main function?
> >>>>
> >>>> I think you mean "why should a module calls register_module_init", and I
> >>>> agree that with this patch a module will not call register_module_init.
> >>>>
> >>>> But a module is still using the module_init macro.
> >>>>
> >>>> With this patch, a module will not be able to use the module_init macro
> >>>> twice.  I am not sure this is an acceptable limitation, especially if we
> >>>> do not have a dependency system within modules and/or load them with
> >>>> G_MODULE_LOCAL/RTLD_LOCAL.
> >>>
> >>> Why would a module ever want to use the module_init macro twice ?
> >>
> >> Because our coding standard is to have each source file do its own
> >> one-time initialization, using static functions and an invocation of
> >> module_init per source file.
> > 
> > Is there ever a case where two source files, each using module_init
> > will be compiled into the same .so loadable module. Looking at the
> > uses of block_init(), I don't see any obvious candidates for trouble,
> > all uses look like they'd be going into separate .so files.
> 
> Without inter-module exports, all of SPICE probably would have to be in
> a single .so file.  This includes spice-qemu-char.c and
> hw/display/qxl.c, both of which use type_init.
> 
> If we use G_MODULE_GLOBAL as a primitive system for intermodule exports,
> then indeed this is a much smaller problem, but then we need a
> dependency system.  But I'm almost sure that Windows and maybe Darwin
> lack support for the equivalent of G_MODULE_GLOBAL.
> 

An idea for single .so file:
    - before loads a .so, an empty initializer list is created.
    - module_init adds a __attribute__((constructor)) function, which appends
      its real initializer to the initializer list. So this function is
      automatically called after dlopen().
    - make init_$(date +%s$$$RANDOM) a dummy symbol.
    - module_load first checks the presense of the symbol, if yes, call the
      functions in the initializer list. Else clean up and unload .so.

Does this enable multiple calls of module_init()?

OTOH. As for multiple spice modules, is it possible to solve it by having a
spice-common.o and link all spice modules to it, to share code?

Fam
Alex Bligh - Sept. 16, 2013, 11:30 a.m.
On 16 Sep 2013, at 12:08, Paolo Bonzini wrote:

> But the reason to do modularization is not to "cope with different
> versions of libraries".  In fact that's a problem that Fam's patches do
> not solve at all.  The reason to do modularization is to make libraries
> optional, i.e. let them be completely absent.

You are right. In fact the 'different versions of libraries' thing is
largely an orthogonal issue as a modular or non-modualar rbd driver
would have much the same set of issues here.

Moreover, one Fam's patches (hopefully simplified) could still help
here by permitting (e.g.) a blkrbd-new.so & blkrbd-old.so (hopefully
with more useful names), for those with an allergy to weak binding.
Paolo Bonzini - Sept. 16, 2013, 11:33 a.m.
Il 16/09/2013 13:29, Fam Zheng ha scritto:
> An idea for single .so file:
>     - before loads a .so, an empty initializer list is created.
>     - module_init adds a __attribute__((constructor)) function, which appends
>       its real initializer to the initializer list. So this function is
>       automatically called after dlopen().
>     - make init_$(date +%s$$$RANDOM) a dummy symbol.
>     - module_load first checks the presense of the symbol, if yes, call the
>       functions in the initializer list. Else clean up and unload .so.
> 
> Does this enable multiple calls of module_init()?

Yes.  Basically you are delaying the constructors; that would work.

> OTOH. As for multiple spice modules, is it possible to solve it by having a
> spice-common.o and link all spice modules to it, to share code?

Looks like there is global state in ui/spice-core.c, so likely no.

Paolo
Fam Zheng - Sept. 16, 2013, 11:46 a.m.
On Mon, 09/16 13:33, Paolo Bonzini wrote:
> Il 16/09/2013 13:29, Fam Zheng ha scritto:
> > An idea for single .so file:
> >     - before loads a .so, an empty initializer list is created.
> >     - module_init adds a __attribute__((constructor)) function, which appends
> >       its real initializer to the initializer list. So this function is
> >       automatically called after dlopen().
> >     - make init_$(date +%s$$$RANDOM) a dummy symbol.
> >     - module_load first checks the presense of the symbol, if yes, call the
> >       functions in the initializer list. Else clean up and unload .so.
> > 
> > Does this enable multiple calls of module_init()?
> 
> Yes.  Basically you are delaying the constructors; that would work.
> 
> > OTOH. As for multiple spice modules, is it possible to solve it by having a
> > spice-common.o and link all spice modules to it, to share code?
> 
> Looks like there is global state in ui/spice-core.c, so likely no.
> 

What if the modules can be loaded by name? Then in spice-qemu-char.so, it can
call require_module("spice-core") before initializing itself, which will load
this dependency if not yet.  This may be the simplest implementation of
dependency resolving.

Fam
Gerd Hoffmann - Sept. 16, 2013, 12:36 p.m.
On Mo, 2013-09-16 at 12:05 +0100, Daniel P. Berrange wrote:
> On Mon, Sep 16, 2013 at 02:50:24PM +0800, Fam Zheng wrote:
> > Added three types of modules:
> > 
> >     typedef enum {
> >         MODULE_LOAD_BLOCK = 0,
> >         MODULE_LOAD_UI,
> >         MODULE_LOAD_NET,
> >         MODULE_LOAD_MAX,
> >     } module_load_type;
> > 
> > and their loading function:
> > 
> >     void module_load(module_load_type).
> > 
> > which loads whitelisted ".so" files of the given type under ${MODDIR}.
> > 
> > Modules of each type should be loaded in respective subsystem
> > initialization code.
> 
> Based on Paolo's note that the SPICE .so module could likely
> end up containing functionality that is spread across several
> different sub-systems, this approach of loading per-type
> seems even more flawed.

spice would need different types indeed (and the list above looks
incomplete).

> I think I'd just have one flat list
> of modules to load and ditch these  MODULE_LOAD_XXXX enums.

Question is how to deal with qemu vs. qemu-img then.  qemu needs
everything and qemu-img needs the block drivers only (and loading
something else probably doesn't work due to unresolved symbols).

cheers,
  Gerd
Richard Henderson - Sept. 16, 2013, 10:16 p.m.
On 09/16/2013 02:28 AM, Fam Zheng wrote:
> What's the disadvantage of this, and why are separate lists better?

You're performing useless work, making the code more confusing in the process.
 How can it not be better to have separate lists?


r~
Richard Henderson - Sept. 16, 2013, 10:31 p.m.
On 09/16/2013 04:46 AM, Fam Zheng wrote:
> On Mon, 09/16 13:33, Paolo Bonzini wrote:
>> Il 16/09/2013 13:29, Fam Zheng ha scritto:
>>> An idea for single .so file:
>>>     - before loads a .so, an empty initializer list is created.
>>>     - module_init adds a __attribute__((constructor)) function, which appends
>>>       its real initializer to the initializer list. So this function is
>>>       automatically called after dlopen().
>>>     - make init_$(date +%s$$$RANDOM) a dummy symbol.
>>>     - module_load first checks the presense of the symbol, if yes, call the
>>>       functions in the initializer list. Else clean up and unload .so.
>>>
>>> Does this enable multiple calls of module_init()?
>>
>> Yes.  Basically you are delaying the constructors; that would work.
>>
>>> OTOH. As for multiple spice modules, is it possible to solve it by having a
>>> spice-common.o and link all spice modules to it, to share code?
>>
>> Looks like there is global state in ui/spice-core.c, so likely no.
>>
> 
> What if the modules can be loaded by name? Then in spice-qemu-char.so, it can
> call require_module("spice-core") before initializing itself, which will load
> this dependency if not yet.  This may be the simplest implementation of
> dependency resolving.

Why oh why would you want to re-invent the dependencies that ld.so already
provides?

Link spice-qemu.char.so against spice-core.so.  The DT_NEEDED entry will be
recorded, and ld.so will do the right thing.

Anything else sounds way too much like Not Invented Here.


r~
Richard Henderson - Sept. 16, 2013, 10:38 p.m.
On 09/16/2013 04:00 AM, Paolo Bonzini wrote:
> We could also have a huge web of shared objects like LibreOffice has
> (spice-core.so depending on qemu-system.so, and spice.mo depending on
> spice-core.so), but I'm not really suggesting that...

I am.  Although in our case I wouldn't expect the web to be huge.

To me it seems like working with the system dynamic linker is easier than
re-inventing our own.


r~
Fam Zheng - Sept. 17, 2013, 12:47 a.m.
On Mon, 09/16 15:16, Richard Henderson wrote:
> On 09/16/2013 02:28 AM, Fam Zheng wrote:
> > What's the disadvantage of this, and why are separate lists better?
> 
> You're performing useless work, making the code more confusing in the process.
>  How can it not be better to have separate lists?
> 
> 
Because it's about adding more code in Makefile to separate them and adding
more code here to define and use multiple lists. I am not sure it's much more
readable than this, but it's really non critical question, let me first try to
do it.

Thanks,

Fam
Fam Zheng - Sept. 17, 2013, 1:29 a.m.
On Mon, 09/16 15:31, Richard Henderson wrote:
> On 09/16/2013 04:46 AM, Fam Zheng wrote:
> > On Mon, 09/16 13:33, Paolo Bonzini wrote:
> >> Il 16/09/2013 13:29, Fam Zheng ha scritto:
> >>> An idea for single .so file:
> >>>     - before loads a .so, an empty initializer list is created.
> >>>     - module_init adds a __attribute__((constructor)) function, which appends
> >>>       its real initializer to the initializer list. So this function is
> >>>       automatically called after dlopen().
> >>>     - make init_$(date +%s$$$RANDOM) a dummy symbol.
> >>>     - module_load first checks the presense of the symbol, if yes, call the
> >>>       functions in the initializer list. Else clean up and unload .so.
> >>>
> >>> Does this enable multiple calls of module_init()?
> >>
> >> Yes.  Basically you are delaying the constructors; that would work.
> >>
> >>> OTOH. As for multiple spice modules, is it possible to solve it by having a
> >>> spice-common.o and link all spice modules to it, to share code?
> >>
> >> Looks like there is global state in ui/spice-core.c, so likely no.
> >>
> > 
> > What if the modules can be loaded by name? Then in spice-qemu-char.so, it can
> > call require_module("spice-core") before initializing itself, which will load
> > this dependency if not yet.  This may be the simplest implementation of
> > dependency resolving.
> 
> Why oh why would you want to re-invent the dependencies that ld.so already
> provides?
> 
> Link spice-qemu.char.so against spice-core.so.  The DT_NEEDED entry will be
> recorded, and ld.so will do the right thing.
> 
> Anything else sounds way too much like Not Invented Here.
> 
How to do the symbol checking as above if spice-core.so is automatically loaded
by ld.so?

And we will need to add $moddir to LD_LIBRARY_PATH and lose the restriction of
modules directory too.

Fam
Richard Henderson - Sept. 17, 2013, 5:40 a.m.
On 09/16/2013 06:29 PM, Fam Zheng wrote:
>> Link spice-qemu.char.so against spice-core.so.  The DT_NEEDED entry will be
>> recorded, and ld.so will do the right thing.
>>
>> Anything else sounds way too much like Not Invented Here.
>>
> How to do the symbol checking as above if spice-core.so is automatically loaded
> by ld.so?

You're checking the version stamp in spice-char.so.  I'd think that's good
enough.  No need to transitively check.

> And we will need to add $moddir to LD_LIBRARY_PATH and lose the restriction of
> modules directory too.

Or add DT_RUN_PATH to the main executable, or even add DT_RUN_PATH to the
module itself.  In particular, link the module with

  -Wl,--enable-new-dtags -Wl,-rpath,'$ORIGIN'

and dependencies for the module will automatically be looked for in the
directory in which the module is found.  Which is almost certiainly the only
thing that we want -- all modules in the same directory.


r~
Fam Zheng - Sept. 17, 2013, 5:55 a.m.
On Mon, 09/16 14:36, Gerd Hoffmann wrote:
> On Mo, 2013-09-16 at 12:05 +0100, Daniel P. Berrange wrote:
> > On Mon, Sep 16, 2013 at 02:50:24PM +0800, Fam Zheng wrote:
> > > Added three types of modules:
> > > 
> > >     typedef enum {
> > >         MODULE_LOAD_BLOCK = 0,
> > >         MODULE_LOAD_UI,
> > >         MODULE_LOAD_NET,
> > >         MODULE_LOAD_MAX,
> > >     } module_load_type;
> > > 
> > > and their loading function:
> > > 
> > >     void module_load(module_load_type).
> > > 
> > > which loads whitelisted ".so" files of the given type under ${MODDIR}.
> > > 
> > > Modules of each type should be loaded in respective subsystem
> > > initialization code.
> > 
> > Based on Paolo's note that the SPICE .so module could likely
> > end up containing functionality that is spread across several
> > different sub-systems, this approach of loading per-type
> > seems even more flawed.
> 
> spice would need different types indeed (and the list above looks
> incomplete).
> 
> > I think I'd just have one flat list
> > of modules to load and ditch these  MODULE_LOAD_XXXX enums.
> 
> Question is how to deal with qemu vs. qemu-img then.  qemu needs
> everything and qemu-img needs the block drivers only (and loading
> something else probably doesn't work due to unresolved symbols).
> 

With lazy symbol binding (G_MODULE_BIND_LAZY), we can just load all the
modules, and wait for subsystem to call module_call_init(MODULE_INIT_*), where
the symbols are resolved. As qemu-img.c doesn't init ui, net, it's not a
problem to load them ahead.

Fam
Alex Bligh - Sept. 17, 2013, 6:33 a.m.
On 17 Sep 2013, at 06:55, Fam Zheng wrote:

>>> 
>>> I think I'd just have one flat list
>>> of modules to load and ditch these  MODULE_LOAD_XXXX enums.
>> 
>> Question is how to deal with qemu vs. qemu-img then.  qemu needs
>> everything and qemu-img needs the block drivers only (and loading
>> something else probably doesn't work due to unresolved symbols).
>> 
> 
> With lazy symbol binding (G_MODULE_BIND_LAZY), we can just load all the
> modules, and wait for subsystem to call module_call_init(MODULE_INIT_*), where
> the symbols are resolved. As qemu-img.c doesn't init ui, net, it's not a
> problem to load them ahead.

Why not have one list per executable you build? You know at compile
time what modules each executable could load. This seems better
than putting the logic in and run time and having different types
of modules etc.

One reason to avoid qemu-img (for instance) loading everything (if
it's present) is init time. I agree dlopen()'ing something that
never gets called should not eat too much RAM but it seems pointless.
Fam Zheng - Sept. 17, 2013, 6:40 a.m.
On Tue, 09/17 07:33, Alex Bligh wrote:
> 
> On 17 Sep 2013, at 06:55, Fam Zheng wrote:
> 
> >>> 
> >>> I think I'd just have one flat list
> >>> of modules to load and ditch these  MODULE_LOAD_XXXX enums.
> >> 
> >> Question is how to deal with qemu vs. qemu-img then.  qemu needs
> >> everything and qemu-img needs the block drivers only (and loading
> >> something else probably doesn't work due to unresolved symbols).
> >> 
> > 
> > With lazy symbol binding (G_MODULE_BIND_LAZY), we can just load all the
> > modules, and wait for subsystem to call module_call_init(MODULE_INIT_*), where
> > the symbols are resolved. As qemu-img.c doesn't init ui, net, it's not a
> > problem to load them ahead.
> 
> Why not have one list per executable you build? You know at compile
> time what modules each executable could load. This seems better
> than putting the logic in and run time and having different types
> of modules etc.
> 
> One reason to avoid qemu-img (for instance) loading everything (if
> it's present) is init time. I agree dlopen()'ing something that
> never gets called should not eat too much RAM but it seems pointless.
> 

OK, good point.

Fam
Wayne Xia - Sept. 17, 2013, 8:26 a.m.
于 2013/9/16 18:38, Paolo Bonzini 写道:
> Il 16/09/2013 12:24, Alex Bligh ha scritto:
>> At risk of heresy, can I suggest a rather simpler scheme that requires
>> a total of zero infrastructure changes?
>>
>> Here's a patch against qemu 1.0 (sorry) Ubuntu dist (sorry) that
>> uses weak binding to load and compile against any version of
>> librbd:
>>    https://github.com/flexiant/qemu/commit/6fa2e9c95bdaca7c814881e27f04424fb6cc2960
>>
>> This requires librbd-dev headers of some sort in order to build. But
>> librbd does not need to be there on a deployment, and indeed multiple
>> versions work (and the patch does different things depending on
>> librbd version).
> No, librbd does need to be there for the other symbols that are not weak
> (e.g. rbd_aio_read).  This approach cannot be "taken to the limit", i.e.
> removing the librbd dependency altogether.  For example:
>
> xx.c:
> int f(void)
> {
> 	return 42;
> }
>
> yy.c:
> #pragma weak f
> extern int f(void);
> int main()
> {
> 	printf("%p %d", f, f ? f(): 67);
> }
>
> $ gcc xx.c -shared -o xx.so
> $ LD_RUN_PATH=$PWD gcc yy.c xx.so -o yy
> $ ./yy
> 0x4005b0 42
> $ rm xx.so
> $ ./yy
> ./yy: error while loading shared libraries: xx.so: cannot open shared
> object file: No such file or directory
>
Is there a way to tell the program loader: don't resolve the dynamic 
symbol, f,
then code can run into main(), and we add code do 
dlopen("/another_dir/xx.so")? (Assume xx.so
exist in /another_dir/?

> Also, the code _is_ ugly.  Do it once and it's perhaps acceptable.  Do
> it for libiscsi, librbd, libcurl, libssh2, SPICE, GTK+, SDL etc. and it
> becomes unmaintainable.
>
> Paolo
>
>> This would seem to achieve the stated objective (qemu package can
>> be installed without librbd dependencies) without any reliance
>> on modules at all. So we could scrap the whole modules infrastructure
>> and just tell people developing third party libraries to use weak
>> binding. As you can see, for librbd anyway, the changes required
>> to support weak binding are pretty minimal.
>
>
Wayne Xia - Sept. 17, 2013, 8:50 a.m.
于 2013/9/16 19:29, Fam Zheng 写道:
> On Mon, 09/16 12:30, Paolo Bonzini wrote:
>> Il 16/09/2013 12:21, Daniel P. Berrange ha scritto:
>>> On Mon, Sep 16, 2013 at 12:18:54PM +0200, Paolo Bonzini wrote:
>>>> Il 16/09/2013 12:14, Daniel P. Berrange ha scritto:
>>>>> On Mon, Sep 16, 2013 at 12:09:47PM +0200, Paolo Bonzini wrote:
>>>>>> Il 16/09/2013 11:51, Fam Zheng ha scritto:
>>>>>>> On Mon, 09/16 11:44, Paolo Bonzini wrote:
>>>>>>>> Il 16/09/2013 10:59, Daniel P. Berrange ha scritto:
>>>>>>>>>> The init function of dynamic module is no longer with
>>>>>>>>>> __attribute__((constructor)) as static linked version, and need to be
>>>>>>>>>> explicitly called once loaded. The function name is mangled with per
>>>>>>>>>> configure fingerprint as:
>>>>>>>>>>
>>>>>>>>>>      init_$(date +%s$$$RANDOM)
>>>>>>>> Does this work for a module that calls module_init multiple times?
>>>>>>> Why should a module calls module_init, instead of the main function?
>>>>>> I think you mean "why should a module calls register_module_init", and I
>>>>>> agree that with this patch a module will not call register_module_init.
>>>>>>
>>>>>> But a module is still using the module_init macro.
>>>>>>
>>>>>> With this patch, a module will not be able to use the module_init macro
>>>>>> twice.  I am not sure this is an acceptable limitation, especially if we
>>>>>> do not have a dependency system within modules and/or load them with
>>>>>> G_MODULE_LOCAL/RTLD_LOCAL.
>>>>> Why would a module ever want to use the module_init macro twice ?
>>>> Because our coding standard is to have each source file do its own
>>>> one-time initialization, using static functions and an invocation of
>>>> module_init per source file.
>>> Is there ever a case where two source files, each using module_init
>>> will be compiled into the same .so loadable module. Looking at the
>>> uses of block_init(), I don't see any obvious candidates for trouble,
>>> all uses look like they'd be going into separate .so files.
>> Without inter-module exports, all of SPICE probably would have to be in
>> a single .so file.  This includes spice-qemu-char.c and
>> hw/display/qxl.c, both of which use type_init.
>>
>> If we use G_MODULE_GLOBAL as a primitive system for intermodule exports,
>> then indeed this is a much smaller problem, but then we need a
>> dependency system.  But I'm almost sure that Windows and maybe Darwin
>> lack support for the equivalent of G_MODULE_GLOBAL.
>>
> An idea for single .so file:
>      - before loads a .so, an empty initializer list is created.
>      - module_init adds a __attribute__((constructor)) function, which appends
>        its real initializer to the initializer list. So this function is
>        automatically called after dlopen().
>      - make init_$(date +%s$$$RANDOM) a dummy symbol.
>      - module_load first checks the presense of the symbol, if yes, call the
>        functions in the initializer list. Else clean up and unload .so.
>
> Does this enable multiple calls of module_init()?
>
I like this way since it keeps the old init behavior which delayed the 
work with a list.

> OTOH. As for multiple spice modules, is it possible to solve it by having a
> spice-common.o and link all spice modules to it, to share code?
>
> Fam
>
Paolo Bonzini - Sept. 18, 2013, 11:45 a.m.
Il 17/09/2013 07:40, Richard Henderson ha scritto:
> On 09/16/2013 06:29 PM, Fam Zheng wrote:
>>> Link spice-qemu.char.so against spice-core.so.  The DT_NEEDED entry will be
>>> recorded, and ld.so will do the right thing.
>>>
>>> Anything else sounds way too much like Not Invented Here.
>>>
>> How to do the symbol checking as above if spice-core.so is automatically loaded
>> by ld.so?
> 
> You're checking the version stamp in spice-char.so.  I'd think that's good
> enough.  No need to transitively check.
> 
>> And we will need to add $moddir to LD_LIBRARY_PATH and lose the restriction of
>> modules directory too.
> 
> Or add DT_RUN_PATH to the main executable, or even add DT_RUN_PATH to the
> module itself.  In particular, link the module with
> 
>   -Wl,--enable-new-dtags -Wl,-rpath,'$ORIGIN'
> 
> and dependencies for the module will automatically be looked for in the
> directory in which the module is found.  Which is almost certiainly the only
> thing that we want -- all modules in the same directory.

This is not portable.

It looks like it can be emulated on Windows using LoadLibraryEx with the
LOAD_WITH_ALTERED_SEARCH_PATH flag and passing an absolute path to the
shared library.

But on Mac OS X a bundle (module loaded with dlopen) cannot have a
dependency on another bundle, afaik.

Paolo
Richard Henderson - Sept. 18, 2013, 2:44 p.m.
On 09/18/2013 04:45 AM, Paolo Bonzini wrote:
> But on Mac OS X a bundle (module loaded with dlopen) cannot have a
> dependency on another bundle, afaik.

The documentation for NSAddImage suggests that they can:

> NSADDIMAGE_OPTION_WITH_SEARCHING
> With this option, the image_name passed for the library and all its
> dependents is affected by the various dyld environment variables as if this
> library were linked into the program.

Note "and all its dependents".

As for controlling the search path... from C it looks like we might be
limited to the default $LD_LIBRARY_PATH, $DYLD_LIBRARY_PATH, current working
directory, $DYLD_FALLBACK_LIBRARY_PATH" path.

It would be interesting to know when dyld reads those environment variables:
at startup only, or could we control them from main?

Failing that, it appears that explicit control over search paths can be had
from Objective C, via the NSBundle class.

Something for someone who actually cares about macosx to work on.

But you're absolutely right that we can't just whack those elf parameters into
the makefile like that.


r~
Paolo Bonzini - Sept. 18, 2013, 3 p.m.
Il 18/09/2013 16:44, Richard Henderson ha scritto:
> On 09/18/2013 04:45 AM, Paolo Bonzini wrote:
>> But on Mac OS X a bundle (module loaded with dlopen) cannot have a
>> dependency on another bundle, afaik.
> 
> The documentation for NSAddImage suggests that they can:
> 
>> NSADDIMAGE_OPTION_WITH_SEARCHING
>> With this option, the image_name passed for the library and all its
>> dependents is affected by the various dyld environment variables as if this
>> library were linked into the program.
> 
> Note "and all its dependents".

The module can have shared libraries as dependencies.  IIRC it cannot
have other modules.

But perhaps as you wrote yesterday the SPICE core can be built as a
shared library (i.e. compiled with -shared and linked to a .dylib file).

> Failing that, it appears that explicit control over search paths can be had
> from Objective C, via the NSBundle class.

Everything you can do in ObjC you can also do in C with the
corresponding CoreFoundation library, in this case CFBundle, but it
looks like "that" bundle is a different thing than a Mach-O bundle.  It
is a directory that the UI shows as if it were a single file.  WTH...

> Something for someone who actually cares about macosx to work on.

Sure, at this point making Mac OS X not support modules actually makes
sense.  ELF and Windows seem to be okay.

Paolo

Patch

diff --git a/Makefile b/Makefile
index b59b49c..20167b8 100644
--- a/Makefile
+++ b/Makefile
@@ -196,6 +196,9 @@  Makefile: $(version-obj-y) $(version-lobj-y)
 libqemustub.a: $(stub-obj-y)
 libqemuutil.a: $(util-obj-y) qapi-types.o qapi-visit.o
 
+default-whitelist = $(foreach o,$(modules-m),"$(subst /,-,$o)",) NULL
+util/module.o-cflags = -D'CONFIG_MODULE_WHITELIST=$(default-whitelist)'
+
 ######################################################################
 
 qemu-img.o: qemu-img-cmds.h
diff --git a/block.c b/block.c
index a325efc..cd4f90e 100644
--- a/block.c
+++ b/block.c
@@ -3897,6 +3897,7 @@  BlockDriverAIOCB *bdrv_aio_discard(BlockDriverState *bs,
 
 void bdrv_init(void)
 {
+    module_load(MODULE_LOAD_BLOCK);
     module_call_init(MODULE_INIT_BLOCK);
 }
 
diff --git a/configure b/configure
index 55a75d8..9cb9c7b 100755
--- a/configure
+++ b/configure
@@ -199,6 +199,7 @@  datadir="\${prefix}/share"
 qemu_docdir="\${prefix}/share/doc/qemu"
 bindir="\${prefix}/bin"
 libdir="\${prefix}/lib"
+moddir="\${prefix}/lib/qemu"
 libexecdir="\${prefix}/libexec"
 includedir="\${prefix}/include"
 sysconfdir="\${prefix}/etc"
@@ -664,7 +665,9 @@  for opt do
   ;;
   --disable-debug-info)
   ;;
-  --enable-modules) modules="yes"
+  --enable-modules|--enable-modules=*)
+      modules="yes"
+      module_list=`echo "$optarg" | sed -e 's/,/ /g'`
   ;;
   --cpu=*)
   ;;
@@ -689,6 +692,8 @@  for opt do
   ;;
   --libdir=*) libdir="$optarg"
   ;;
+  --moddir=*) moddir="$optarg"
+  ;;
   --libexecdir=*) libexecdir="$optarg"
   ;;
   --includedir=*) includedir="$optarg"
@@ -1078,10 +1083,14 @@  echo "  --datadir=PATH           install firmware in PATH$confsuffix"
 echo "  --docdir=PATH            install documentation in PATH$confsuffix"
 echo "  --bindir=PATH            install binaries in PATH"
 echo "  --libdir=PATH            install libraries in PATH"
+echo "  --moddir=PATH            install modules in PATH"
 echo "  --sysconfdir=PATH        install config in PATH$confsuffix"
 echo "  --localstatedir=PATH     install local state in PATH (set at runtime on win32)"
 echo "  --with-confsuffix=SUFFIX suffix for QEMU data inside datadir and sysconfdir [$confsuffix]"
-echo "  --enable-modules         enable modules support"
+echo "  --enable-modules         enable modules support and whitelist all modules"
+echo "  --enable-modules=L       enable modules and provide a whitelist"
+echo "                           Available modules: block-curl block-iscsi block-gluster"
+echo "                                              block-ssh block-rbd"
 echo "  --enable-debug-tcg       enable TCG debugging"
 echo "  --disable-debug-tcg      disable TCG debugging (default)"
 echo "  --enable-debug-info       enable debugging information (default)"
@@ -2284,15 +2293,19 @@  if test "$mingw32" = yes; then
 else
     glib_req_ver=2.12
 fi
-if $pkg_config --atleast-version=$glib_req_ver gthread-2.0; then
-    glib_cflags=`$pkg_config --cflags gthread-2.0`
-    glib_libs=`$pkg_config --libs gthread-2.0`
-    CFLAGS="$glib_cflags $CFLAGS"
-    LIBS="$glib_libs $LIBS"
-    libs_qga="$glib_libs $libs_qga"
-else
-    error_exit "glib-$glib_req_ver required to compile QEMU"
-fi
+
+for i in gthread-2.0 gmodule-2.0; do
+    if $pkg_config --atleast-version=$glib_req_ver $i; then
+        glib_cflags=`$pkg_config --cflags $i`
+        glib_libs=`$pkg_config --libs $i`
+        CFLAGS="$glib_cflags $CFLAGS"
+        LIBS="$glib_libs $LIBS"
+        libs_qga="$glib_libs $libs_qga"
+    else
+        error_exit "glib-$glib_req_ver required to compile QEMU"
+    fi
+done
+
 
 ##########################################
 # pixman support probe
@@ -3643,6 +3656,7 @@  echo "Install prefix    $prefix"
 echo "BIOS directory    `eval echo $qemu_datadir`"
 echo "binary directory  `eval echo $bindir`"
 echo "library directory `eval echo $libdir`"
+echo "module directory  `eval echo $moddir`"
 echo "libexec directory `eval echo $libexecdir`"
 echo "include directory `eval echo $includedir`"
 echo "config directory  `eval echo $sysconfdir`"
@@ -3669,6 +3683,9 @@  if test "$slirp" = "yes" ; then
     echo "smbd              $smbd"
 fi
 echo "module support    $modules"
+if test -n "$module_list"; then
+    echo "module whitelist  $module_list"
+fi
 echo "host CPU          $cpu"
 echo "host big endian   $bigendian"
 echo "target list       $target_list"
@@ -3769,6 +3786,7 @@  echo all: >> $config_host_mak
 echo "prefix=$prefix" >> $config_host_mak
 echo "bindir=$bindir" >> $config_host_mak
 echo "libdir=$libdir" >> $config_host_mak
+echo "moddir=$moddir" >> $config_host_mak
 echo "libexecdir=$libexecdir" >> $config_host_mak
 echo "includedir=$includedir" >> $config_host_mak
 echo "mandir=$mandir" >> $config_host_mak
@@ -3787,8 +3805,12 @@  echo "libs_softmmu=$libs_softmmu" >> $config_host_mak
 
 echo "ARCH=$ARCH" >> $config_host_mak
 
+echo "CONFIG_FINGERPRINT=$(date +%s$$$RANDOM)" >> $config_host_mak
 if test "$modules" = "yes"; then
   echo "CONFIG_MODULES=y" >> $config_host_mak
+  if test -n "$module_list"; then
+      echo "CONFIG_MODULE_WHITELIST=$module_list" >> $config_host_mak
+  fi
 fi
 case "$cpu" in
   arm|i386|x86_64|x32|ppc|aarch64)
diff --git a/include/qemu/module.h b/include/qemu/module.h
index c4ccd57..6458d8f 100644
--- a/include/qemu/module.h
+++ b/include/qemu/module.h
@@ -14,11 +14,25 @@ 
 #ifndef QEMU_MODULE_H
 #define QEMU_MODULE_H
 
+#ifdef BUILD_DSO
+
+/* For error message, this function is an identification of qemu module */
+void qemu_module_do_init(void (*init)(void));
+
+/* To restrict loading of arbitrary DSO, The init function name is changed per
+ * "./configure" to refuse unknown DSO file */
+void DSO_INIT_FUN(void);
+
+#define module_init(function, type)                                         \
+void qemu_module_do_init(void (*init)(void)) { init(); }                    \
+void DSO_INIT_FUN(void) { qemu_module_do_init(function); }
+#else
 /* This should not be used directly.  Use block_init etc. instead.  */
 #define module_init(function, type)                                         \
 static void __attribute__((constructor)) do_qemu_init_ ## function(void) {  \
     register_module_init(function, type);                                   \
 }
+#endif
 
 typedef enum {
     MODULE_INIT_BLOCK,
@@ -37,4 +51,13 @@  void register_module_init(void (*fn)(void), module_init_type type);
 
 void module_call_init(module_init_type type);
 
+typedef enum {
+    MODULE_LOAD_BLOCK = 0,
+    MODULE_LOAD_UI,
+    MODULE_LOAD_NET,
+    MODULE_LOAD_MAX,
+} module_load_type;
+
+void module_load(module_load_type type);
+
 #endif
diff --git a/rules.mak b/rules.mak
index 2ff2f16..dc9757a 100644
--- a/rules.mak
+++ b/rules.mak
@@ -69,7 +69,7 @@  endif
 %.o: %.dtrace
 	$(call quiet-command,dtrace -o $@ -G -s $<, "  GEN   $(TARGET_DIR)$@")
 
-%$(DSOSUF): QEMU_CFLAGS += -fPIC
+%$(DSOSUF): QEMU_CFLAGS += -fPIC -DBUILD_DSO
 %$(DSOSUF): LDFLAGS += $(LDFLAGS_SHARED)
 %$(DSOSUF): %.mo libqemustub.a
 	$(call LINK,$^)
@@ -172,13 +172,18 @@  $(if $(nested-dirs),
   $(call unnest-vars-1))
 endef
 
+is-whitelisted = $(if $(CONFIG_MODULE_WHITELIST),$(strip \
+    $(filter $(CONFIG_MODULE_WHITELIST),$(subst /,-,$(basename $1)))),\
+	yes)
 define add-modules
 $(foreach o,$(filter %.o,$($1)),
 	$(eval $(patsubst %.o,%.mo,$o): $o) \
 	$(eval $(patsubst %.o,%.mo,$o)-objs := $o))
 $(foreach o,$(filter %.mo,$($1)),$(eval \
     $o: $($o-objs)))
-$(eval modules-m += $(patsubst %.o,%.mo,$($1)))
+$(eval t := $(patsubst %.o,%.mo,$($1)))
+$(foreach o,$t,$(if $(call is-whitelisted,$o),$(eval \
+	modules-m += $o)))
 endef
 
 define unnest-vars
diff --git a/scripts/create_config b/scripts/create_config
index b1adbf5..6d47df7 100755
--- a/scripts/create_config
+++ b/scripts/create_config
@@ -26,6 +26,25 @@  case $line in
     # save for the next definitions
     prefix=${line#*=}
     ;;
+ moddir=*)
+    eval "moddir=\"${line#*=}\""
+    echo "#define CONFIG_MODDIR \"$moddir\""
+    ;;
+ CONFIG_FINGERPRINT=*)
+    echo "#define DSO_INIT_FUN init_${line#*=}"
+    echo "#define DSO_INIT_FUN_STR \"init_${line#*=}\""
+    ;;
+ CONFIG_MODULES=*)
+    echo "#define CONFIG_MODULES \"${line#*=}\""
+    ;;
+ CONFIG_MODULE_WHITELIST=*)
+    echo "#undef CONFIG_MODULE_WHITELIST"
+    echo "#define CONFIG_MODULE_WHITELIST\\"
+    for mod in ${line#*=}; do
+      echo "    \"${mod}\",\\"
+    done
+    echo "    NULL"
+    ;;
  CONFIG_AUDIO_DRIVERS=*)
     drivers=${line#*=}
     echo "#define CONFIG_AUDIO_DRIVERS \\"
@@ -104,6 +123,9 @@  case $line in
     value=${line#*=}
     echo "#define $name $value"
     ;;
+ DSOSUF=*)
+    echo "#define HOST_DSOSUF \"${line#*=}\""
+    ;;
 esac
 
 done # read
diff --git a/util/module.c b/util/module.c
index 7acc33d..70da1ca 100644
--- a/util/module.c
+++ b/util/module.c
@@ -13,6 +13,7 @@ 
  * GNU GPL, version 2 or (at your option) any later version.
  */
 
+#include <gmodule.h>
 #include "qemu-common.h"
 #include "qemu/queue.h"
 #include "qemu/module.h"
@@ -79,3 +80,83 @@  void module_call_init(module_init_type type)
         e->init();
     }
 }
+
+#ifdef CONFIG_MODULES
+static void module_load_file(const char *fname)
+{
+    GModule *g_module;
+    void (*init_fun)(void);
+    const char *dsosuf = HOST_DSOSUF;
+    int len = strlen(fname);
+    int suf_len = strlen(dsosuf);
+
+    if (len <= suf_len || strcmp(&fname[len - suf_len], dsosuf)) {
+        /* wrong suffix */
+        return;
+    }
+    if (access(fname, F_OK)) {
+        return;
+    }
+
+    g_module = g_module_open(fname, G_MODULE_BIND_LAZY | G_MODULE_BIND_LOCAL);
+    if (!g_module) {
+        fprintf(stderr, "Failed to load module: %s\n",
+                g_module_error());
+        return;
+    }
+    if (!g_module_symbol(g_module, DSO_INIT_FUN_STR, (gpointer *)&init_fun)) {
+        fprintf(stderr, "Failed to initialize module: %s\n",
+                fname);
+        /* Print some info if this is a QEMU module (but from different build),
+         * this will make debugging user problems easier. */
+        if (g_module_symbol(g_module, "qemu_module_do_init",
+                            (gpointer *)&init_fun)) {
+            fprintf(stderr,
+                    "Note: only modules from the same build can be loaded.\n");
+        }
+        g_module_close(g_module);
+        return;
+    }
+    init_fun();
+}
+#endif
+
+void module_load(module_load_type type)
+{
+#ifdef CONFIG_MODULES
+    const char *prefix;
+    char *fname = NULL;
+    const char **mp;
+    static const char *module_whitelist[] = {
+        CONFIG_MODULE_WHITELIST
+    };
+
+    if (!g_module_supported()) {
+        return;
+    }
+
+    switch (type) {
+    case MODULE_LOAD_BLOCK:
+        prefix = "block-";
+        break;
+    case MODULE_LOAD_UI:
+        prefix = "ui-";
+        break;
+    case MODULE_LOAD_NET:
+        prefix = "ui-";
+        break;
+    default:
+        return;
+    }
+
+    for (mp = &module_whitelist[0]; *mp; mp++) {
+        if (strncmp(prefix, *mp, strlen(prefix))) {
+            continue;
+        }
+        fname = g_strdup_printf("%s/%s%s", CONFIG_MODDIR, *mp, HOST_DSOSUF);
+        module_load_file(fname);
+        g_free(fname);
+    }
+
+#endif
+}
diff --git a/vl.c b/vl.c
index 4e709d5..b20e7fb 100644
--- a/vl.c
+++ b/vl.c
@@ -2866,6 +2866,8 @@  int main(int argc, char **argv, char **envp)
 #endif
     }
 
+    module_load(MODULE_LOAD_UI);
+    module_load(MODULE_LOAD_NET);
     module_call_init(MODULE_INIT_QOM);
 
     qemu_add_opts(&qemu_drive_opts);