| Message ID | 1466631354-17309-4-git-send-email-clord@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 06/22 17:35, Colin Lord wrote: > From: Marc Mari <markmb@redhat.com> > > Extend the current module interface to allow for block drivers to be loaded > dynamically on request. > > The only block drivers that can be converted into modules are the drivers > that don't perform any init operation except for registering themselves. > > All the necessary module information is located in a new structure found in > include/qemu/module_block.h > > Signed-off-by: Marc Mari <markmb@redhat.com> > Signed-off-by: Colin Lord <clord@redhat.com> > --- > Makefile | 3 -- > block.c | 86 +++++++++++++++++++++++++++++++++++++++++++++------ > include/qemu/module.h | 3 ++ > util/module.c | 37 ++++++---------------- > 4 files changed, 90 insertions(+), 39 deletions(-) > > diff --git a/Makefile b/Makefile > index 8f8b6a2..461187c 100644 > --- a/Makefile > +++ b/Makefile > @@ -247,9 +247,6 @@ Makefile: $(version-obj-y) $(version-lobj-y) > libqemustub.a: $(stub-obj-y) > libqemuutil.a: $(util-obj-y) > > -block-modules = $(foreach o,$(block-obj-m),"$(basename $(subst /,-,$o))",) NULL > -util/module.o-cflags = -D'CONFIG_BLOCK_MODULES=$(block-modules)' > - > ###################################################################### > > qemu-img.o: qemu-img-cmds.h > diff --git a/block.c b/block.c > index f54bc25..7a91434 100644 > --- a/block.c > +++ b/block.c > @@ -26,6 +26,7 @@ > #include "block/block_int.h" > #include "block/blockjob.h" > #include "qemu/error-report.h" > +#include "qemu/module_block.h" > #include "qemu/module.h" > #include "qapi/qmp/qerror.h" > #include "qapi/qmp/qbool.h" > @@ -242,11 +243,29 @@ BlockDriverState *bdrv_new(void) > BlockDriver *bdrv_find_format(const char *format_name) > { > BlockDriver *drv1; > + size_t i; > + > QLIST_FOREACH(drv1, &bdrv_drivers, list) { > if (!strcmp(drv1->format_name, format_name)) { > return drv1; > } > } > + > + for (i = 0; i < ARRAY_SIZE(block_driver_modules); ++i) { > + if (!strcmp(block_driver_modules[i].format_name, format_name)) { > + block_module_load_one(block_driver_modules[i].library_name); > + /* Copying code is not nice, but this way the current discovery is > + * not modified. Calling recursively could fail if the library > + * has been deleted. > + */ > + QLIST_FOREACH(drv1, &bdrv_drivers, list) { > + if (!strcmp(drv1->format_name, format_name)) { > + return drv1; > + } > + } > + } > + } > + > return NULL; > } > > @@ -447,8 +466,15 @@ int get_tmp_filename(char *filename, int size) > static BlockDriver *find_hdev_driver(const char *filename) > { > int score_max = 0, score; > + size_t i; > BlockDriver *drv = NULL, *d; > > + for (i = 0; i < ARRAY_SIZE(block_driver_modules); ++i) { > + if (block_driver_modules[i].has_probe_device) { > + block_module_load_one(block_driver_modules[i].library_name); > + } > + } > + > QLIST_FOREACH(d, &bdrv_drivers, list) { > if (d->bdrv_probe_device) { > score = d->bdrv_probe_device(filename); > @@ -470,6 +496,7 @@ BlockDriver *bdrv_find_protocol(const char *filename, > char protocol[128]; > int len; > const char *p; > + size_t i; > > /* TODO Drivers without bdrv_file_open must be specified explicitly */ > > @@ -496,6 +523,7 @@ BlockDriver *bdrv_find_protocol(const char *filename, > len = sizeof(protocol) - 1; > memcpy(protocol, filename, len); > protocol[len] = '\0'; > + > QLIST_FOREACH(drv1, &bdrv_drivers, list) { > if (drv1->protocol_name && > !strcmp(drv1->protocol_name, protocol)) { > @@ -503,6 +531,23 @@ BlockDriver *bdrv_find_protocol(const char *filename, > } > } > > + for (i = 0; i < ARRAY_SIZE(block_driver_modules); ++i) { > + if (block_driver_modules[i].protocol_name && > + !strcmp(block_driver_modules[i].protocol_name, protocol)) { > + block_module_load_one(block_driver_modules[i].library_name); > + /* Copying code is not nice, but this way the current discovery is > + * not modified. Calling recursively could fail if the library > + * has been deleted. > + */ > + QLIST_FOREACH(drv1, &bdrv_drivers, list) { > + if (drv1->protocol_name && > + !strcmp(drv1->protocol_name, protocol)) { > + return drv1; > + } > + } > + } > + } > + > error_setg(errp, "Unknown protocol '%s'", protocol); > return NULL; > } > @@ -525,8 +570,15 @@ BlockDriver *bdrv_probe_all(const uint8_t *buf, int buf_size, > const char *filename) > { > int score_max = 0, score; > + size_t i; > BlockDriver *drv = NULL, *d; > > + for (i = 0; i < ARRAY_SIZE(block_driver_modules); ++i) { > + if (block_driver_modules[i].has_probe) { > + block_module_load_one(block_driver_modules[i].library_name); > + } > + } > + > QLIST_FOREACH(d, &bdrv_drivers, list) { > if (d->bdrv_probe) { > score = d->bdrv_probe(buf, buf_size, filename); > @@ -2738,26 +2790,42 @@ static int qsort_strcmp(const void *a, const void *b) > return strcmp(a, b); > } > > +static const char **add_format(const char **formats, int *count, > + const char *format_name) > +{ > + int i; > + > + for (i = 0; i < *count; i++) { > + if (!strcmp(formats[i], format_name)) { > + return formats; > + } > + } > + > + *count += 1; > + formats = g_renew(const char *, formats, *count); > + formats[*count] = format_name; > + return formats; > +} > + > void bdrv_iterate_format(void (*it)(void *opaque, const char *name), > void *opaque) > { > BlockDriver *drv; > int count = 0; > int i; > + size_t n; > const char **formats = NULL; > > QLIST_FOREACH(drv, &bdrv_drivers, list) { > if (drv->format_name) { > - bool found = false; > - int i = count; > - while (formats && i && !found) { > - found = !strcmp(formats[--i], drv->format_name); > - } > + formats = add_format(formats, &count, drv->format_name); > + } > + } > > - if (!found) { > - formats = g_renew(const char *, formats, count + 1); > - formats[count++] = drv->format_name; > - } > + for (n = 0; n < ARRAY_SIZE(block_driver_modules); ++n) { > + if (block_driver_modules[n].format_name) { > + formats = add_format(formats, &count, > + block_driver_modules[n].format_name); > } > } > > diff --git a/include/qemu/module.h b/include/qemu/module.h > index 2370708..4729858 100644 > --- a/include/qemu/module.h > +++ b/include/qemu/module.h > @@ -52,9 +52,12 @@ typedef enum { > #define qapi_init(function) module_init(function, MODULE_INIT_QAPI) > #define type_init(function) module_init(function, MODULE_INIT_QOM) > > +#define block_module_load_one(lib) module_load_one("block-", lib); Superfluous ending semi-colon? > + > void register_module_init(void (*fn)(void), module_init_type type); > void register_dso_module_init(void (*fn)(void), module_init_type type); > > void module_call_init(module_init_type type); > +void module_load_one(const char *prefix, const char *lib_name); > > #endif > diff --git a/util/module.c b/util/module.c > index ce058ae..dbc6e52 100644 > --- a/util/module.c > +++ b/util/module.c > @@ -91,14 +91,11 @@ void register_dso_module_init(void (*fn)(void), module_init_type type) > QTAILQ_INSERT_TAIL(&dso_init_list, e, node); > } > > -static void module_load(module_init_type type); > - > void module_call_init(module_init_type type) > { > ModuleTypeList *l; > ModuleEntry *e; > > - module_load(type); > l = find_type(type); > > QTAILQ_FOREACH(e, l, node) { > @@ -163,14 +160,10 @@ out: > } > #endif > > -static void module_load(module_init_type type) > +void module_load_one(const char *prefix, const char *lib_name) > { > #ifdef CONFIG_MODULES > char *fname = NULL; > - const char **mp; > - static const char *block_modules[] = { > - CONFIG_BLOCK_MODULES > - }; > char *exec_dir; > char *dirs[3]; > int i = 0; > @@ -181,15 +174,6 @@ static void module_load(module_init_type type) > return; > } > > - switch (type) { > - case MODULE_INIT_BLOCK: > - mp = block_modules; > - break; > - default: > - /* no other types have dynamic modules for now*/ > - return; > - } > - > exec_dir = qemu_get_exec_dir(); > dirs[i++] = g_strdup_printf("%s", CONFIG_QEMU_MODDIR); > dirs[i++] = g_strdup_printf("%s/..", exec_dir ? : ""); > @@ -198,16 +182,15 @@ static void module_load(module_init_type type) > g_free(exec_dir); > exec_dir = NULL; > > - for ( ; *mp; mp++) { > - for (i = 0; i < ARRAY_SIZE(dirs); i++) { > - fname = g_strdup_printf("%s/%s%s", dirs[i], *mp, HOST_DSOSUF); > - ret = module_load_file(fname); > - g_free(fname); > - fname = NULL; > - /* Try loading until loaded a module file */ > - if (!ret) { > - break; > - } > + for (i = 0; i < ARRAY_SIZE(dirs); i++) { > + fname = g_strdup_printf("%s/%s%s%s", > + dirs[i], prefix, lib_name, HOST_DSOSUF); > + ret = module_load_file(fname); > + g_free(fname); > + fname = NULL; > + /* Try loading until loaded a module file */ > + if (!ret) { > + break; > } > } > > -- > 2.5.5 > >
On Wed, 06/22 17:35, Colin Lord wrote: > From: Marc Mari <markmb@redhat.com> > > Extend the current module interface to allow for block drivers to be loaded > dynamically on request. > > The only block drivers that can be converted into modules are the drivers > that don't perform any init operation except for registering themselves. > > All the necessary module information is located in a new structure found in > include/qemu/module_block.h > > Signed-off-by: Marc Mari <markmb@redhat.com> > Signed-off-by: Colin Lord <clord@redhat.com> > --- > Makefile | 3 -- > block.c | 86 +++++++++++++++++++++++++++++++++++++++++++++------ > include/qemu/module.h | 3 ++ > util/module.c | 37 ++++++---------------- > 4 files changed, 90 insertions(+), 39 deletions(-) > > diff --git a/Makefile b/Makefile > index 8f8b6a2..461187c 100644 > --- a/Makefile > +++ b/Makefile > @@ -247,9 +247,6 @@ Makefile: $(version-obj-y) $(version-lobj-y) > libqemustub.a: $(stub-obj-y) > libqemuutil.a: $(util-obj-y) > > -block-modules = $(foreach o,$(block-obj-m),"$(basename $(subst /,-,$o))",) NULL > -util/module.o-cflags = -D'CONFIG_BLOCK_MODULES=$(block-modules)' > - > ###################################################################### > > qemu-img.o: qemu-img-cmds.h > diff --git a/block.c b/block.c > index f54bc25..7a91434 100644 > --- a/block.c > +++ b/block.c > @@ -26,6 +26,7 @@ > #include "block/block_int.h" > #include "block/blockjob.h" > #include "qemu/error-report.h" > +#include "qemu/module_block.h" > #include "qemu/module.h" > #include "qapi/qmp/qerror.h" > #include "qapi/qmp/qbool.h" > @@ -242,11 +243,29 @@ BlockDriverState *bdrv_new(void) > BlockDriver *bdrv_find_format(const char *format_name) > { > BlockDriver *drv1; > + size_t i; > + > QLIST_FOREACH(drv1, &bdrv_drivers, list) { > if (!strcmp(drv1->format_name, format_name)) { > return drv1; > } > } > + > + for (i = 0; i < ARRAY_SIZE(block_driver_modules); ++i) { > + if (!strcmp(block_driver_modules[i].format_name, format_name)) { > + block_module_load_one(block_driver_modules[i].library_name); > + /* Copying code is not nice, but this way the current discovery is > + * not modified. Calling recursively could fail if the library > + * has been deleted. > + */ > + QLIST_FOREACH(drv1, &bdrv_drivers, list) { > + if (!strcmp(drv1->format_name, format_name)) { > + return drv1; > + } > + } > + } > + } > + > return NULL; > } > > @@ -447,8 +466,15 @@ int get_tmp_filename(char *filename, int size) > static BlockDriver *find_hdev_driver(const char *filename) > { > int score_max = 0, score; > + size_t i; > BlockDriver *drv = NULL, *d; > > + for (i = 0; i < ARRAY_SIZE(block_driver_modules); ++i) { > + if (block_driver_modules[i].has_probe_device) { > + block_module_load_one(block_driver_modules[i].library_name); > + } > + } > + > QLIST_FOREACH(d, &bdrv_drivers, list) { > if (d->bdrv_probe_device) { > score = d->bdrv_probe_device(filename); > @@ -470,6 +496,7 @@ BlockDriver *bdrv_find_protocol(const char *filename, > char protocol[128]; > int len; > const char *p; > + size_t i; > > /* TODO Drivers without bdrv_file_open must be specified explicitly */ > > @@ -496,6 +523,7 @@ BlockDriver *bdrv_find_protocol(const char *filename, > len = sizeof(protocol) - 1; > memcpy(protocol, filename, len); > protocol[len] = '\0'; > + > QLIST_FOREACH(drv1, &bdrv_drivers, list) { > if (drv1->protocol_name && > !strcmp(drv1->protocol_name, protocol)) { > @@ -503,6 +531,23 @@ BlockDriver *bdrv_find_protocol(const char *filename, > } > } > > + for (i = 0; i < ARRAY_SIZE(block_driver_modules); ++i) { > + if (block_driver_modules[i].protocol_name && > + !strcmp(block_driver_modules[i].protocol_name, protocol)) { > + block_module_load_one(block_driver_modules[i].library_name); > + /* Copying code is not nice, but this way the current discovery is > + * not modified. Calling recursively could fail if the library > + * has been deleted. > + */ > + QLIST_FOREACH(drv1, &bdrv_drivers, list) { > + if (drv1->protocol_name && > + !strcmp(drv1->protocol_name, protocol)) { > + return drv1; > + } > + } > + } > + } > + > error_setg(errp, "Unknown protocol '%s'", protocol); > return NULL; > } > @@ -525,8 +570,15 @@ BlockDriver *bdrv_probe_all(const uint8_t *buf, int buf_size, > const char *filename) > { > int score_max = 0, score; > + size_t i; > BlockDriver *drv = NULL, *d; > > + for (i = 0; i < ARRAY_SIZE(block_driver_modules); ++i) { > + if (block_driver_modules[i].has_probe) { > + block_module_load_one(block_driver_modules[i].library_name); > + } > + } > + One idea about probing: we can avoid loading unnecessary modules if we move all .bdrv_probe functions from BlockDriver structs into a new block/probe.c, and let them return a driver name - all probing are just testing first bytes in the header, therefore quite independent from the rest of the drivers. Then we maintain a list of probing functions there. in block/probe.c: typedef const char *BdrvProbeFunc(const uint8_t *buf, int buf_size, const char *filename, int *score); const char *qcow2_probe(const uint8_t *buf, int buf_size, const char *filename, int *score) { ... } static BdrvProbeFunc format_probes[] = { qcow2_probe, dmg_probe, vmdk_probe, ... }; static BdrvProbeFunc device_probes[] = { hdev_probe_device, }; Then bdrv_probe_all can call each probe function in the list and find the highest score, and load the corresponding block module. The downside is each driver code is now split into two, but on the other hand it is more compact than, for example, separate qcow2-probe.c, dmg-probe.c, vmdk-probe.c that are linked to the main executable. Fam > QLIST_FOREACH(d, &bdrv_drivers, list) { > if (d->bdrv_probe) { > score = d->bdrv_probe(buf, buf_size, filename); > @@ -2738,26 +2790,42 @@ static int qsort_strcmp(const void *a, const void *b) > return strcmp(a, b); > } > > +static const char **add_format(const char **formats, int *count, > + const char *format_name) > +{ > + int i; > + > + for (i = 0; i < *count; i++) { > + if (!strcmp(formats[i], format_name)) { > + return formats; > + } > + } > + > + *count += 1; > + formats = g_renew(const char *, formats, *count); > + formats[*count] = format_name; > + return formats; > +} > + > void bdrv_iterate_format(void (*it)(void *opaque, const char *name), > void *opaque) > { > BlockDriver *drv; > int count = 0; > int i; > + size_t n; > const char **formats = NULL; > > QLIST_FOREACH(drv, &bdrv_drivers, list) { > if (drv->format_name) { > - bool found = false; > - int i = count; > - while (formats && i && !found) { > - found = !strcmp(formats[--i], drv->format_name); > - } > + formats = add_format(formats, &count, drv->format_name); > + } > + } > > - if (!found) { > - formats = g_renew(const char *, formats, count + 1); > - formats[count++] = drv->format_name; > - } > + for (n = 0; n < ARRAY_SIZE(block_driver_modules); ++n) { > + if (block_driver_modules[n].format_name) { > + formats = add_format(formats, &count, > + block_driver_modules[n].format_name); > } > } > > diff --git a/include/qemu/module.h b/include/qemu/module.h > index 2370708..4729858 100644 > --- a/include/qemu/module.h > +++ b/include/qemu/module.h > @@ -52,9 +52,12 @@ typedef enum { > #define qapi_init(function) module_init(function, MODULE_INIT_QAPI) > #define type_init(function) module_init(function, MODULE_INIT_QOM) > > +#define block_module_load_one(lib) module_load_one("block-", lib); > + > void register_module_init(void (*fn)(void), module_init_type type); > void register_dso_module_init(void (*fn)(void), module_init_type type); > > void module_call_init(module_init_type type); > +void module_load_one(const char *prefix, const char *lib_name); > > #endif > diff --git a/util/module.c b/util/module.c > index ce058ae..dbc6e52 100644 > --- a/util/module.c > +++ b/util/module.c > @@ -91,14 +91,11 @@ void register_dso_module_init(void (*fn)(void), module_init_type type) > QTAILQ_INSERT_TAIL(&dso_init_list, e, node); > } > > -static void module_load(module_init_type type); > - > void module_call_init(module_init_type type) > { > ModuleTypeList *l; > ModuleEntry *e; > > - module_load(type); > l = find_type(type); > > QTAILQ_FOREACH(e, l, node) { > @@ -163,14 +160,10 @@ out: > } > #endif > > -static void module_load(module_init_type type) > +void module_load_one(const char *prefix, const char *lib_name) > { > #ifdef CONFIG_MODULES > char *fname = NULL; > - const char **mp; > - static const char *block_modules[] = { > - CONFIG_BLOCK_MODULES > - }; > char *exec_dir; > char *dirs[3]; > int i = 0; > @@ -181,15 +174,6 @@ static void module_load(module_init_type type) > return; > } > > - switch (type) { > - case MODULE_INIT_BLOCK: > - mp = block_modules; > - break; > - default: > - /* no other types have dynamic modules for now*/ > - return; > - } > - > exec_dir = qemu_get_exec_dir(); > dirs[i++] = g_strdup_printf("%s", CONFIG_QEMU_MODDIR); > dirs[i++] = g_strdup_printf("%s/..", exec_dir ? : ""); > @@ -198,16 +182,15 @@ static void module_load(module_init_type type) > g_free(exec_dir); > exec_dir = NULL; > > - for ( ; *mp; mp++) { > - for (i = 0; i < ARRAY_SIZE(dirs); i++) { > - fname = g_strdup_printf("%s/%s%s", dirs[i], *mp, HOST_DSOSUF); > - ret = module_load_file(fname); > - g_free(fname); > - fname = NULL; > - /* Try loading until loaded a module file */ > - if (!ret) { > - break; > - } > + for (i = 0; i < ARRAY_SIZE(dirs); i++) { > + fname = g_strdup_printf("%s/%s%s%s", > + dirs[i], prefix, lib_name, HOST_DSOSUF); > + ret = module_load_file(fname); > + g_free(fname); > + fname = NULL; > + /* Try loading until loaded a module file */ > + if (!ret) { > + break; > } > } > > -- > 2.5.5 > >
On Wed, Jun 22, 2016 at 05:35:54PM -0400, Colin Lord wrote: > + for (i = 0; i < ARRAY_SIZE(block_driver_modules); ++i) { > + if (!strcmp(block_driver_modules[i].format_name, format_name)) { > + block_module_load_one(block_driver_modules[i].library_name); > + /* Copying code is not nice, but this way the current discovery is > + * not modified. Calling recursively could fail if the library > + * has been deleted. > + */ > + QLIST_FOREACH(drv1, &bdrv_drivers, list) { > + if (!strcmp(drv1->format_name, format_name)) { > + return drv1; > + } > + } > + } > + } > + > return NULL; > } No recursion: static BlockDriver *bdrv_do_find_format(const char *format_name) { BlockDriver *drv1; QLIST_FOREACH(drv1, &bdrv_drivers, list) { if (!strcmp(drv1->format_name, format_name)) { return drv1; } } return NULL; } BlockDriver *bdrv_find_format(const char *format_name) { BlockDriver *drv; size_t i; drv = bdrv_do_find_format(format_name); if (drv) { return drv; } /* The driver isn't registered, maybe we need to load a module */ for (i = 0; i < ARRAY_SIZE(block_driver_modules); ++i) { if (!strcmp(block_driver_modules[i].format_name, format_name)) { block_module_load_one(block_driver_modules[i].library_name); break; } } return bdrv_do_find_format(format_name); } > > @@ -447,8 +466,15 @@ int get_tmp_filename(char *filename, int size) > static BlockDriver *find_hdev_driver(const char *filename) > { > int score_max = 0, score; > + size_t i; > BlockDriver *drv = NULL, *d; > > + for (i = 0; i < ARRAY_SIZE(block_driver_modules); ++i) { > + if (block_driver_modules[i].has_probe_device) { > + block_module_load_one(block_driver_modules[i].library_name); > + } > + } This patch series needs to solve probing so that we don't end up loading all block drivers. Fam's suggestion for a built-in probe.c sounds good to me.
On Fri, Jun 24, 2016 at 11:04:43AM +0100, Stefan Hajnoczi wrote: > On Wed, Jun 22, 2016 at 05:35:54PM -0400, Colin Lord wrote: > > > > > @@ -447,8 +466,15 @@ int get_tmp_filename(char *filename, int size) > > static BlockDriver *find_hdev_driver(const char *filename) > > { > > int score_max = 0, score; > > + size_t i; > > BlockDriver *drv = NULL, *d; > > > > + for (i = 0; i < ARRAY_SIZE(block_driver_modules); ++i) { > > + if (block_driver_modules[i].has_probe_device) { > > + block_module_load_one(block_driver_modules[i].library_name); > > + } > > + } > > This patch series needs to solve probing so that we don't end up loading > all block drivers. Fam's suggestion for a built-in probe.c sounds good > to me. Do we really care if probing loads all drivers ? Last time we discussed this I thought we decided that because probing almost always leads to security vulnerabilities, no one should use it by default and so we don't really need to worry about optimizing it. Regards, Daniel
On Fri, 06/24 11:37, Daniel P. Berrange wrote: > On Fri, Jun 24, 2016 at 11:04:43AM +0100, Stefan Hajnoczi wrote: > > On Wed, Jun 22, 2016 at 05:35:54PM -0400, Colin Lord wrote: > > > > > > > > @@ -447,8 +466,15 @@ int get_tmp_filename(char *filename, int size) > > > static BlockDriver *find_hdev_driver(const char *filename) > > > { > > > int score_max = 0, score; > > > + size_t i; > > > BlockDriver *drv = NULL, *d; > > > > > > + for (i = 0; i < ARRAY_SIZE(block_driver_modules); ++i) { > > > + if (block_driver_modules[i].has_probe_device) { > > > + block_module_load_one(block_driver_modules[i].library_name); > > > + } > > > + } > > > > This patch series needs to solve probing so that we don't end up loading > > all block drivers. Fam's suggestion for a built-in probe.c sounds good > > to me. > > Do we really care if probing loads all drivers ? Last time we discussed > this I thought we decided that because probing almost always leads to > security vulnerabilities, no one should use it by default and so we > don't really need to worry about optimizing it. Does this mean we can drop "has_probe" and "has_probe_device" from the generated header and load all modules for probe? If so, I'd like to again stress my preference for a "simplified" code "parser": All of current block_init() calls except quorum can be converted with this: #define BLOCK_DRIVER_EXPORT(fmt_name, prot_name, drv) \ static void bdrv_ ## fmt_name ## _ ## prot_name(void) \ { \ if (strlen(#fmt_name)) { \ drv->format_name = #fmt_name; \ } \ if (strlen(#prot_name)) { \ drv->protocol_name = #prot_name; \ } \ bdrv_register(&(drv)); \ } \ block_init(bdrv_ ## fmt_name ## prot_name) curl.c: BLOCK_DRIVER_EXPORT(http, http, bdrv_http); BLOCK_DRIVER_EXPORT(https, https, bdrv_https); BLOCK_DRIVER_EXPORT(ftp, ftp, bdrv_ftp); ... iscsi.c (on top of patch 1): BLOCK_DRIVER_EXPORT(iscsi, iscsi, bdrv_iscsi); vmdk.c: BLOCK_DRIVER_EXPORT(vmdk,, bdrv_vmdk); Then the python generator greps for BLOCK_DRIVER_EXPORT, instead of 'static BlockDriver', which is much more reliable (in the sense of whitespace subtlety, field name changing, etc). Fam
On Fri, Jun 24, 2016 at 11:37:56AM +0100, Daniel P. Berrange wrote: > On Fri, Jun 24, 2016 at 11:04:43AM +0100, Stefan Hajnoczi wrote: > > On Wed, Jun 22, 2016 at 05:35:54PM -0400, Colin Lord wrote: > > > > > > > > @@ -447,8 +466,15 @@ int get_tmp_filename(char *filename, int size) > > > static BlockDriver *find_hdev_driver(const char *filename) > > > { > > > int score_max = 0, score; > > > + size_t i; > > > BlockDriver *drv = NULL, *d; > > > > > > + for (i = 0; i < ARRAY_SIZE(block_driver_modules); ++i) { > > > + if (block_driver_modules[i].has_probe_device) { > > > + block_module_load_one(block_driver_modules[i].library_name); > > > + } > > > + } > > > > This patch series needs to solve probing so that we don't end up loading > > all block drivers. Fam's suggestion for a built-in probe.c sounds good > > to me. > > Do we really care if probing loads all drivers ? Last time we discussed > this I thought we decided that because probing almost always leads to > security vulnerabilities, no one should use it by default and so we > don't really need to worry about optimizing it. If the code to handle probing is simple then doing it is nice. Stefan
diff --git a/Makefile b/Makefile index 8f8b6a2..461187c 100644 --- a/Makefile +++ b/Makefile @@ -247,9 +247,6 @@ Makefile: $(version-obj-y) $(version-lobj-y) libqemustub.a: $(stub-obj-y) libqemuutil.a: $(util-obj-y) -block-modules = $(foreach o,$(block-obj-m),"$(basename $(subst /,-,$o))",) NULL -util/module.o-cflags = -D'CONFIG_BLOCK_MODULES=$(block-modules)' - ###################################################################### qemu-img.o: qemu-img-cmds.h diff --git a/block.c b/block.c index f54bc25..7a91434 100644 --- a/block.c +++ b/block.c @@ -26,6 +26,7 @@ #include "block/block_int.h" #include "block/blockjob.h" #include "qemu/error-report.h" +#include "qemu/module_block.h" #include "qemu/module.h" #include "qapi/qmp/qerror.h" #include "qapi/qmp/qbool.h" @@ -242,11 +243,29 @@ BlockDriverState *bdrv_new(void) BlockDriver *bdrv_find_format(const char *format_name) { BlockDriver *drv1; + size_t i; + QLIST_FOREACH(drv1, &bdrv_drivers, list) { if (!strcmp(drv1->format_name, format_name)) { return drv1; } } + + for (i = 0; i < ARRAY_SIZE(block_driver_modules); ++i) { + if (!strcmp(block_driver_modules[i].format_name, format_name)) { + block_module_load_one(block_driver_modules[i].library_name); + /* Copying code is not nice, but this way the current discovery is + * not modified. Calling recursively could fail if the library + * has been deleted. + */ + QLIST_FOREACH(drv1, &bdrv_drivers, list) { + if (!strcmp(drv1->format_name, format_name)) { + return drv1; + } + } + } + } + return NULL; } @@ -447,8 +466,15 @@ int get_tmp_filename(char *filename, int size) static BlockDriver *find_hdev_driver(const char *filename) { int score_max = 0, score; + size_t i; BlockDriver *drv = NULL, *d; + for (i = 0; i < ARRAY_SIZE(block_driver_modules); ++i) { + if (block_driver_modules[i].has_probe_device) { + block_module_load_one(block_driver_modules[i].library_name); + } + } + QLIST_FOREACH(d, &bdrv_drivers, list) { if (d->bdrv_probe_device) { score = d->bdrv_probe_device(filename); @@ -470,6 +496,7 @@ BlockDriver *bdrv_find_protocol(const char *filename, char protocol[128]; int len; const char *p; + size_t i; /* TODO Drivers without bdrv_file_open must be specified explicitly */ @@ -496,6 +523,7 @@ BlockDriver *bdrv_find_protocol(const char *filename, len = sizeof(protocol) - 1; memcpy(protocol, filename, len); protocol[len] = '\0'; + QLIST_FOREACH(drv1, &bdrv_drivers, list) { if (drv1->protocol_name && !strcmp(drv1->protocol_name, protocol)) { @@ -503,6 +531,23 @@ BlockDriver *bdrv_find_protocol(const char *filename, } } + for (i = 0; i < ARRAY_SIZE(block_driver_modules); ++i) { + if (block_driver_modules[i].protocol_name && + !strcmp(block_driver_modules[i].protocol_name, protocol)) { + block_module_load_one(block_driver_modules[i].library_name); + /* Copying code is not nice, but this way the current discovery is + * not modified. Calling recursively could fail if the library + * has been deleted. + */ + QLIST_FOREACH(drv1, &bdrv_drivers, list) { + if (drv1->protocol_name && + !strcmp(drv1->protocol_name, protocol)) { + return drv1; + } + } + } + } + error_setg(errp, "Unknown protocol '%s'", protocol); return NULL; } @@ -525,8 +570,15 @@ BlockDriver *bdrv_probe_all(const uint8_t *buf, int buf_size, const char *filename) { int score_max = 0, score; + size_t i; BlockDriver *drv = NULL, *d; + for (i = 0; i < ARRAY_SIZE(block_driver_modules); ++i) { + if (block_driver_modules[i].has_probe) { + block_module_load_one(block_driver_modules[i].library_name); + } + } + QLIST_FOREACH(d, &bdrv_drivers, list) { if (d->bdrv_probe) { score = d->bdrv_probe(buf, buf_size, filename); @@ -2738,26 +2790,42 @@ static int qsort_strcmp(const void *a, const void *b) return strcmp(a, b); } +static const char **add_format(const char **formats, int *count, + const char *format_name) +{ + int i; + + for (i = 0; i < *count; i++) { + if (!strcmp(formats[i], format_name)) { + return formats; + } + } + + *count += 1; + formats = g_renew(const char *, formats, *count); + formats[*count] = format_name; + return formats; +} + void bdrv_iterate_format(void (*it)(void *opaque, const char *name), void *opaque) { BlockDriver *drv; int count = 0; int i; + size_t n; const char **formats = NULL; QLIST_FOREACH(drv, &bdrv_drivers, list) { if (drv->format_name) { - bool found = false; - int i = count; - while (formats && i && !found) { - found = !strcmp(formats[--i], drv->format_name); - } + formats = add_format(formats, &count, drv->format_name); + } + } - if (!found) { - formats = g_renew(const char *, formats, count + 1); - formats[count++] = drv->format_name; - } + for (n = 0; n < ARRAY_SIZE(block_driver_modules); ++n) { + if (block_driver_modules[n].format_name) { + formats = add_format(formats, &count, + block_driver_modules[n].format_name); } } diff --git a/include/qemu/module.h b/include/qemu/module.h index 2370708..4729858 100644 --- a/include/qemu/module.h +++ b/include/qemu/module.h @@ -52,9 +52,12 @@ typedef enum { #define qapi_init(function) module_init(function, MODULE_INIT_QAPI) #define type_init(function) module_init(function, MODULE_INIT_QOM) +#define block_module_load_one(lib) module_load_one("block-", lib); + void register_module_init(void (*fn)(void), module_init_type type); void register_dso_module_init(void (*fn)(void), module_init_type type); void module_call_init(module_init_type type); +void module_load_one(const char *prefix, const char *lib_name); #endif diff --git a/util/module.c b/util/module.c index ce058ae..dbc6e52 100644 --- a/util/module.c +++ b/util/module.c @@ -91,14 +91,11 @@ void register_dso_module_init(void (*fn)(void), module_init_type type) QTAILQ_INSERT_TAIL(&dso_init_list, e, node); } -static void module_load(module_init_type type); - void module_call_init(module_init_type type) { ModuleTypeList *l; ModuleEntry *e; - module_load(type); l = find_type(type); QTAILQ_FOREACH(e, l, node) { @@ -163,14 +160,10 @@ out: } #endif -static void module_load(module_init_type type) +void module_load_one(const char *prefix, const char *lib_name) { #ifdef CONFIG_MODULES char *fname = NULL; - const char **mp; - static const char *block_modules[] = { - CONFIG_BLOCK_MODULES - }; char *exec_dir; char *dirs[3]; int i = 0; @@ -181,15 +174,6 @@ static void module_load(module_init_type type) return; } - switch (type) { - case MODULE_INIT_BLOCK: - mp = block_modules; - break; - default: - /* no other types have dynamic modules for now*/ - return; - } - exec_dir = qemu_get_exec_dir(); dirs[i++] = g_strdup_printf("%s", CONFIG_QEMU_MODDIR); dirs[i++] = g_strdup_printf("%s/..", exec_dir ? : ""); @@ -198,16 +182,15 @@ static void module_load(module_init_type type) g_free(exec_dir); exec_dir = NULL; - for ( ; *mp; mp++) { - for (i = 0; i < ARRAY_SIZE(dirs); i++) { - fname = g_strdup_printf("%s/%s%s", dirs[i], *mp, HOST_DSOSUF); - ret = module_load_file(fname); - g_free(fname); - fname = NULL; - /* Try loading until loaded a module file */ - if (!ret) { - break; - } + for (i = 0; i < ARRAY_SIZE(dirs); i++) { + fname = g_strdup_printf("%s/%s%s%s", + dirs[i], prefix, lib_name, HOST_DSOSUF); + ret = module_load_file(fname); + g_free(fname); + fname = NULL; + /* Try loading until loaded a module file */ + if (!ret) { + break; } }