| Message ID | 1378877909-28518-5-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
Il 11/09/2013 07:38, Fam Zheng ha scritto: > Added three types of modules: > > typedef enum { > MODULE_LOAD_BLOCK = 0, > MODULE_LOAD_UI, > MODULE_LOAD_NET, > MODULE_LOAD_MAX, > } module_load_type; If you want to make spice into a module, you probably need also audio, char and hw modules. Paolo > and their loading function: > > void module_load(module_load_type). > > which loads all ".so" files in a subdir under "${PREFIX}/qemu/", e.g. > "/usr/lib/qemu/block". Modules of each type should be loaded before > respective subsystem initialization code. > > Requires gmodule-2.0 from glib. > > Signed-off-by: Fam Zheng <famz@redhat.com> > --- > block.c | 1 + > bsd-user/main.c | 3 +++ > configure | 22 ++++++++++++--------- > include/qemu/module.h | 9 +++++++++ > linux-user/main.c | 3 +++ > scripts/create_config | 4 ++++ > util/module.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++ > vl.c | 2 ++ > 8 files changed, 88 insertions(+), 9 deletions(-) > > diff --git a/block.c b/block.c > index 26639e8..16ceaaf 100644 > --- a/block.c > +++ b/block.c > @@ -4008,6 +4008,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/bsd-user/main.c b/bsd-user/main.c > index f9246aa..6cb9e35 100644 > --- a/bsd-user/main.c > +++ b/bsd-user/main.c > @@ -33,6 +33,7 @@ > #include "tcg.h" > #include "qemu/timer.h" > #include "qemu/envlist.h" > +#include "qemu/module.h" > > int singlestep; > #if defined(CONFIG_USE_GUEST_BASE) > @@ -749,6 +750,8 @@ int main(int argc, char **argv) > if (argc <= 1) > usage(); > > + module_load(MODULE_LOAD_UI); > + module_load(MODULE_LOAD_NET); > module_call_init(MODULE_INIT_QOM); > > if ((envlist = envlist_create()) == NULL) { > diff --git a/configure b/configure > index c6d4a62..a2858c2 100755 > --- a/configure > +++ b/configure > @@ -2252,15 +2252,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 > diff --git a/include/qemu/module.h b/include/qemu/module.h > index c4ccd57..f00bc25 100644 > --- a/include/qemu/module.h > +++ b/include/qemu/module.h > @@ -37,4 +37,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/linux-user/main.c b/linux-user/main.c > index 5c2f7b2..db08c23 100644 > --- a/linux-user/main.c > +++ b/linux-user/main.c > @@ -34,6 +34,7 @@ > #include "qemu/timer.h" > #include "qemu/envlist.h" > #include "elf.h" > +#include <qemu/module.h> > > char *exec_path; > > @@ -3551,6 +3552,8 @@ int main(int argc, char **argv, char **envp) > int i; > int ret; > > + module_load(MODULE_LOAD_UI); > + module_load(MODULE_LOAD_NET); > module_call_init(MODULE_INIT_QOM); > > qemu_cache_utils_init(envp); > diff --git a/scripts/create_config b/scripts/create_config > index b1adbf5..7a54f2d 100755 > --- a/scripts/create_config > +++ b/scripts/create_config > @@ -25,6 +25,7 @@ case $line in > prefix=*) > # save for the next definitions > prefix=${line#*=} > + echo "#define CONFIG_PREFIX \"$prefix\"" > ;; > CONFIG_AUDIO_DRIVERS=*) > drivers=${line#*=} > @@ -104,6 +105,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..ef75f8e 100644 > --- a/util/module.c > +++ b/util/module.c > @@ -13,6 +13,8 @@ > * GNU GPL, version 2 or (at your option) any later version. > */ > > +#include <gmodule.h> > +#include <dirent.h> > #include "qemu-common.h" > #include "qemu/queue.h" > #include "qemu/module.h" > @@ -79,3 +81,54 @@ void module_call_init(module_init_type type) > e->init(); > } > } > + > +void module_load(module_load_type type) > +{ > + const char *path; > + const char *dsosuf = HOST_DSOSUF; > + char *fname; > + int suf_len = strlen(dsosuf); > + DIR *dp; > + struct dirent *ep = NULL; > + GModule *g_module; > + > + if (!g_module_supported()) { > + return; > + } > + > + switch (type) { > + case MODULE_LOAD_BLOCK: > + path = CONFIG_PREFIX "/qemu/block/"; > + break; > + case MODULE_LOAD_UI: > + path = CONFIG_PREFIX "/qemu/ui/"; > + break; > + case MODULE_LOAD_NET: > + path = CONFIG_PREFIX "/qemu/net/"; > + break; > + default: > + return; > + } > + > + dp = opendir(path); > + if (!dp) { > + fprintf(stderr, "Failed to open dir %s\n", path); > + return; > + } > + for (ep = readdir(dp); ep; ep = readdir(dp)) { > + int len = strlen(ep->d_name); > + if (len > suf_len && > + !strcmp(&ep->d_name[len - suf_len], dsosuf)) { > + fname = g_strdup_printf("%s%s", path, ep->d_name); > + g_module = g_module_open(fname, > + G_MODULE_BIND_LAZY | G_MODULE_BIND_LOCAL); > + if (!g_module) { > + fprintf(stderr, "Failed to open module file %s\n", > + g_module_error()); > + g_free(fname); > + continue; > + } > + g_free(fname); > + } > + } > +} > diff --git a/vl.c b/vl.c > index b4b119a..659e53a 100644 > --- a/vl.c > +++ b/vl.c > @@ -2940,6 +2940,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); >
--On 11 September 2013 13:38:27 +0800 Fam Zheng <famz@redhat.com> wrote: > + switch (type) { > + case MODULE_LOAD_BLOCK: > + path = CONFIG_PREFIX "/qemu/block/"; > + break; > + case MODULE_LOAD_UI: > + path = CONFIG_PREFIX "/qemu/ui/"; > + break; > + case MODULE_LOAD_NET: > + path = CONFIG_PREFIX "/qemu/net/"; > + break; > + default: > + return; > + } > + I appreciate I am coming in late into this discussion, and am only scanning the code quickly, so apologies if I have the wrong end of the stick. This APPEARS to load modules from a) a fixed path determined at compile time b) a path which is not dependent on qemu version This would make it hard to have 2 versions of qemu installed on a system at once, or even develop one version of qemu with another version installed. I suspect this will be hard not only for developers, but also for distributions, particularly if the idea is to keep vms running during upgrades. Consider the case where packages A and B both depend on qemu module package C, then you wish to upgrade to A', B' and C'. At some point you are likely to want both C and C' installed. Is the idea here that QEMU is always built with CONFIG_PREFIX having versioning inside it (in a distro environment)? Can I suggest that at the very least, it should be possible to specify an alternate path to the module directory via the CLI?
On 11 September 2013 06:38, Fam Zheng <famz@redhat.com> wrote: > --- a/linux-user/main.c > +++ b/linux-user/main.c > @@ -34,6 +34,7 @@ > #include "qemu/timer.h" > #include "qemu/envlist.h" > #include "elf.h" > +#include <qemu/module.h> > > char *exec_path; > > @@ -3551,6 +3552,8 @@ int main(int argc, char **argv, char **envp) > int i; > int ret; > > + module_load(MODULE_LOAD_UI); > + module_load(MODULE_LOAD_NET); > module_call_init(MODULE_INIT_QOM); This looks kind of fishy. The *-user binaries don't even have any UI, and they shouldn't be using the networking either. For that matter it's really unclear to me that they should have any kind of loadable modules at all. -- PMM
Il 11/09/2013 16:10, Alex Bligh ha scritto: > > > --On 11 September 2013 13:38:27 +0800 Fam Zheng <famz@redhat.com> wrote: > >> + switch (type) { >> + case MODULE_LOAD_BLOCK: >> + path = CONFIG_PREFIX "/qemu/block/"; >> + break; >> + case MODULE_LOAD_UI: >> + path = CONFIG_PREFIX "/qemu/ui/"; >> + break; >> + case MODULE_LOAD_NET: >> + path = CONFIG_PREFIX "/qemu/net/"; >> + break; >> + default: >> + return; >> + } >> + > > I appreciate I am coming in late into this discussion, and am only scanning > the code quickly, so apologies if I have the wrong end of the stick. You're absolutely not coming in late! So far we really just discussed the build side of the implementation, and I didn't review this patch at all. > This APPEARS to load modules from > a) a fixed path determined at compile time > b) a path which is not dependent on qemu version c) a path that is not under the normal /usr/lib or similar path. > This would make it hard to have 2 versions of qemu installed on a > system at once, or even develop one version of qemu with another > version installed. This is hard anyway because the firmware files are not necessarily compatible with different QEMU versions. With 2 versions of QEMU installed on a system, I would suggest putting both of them in different subdirectories under /opt. However, (c) is a problem and... > I suspect this will be hard not only for developers, > but also for distributions, particularly if the idea is to keep vms > running during upgrades. Consider the case where packages A and B > both depend on qemu module package C, then you wish to upgrade to > A', B' and C'. At some point you are likely to want both C and C' > installed. Is the idea here that QEMU is always built with CONFIG_PREFIX > having versioning inside it (in a distro environment)? > > Can I suggest that at the very least, it should be possible to specify > an alternate path to the module directory via the CLI? ... this is also a good idea. Probably it should use an algorithm similar to that used for data_dir. Paolo
On 09/11/2013 07:10 AM, Alex Bligh wrote: > > > --On 11 September 2013 13:38:27 +0800 Fam Zheng <famz@redhat.com> wrote: > >> + switch (type) { >> + case MODULE_LOAD_BLOCK: >> + path = CONFIG_PREFIX "/qemu/block/"; >> + break; >> + case MODULE_LOAD_UI: >> + path = CONFIG_PREFIX "/qemu/ui/"; >> + break; >> + case MODULE_LOAD_NET: >> + path = CONFIG_PREFIX "/qemu/net/"; >> + break; >> + default: >> + return; >> + } >> + > > I appreciate I am coming in late into this discussion, and am only scanning > the code quickly, so apologies if I have the wrong end of the stick. > > This APPEARS to load modules from > a) a fixed path determined at compile time > b) a path which is not dependent on qemu version > > This would make it hard to have 2 versions of qemu installed on a > system at once, or even develop one version of qemu with another > version installed. I suspect this will be hard not only for developers, > but also for distributions, particularly if the idea is to keep vms > running during upgrades. Consider the case where packages A and B > both depend on qemu module package C, then you wish to upgrade to > A', B' and C'. At some point you are likely to want both C and C' > installed. Is the idea here that QEMU is always built with CONFIG_PREFIX > having versioning inside it (in a distro environment)? > > Can I suggest that at the very least, it should be possible to specify > an alternate path to the module directory via the CLI? > If we want dependencies between modules, we may well need to get the dynamic linker involved with the search directory. This would rule out any command-line, or monitor-line altering of the path. But it does suggest the default being DT_RUNPATH, overridable "near" the command-line via LD_RUN_PATH. I also wonder about the utility of the subdirectories above, as opposed to filename prefixes. r~
--On 11 September 2013 08:06:20 -0700 Richard Henderson <rth@twiddle.net> wrote: > If we want dependencies between modules, we may well need to get the > dynamic linker involved with the search directory. This would rule > out any command-line, or monitor-line altering of the path. > > But it does suggest the default being DT_RUNPATH, overridable "near" > the command-line via LD_RUN_PATH. Not quite sure what you are suggesting here. DT_RUNPATH is an elf tag IIRC. Not sure what LD_RUN_PATH does. LD_LIBRARY_PATH affects more than just this. If we are using the dynamic linker and DT_RUNPATH (which I believe is in the executable) or the normal paths we WILL NEED proper API versioning etc.; in this case (in the distro example I pulled out), an upgrade to qemu making an API change will have different API versioned libs and these can happily coexist, so no need for directories. But are we really sufficiently stable for that? Any internal change in API (as modules communicate using an internal API, not something wrapped with opaque struct *) is going to result in a library major version number change as far as I can see. That's going to play merry hell with distributions. Assuming we restrict this to load dependencies (as opposed to version dependencies), I would have thought the way to go is a) ensure that each build of qemu has ALL the modules available. b) qemu and all its modules always look in the same place c) that place can be changed at load time (as well as at compile time) to permit different versions of qemu to be around. I think this can be done with dlopen() as follows: If filename contains a slash ("/"), then it is interpreted as a (relative or absolute) pathname. This means no searching is done with DT_RPATH, DT_RUNPATH etc. It later says: If the library has dependencies on other shared libraries, then these are also automatically loaded by the dynamic linker using the same rules. (This process may occur recursively, if those libraries in turn have dependencies, and so on.) My dlopen() foo is insufficiently strong to know what happens if you have dependencies AND specify an absolute or relative path (as opposed to just a module name). In an ideal world it would just look in the same directory and not elsewhere, in which case we can use the scheme I suggested AND use dlopen() AND not worry about API versioning (yet). > I also wonder about the utility of the subdirectories above, as > opposed to filename prefixes. +1
On 09/11/2013 08:23 AM, Alex Bligh wrote: >> But it does suggest the default being DT_RUNPATH, overridable "near" >> the command-line via LD_RUN_PATH. > > Not quite sure what you are suggesting here. DT_RUNPATH is an elf > tag IIRC. Not sure what LD_RUN_PATH does. LD_LIBRARY_PATH affects > more than just this. LD_RUN_PATH was my thinko -- LD_LIBRARY_PATH was what I was thinking. And while it does affect more than this, if set specifically for the qemu executable e.g. by a script then it need not really affect much. > If we are using the dynamic linker and DT_RUNPATH (which I believe > is in the executable) or the normal paths we WILL NEED proper API > versioning etc.; in this case (in the distro example I pulled out), > an upgrade to qemu making an API change will have different API > versioned libs and these can happily coexist, so no need for directories. I don't believe I ever suggested we're stable, or should even pretend to be so at this point. Install multiple qemu to different trees, and use different paths. Or even use the $ORIGIN prefix, which allows paths relative to the main executable, allowing the installed qemu tree to be moved at its root. r~
On Wed, 09/11 15:24, Peter Maydell wrote: > On 11 September 2013 06:38, Fam Zheng <famz@redhat.com> wrote: > > --- a/linux-user/main.c > > +++ b/linux-user/main.c > > @@ -34,6 +34,7 @@ > > #include "qemu/timer.h" > > #include "qemu/envlist.h" > > #include "elf.h" > > +#include <qemu/module.h> > > > > char *exec_path; > > > > @@ -3551,6 +3552,8 @@ int main(int argc, char **argv, char **envp) > > int i; > > int ret; > > > > + module_load(MODULE_LOAD_UI); > > + module_load(MODULE_LOAD_NET); > > module_call_init(MODULE_INIT_QOM); > > This looks kind of fishy. The *-user binaries don't even > have any UI, and they shouldn't be using the networking > either. For that matter it's really unclear to me that they > should have any kind of loadable modules at all. > I see, dropping this change.
diff --git a/block.c b/block.c index 26639e8..16ceaaf 100644 --- a/block.c +++ b/block.c @@ -4008,6 +4008,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/bsd-user/main.c b/bsd-user/main.c index f9246aa..6cb9e35 100644 --- a/bsd-user/main.c +++ b/bsd-user/main.c @@ -33,6 +33,7 @@ #include "tcg.h" #include "qemu/timer.h" #include "qemu/envlist.h" +#include "qemu/module.h" int singlestep; #if defined(CONFIG_USE_GUEST_BASE) @@ -749,6 +750,8 @@ int main(int argc, char **argv) if (argc <= 1) usage(); + module_load(MODULE_LOAD_UI); + module_load(MODULE_LOAD_NET); module_call_init(MODULE_INIT_QOM); if ((envlist = envlist_create()) == NULL) { diff --git a/configure b/configure index c6d4a62..a2858c2 100755 --- a/configure +++ b/configure @@ -2252,15 +2252,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 diff --git a/include/qemu/module.h b/include/qemu/module.h index c4ccd57..f00bc25 100644 --- a/include/qemu/module.h +++ b/include/qemu/module.h @@ -37,4 +37,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/linux-user/main.c b/linux-user/main.c index 5c2f7b2..db08c23 100644 --- a/linux-user/main.c +++ b/linux-user/main.c @@ -34,6 +34,7 @@ #include "qemu/timer.h" #include "qemu/envlist.h" #include "elf.h" +#include <qemu/module.h> char *exec_path; @@ -3551,6 +3552,8 @@ int main(int argc, char **argv, char **envp) int i; int ret; + module_load(MODULE_LOAD_UI); + module_load(MODULE_LOAD_NET); module_call_init(MODULE_INIT_QOM); qemu_cache_utils_init(envp); diff --git a/scripts/create_config b/scripts/create_config index b1adbf5..7a54f2d 100755 --- a/scripts/create_config +++ b/scripts/create_config @@ -25,6 +25,7 @@ case $line in prefix=*) # save for the next definitions prefix=${line#*=} + echo "#define CONFIG_PREFIX \"$prefix\"" ;; CONFIG_AUDIO_DRIVERS=*) drivers=${line#*=} @@ -104,6 +105,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..ef75f8e 100644 --- a/util/module.c +++ b/util/module.c @@ -13,6 +13,8 @@ * GNU GPL, version 2 or (at your option) any later version. */ +#include <gmodule.h> +#include <dirent.h> #include "qemu-common.h" #include "qemu/queue.h" #include "qemu/module.h" @@ -79,3 +81,54 @@ void module_call_init(module_init_type type) e->init(); } } + +void module_load(module_load_type type) +{ + const char *path; + const char *dsosuf = HOST_DSOSUF; + char *fname; + int suf_len = strlen(dsosuf); + DIR *dp; + struct dirent *ep = NULL; + GModule *g_module; + + if (!g_module_supported()) { + return; + } + + switch (type) { + case MODULE_LOAD_BLOCK: + path = CONFIG_PREFIX "/qemu/block/"; + break; + case MODULE_LOAD_UI: + path = CONFIG_PREFIX "/qemu/ui/"; + break; + case MODULE_LOAD_NET: + path = CONFIG_PREFIX "/qemu/net/"; + break; + default: + return; + } + + dp = opendir(path); + if (!dp) { + fprintf(stderr, "Failed to open dir %s\n", path); + return; + } + for (ep = readdir(dp); ep; ep = readdir(dp)) { + int len = strlen(ep->d_name); + if (len > suf_len && + !strcmp(&ep->d_name[len - suf_len], dsosuf)) { + fname = g_strdup_printf("%s%s", path, ep->d_name); + g_module = g_module_open(fname, + G_MODULE_BIND_LAZY | G_MODULE_BIND_LOCAL); + if (!g_module) { + fprintf(stderr, "Failed to open module file %s\n", + g_module_error()); + g_free(fname); + continue; + } + g_free(fname); + } + } +} diff --git a/vl.c b/vl.c index b4b119a..659e53a 100644 --- a/vl.c +++ b/vl.c @@ -2940,6 +2940,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 all ".so" files in a subdir under "${PREFIX}/qemu/", e.g. "/usr/lib/qemu/block". Modules of each type should be loaded before respective subsystem initialization code. Requires gmodule-2.0 from glib. Signed-off-by: Fam Zheng <famz@redhat.com> --- block.c | 1 + bsd-user/main.c | 3 +++ configure | 22 ++++++++++++--------- include/qemu/module.h | 9 +++++++++ linux-user/main.c | 3 +++ scripts/create_config | 4 ++++ util/module.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++ vl.c | 2 ++ 8 files changed, 88 insertions(+), 9 deletions(-)