| Message ID | 1379314227-8855-6-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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 >
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
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.
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
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.
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
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.
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
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
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 > > >
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).
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
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
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
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.
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
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.
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
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
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
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~
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~
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~
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
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
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~
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
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.
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
于 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. > >
于 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 >
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
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~
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
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);
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(-)