| Message ID | 1470679640-18366-4-git-send-email-clord@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 08.08.2016 20:07, 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. > > In addition, only the protocol drivers are being modularized, as they > are the only ones which see significant performance benefits. The format > drivers do not generally link to external libraries, so modularizing > them is of no benefit from a performance perspective. > > All the necessary module information is located in a new structure found > in module_block.h > > Signed-off-by: Marc Marí <markmb@redhat.com> > Signed-off-by: Colin Lord <clord@redhat.com> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > Makefile | 3 --- > block.c | 62 +++++++++++++++++++++++++++++++++++++++++++++------ > block/Makefile.objs | 3 +-- > include/qemu/module.h | 3 +++ > util/module.c | 38 +++++++++---------------------- > 5 files changed, 70 insertions(+), 39 deletions(-) > [...] > diff --git a/block.c b/block.c > index 30d64e6..6c5e249 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 "module_block.h" > #include "qemu/module.h" > #include "qapi/qmp/qerror.h" > #include "qapi/qmp/qbool.h" > @@ -241,17 +242,40 @@ BlockDriverState *bdrv_new(void) > return bs; > } > > -BlockDriver *bdrv_find_format(const char *format_name) > +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 *drv1; > + size_t i; > + > + drv1 = bdrv_do_find_format(format_name); > + if (drv1) { > + return drv1; > + } > + > + /* 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); > +} > + Did you reintroduce this function for dmg? I thought Fam is taking care of that? I'm confused as to how Fam's patch for dmg and this series are supposed to interact; the fact that the script added in patch 2 breaks down with Fam's patch isn't exactly helping... Hm, so is this series now supposed to be applied without Fam's patch with the idea of sorting dmg out later on? Max > static int bdrv_is_whitelisted(BlockDriver *drv, bool read_only) > { > static const char *whitelist_rw[] = {
On 08/10/2016 02:37 PM, Max Reitz wrote: > On 08.08.2016 20:07, 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. >> >> In addition, only the protocol drivers are being modularized, as they >> are the only ones which see significant performance benefits. The format >> drivers do not generally link to external libraries, so modularizing >> them is of no benefit from a performance perspective. >> >> All the necessary module information is located in a new structure found >> in module_block.h >> >> Signed-off-by: Marc Marí <markmb@redhat.com> >> Signed-off-by: Colin Lord <clord@redhat.com> >> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> >> --- >> Makefile | 3 --- >> block.c | 62 +++++++++++++++++++++++++++++++++++++++++++++------ >> block/Makefile.objs | 3 +-- >> include/qemu/module.h | 3 +++ >> util/module.c | 38 +++++++++---------------------- >> 5 files changed, 70 insertions(+), 39 deletions(-) >> > > [...] > >> diff --git a/block.c b/block.c >> index 30d64e6..6c5e249 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 "module_block.h" >> #include "qemu/module.h" >> #include "qapi/qmp/qerror.h" >> #include "qapi/qmp/qbool.h" >> @@ -241,17 +242,40 @@ BlockDriverState *bdrv_new(void) >> return bs; >> } >> >> -BlockDriver *bdrv_find_format(const char *format_name) >> +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 *drv1; >> + size_t i; >> + >> + drv1 = bdrv_do_find_format(format_name); >> + if (drv1) { >> + return drv1; >> + } >> + >> + /* 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); >> +} >> + > > Did you reintroduce this function for dmg? I thought Fam is taking care > of that? I'm confused as to how Fam's patch for dmg and this series are > supposed to interact; the fact that the script added in patch 2 breaks > down with Fam's patch isn't exactly helping... > > Hm, so is this series now supposed to be applied without Fam's patch > with the idea of sorting dmg out later on? > > Max > >> static int bdrv_is_whitelisted(BlockDriver *drv, bool read_only) >> { >> static const char *whitelist_rw[] = { > I'm not completely sure how Fam's patch is supposed to interact with this series actually. I'm kind of hoping it can be done on top of my patches at some future point, but in either case this revision was not done with the dmg patch in mind. The change in find_format was actually due to a bug I discovered in my patch series (I fixed it in v6, but you may have missed that). Essentially, if a user specifies the driver explicitly as part of their call to qemu, eg driver=gluster, there was a bug in v5 where if the driver was modularized, it would not be found/loaded. So since gluster was modularized, if you said driver=gluster on the command line, the gluster module would not be found. The modules could be found by probing perfectly fine, this only happened when the driver was specified manually. The reason is because the drivers get searched based on the format field if they're specified manually, which means bdrv_find_format gets called when the driver is specified on the command line. This makes it necessary for bdrv_find_format to take into account modularized drivers even though the format drivers are not being modularized. That's also why the format field was added to the module_block header file again. Colin
On 10.08.2016 21:04, Colin Lord wrote: > On 08/10/2016 02:37 PM, Max Reitz wrote: >> On 08.08.2016 20:07, 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. >>> >>> In addition, only the protocol drivers are being modularized, as they >>> are the only ones which see significant performance benefits. The format >>> drivers do not generally link to external libraries, so modularizing >>> them is of no benefit from a performance perspective. >>> >>> All the necessary module information is located in a new structure found >>> in module_block.h >>> >>> Signed-off-by: Marc Marí <markmb@redhat.com> >>> Signed-off-by: Colin Lord <clord@redhat.com> >>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> >>> --- >>> Makefile | 3 --- >>> block.c | 62 +++++++++++++++++++++++++++++++++++++++++++++------ >>> block/Makefile.objs | 3 +-- >>> include/qemu/module.h | 3 +++ >>> util/module.c | 38 +++++++++---------------------- >>> 5 files changed, 70 insertions(+), 39 deletions(-) >>> >> >> [...] >> >>> diff --git a/block.c b/block.c >>> index 30d64e6..6c5e249 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 "module_block.h" >>> #include "qemu/module.h" >>> #include "qapi/qmp/qerror.h" >>> #include "qapi/qmp/qbool.h" >>> @@ -241,17 +242,40 @@ BlockDriverState *bdrv_new(void) >>> return bs; >>> } >>> >>> -BlockDriver *bdrv_find_format(const char *format_name) >>> +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 *drv1; >>> + size_t i; >>> + >>> + drv1 = bdrv_do_find_format(format_name); >>> + if (drv1) { >>> + return drv1; >>> + } >>> + >>> + /* 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); >>> +} >>> + >> >> Did you reintroduce this function for dmg? I thought Fam is taking care >> of that? I'm confused as to how Fam's patch for dmg and this series are >> supposed to interact; the fact that the script added in patch 2 breaks >> down with Fam's patch isn't exactly helping... >> >> Hm, so is this series now supposed to be applied without Fam's patch >> with the idea of sorting dmg out later on? >> >> Max >> >>> static int bdrv_is_whitelisted(BlockDriver *drv, bool read_only) >>> { >>> static const char *whitelist_rw[] = { >> > I'm not completely sure how Fam's patch is supposed to interact with > this series actually. I'm kind of hoping it can be done on top of my > patches at some future point, but in either case this revision was not > done with the dmg patch in mind. The change in find_format was actually > due to a bug I discovered in my patch series (I fixed it in v6, but you > may have missed that). > > Essentially, if a user specifies the driver explicitly as part of their > call to qemu, eg driver=gluster, there was a bug in v5 where if the > driver was modularized, it would not be found/loaded. So since gluster > was modularized, if you said driver=gluster on the command line, the > gluster module would not be found. The modules could be found by probing > perfectly fine, this only happened when the driver was specified > manually. The reason is because the drivers get searched based on the > format field if they're specified manually, which means bdrv_find_format > gets called when the driver is specified on the command line. This makes > it necessary for bdrv_find_format to take into account modularized > drivers even though the format drivers are not being modularized. That's > also why the format field was added to the module_block header file again. Ah, that makes sense, thanks for explaining. Patches 1-3: Reviewed-by: Max Reitz <mreitz@redhat.com>
On 08/10/2016 03:04 PM, Colin Lord wrote: > On 08/10/2016 02:37 PM, Max Reitz wrote: >> On 08.08.2016 20:07, 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. >>> >>> In addition, only the protocol drivers are being modularized, as they >>> are the only ones which see significant performance benefits. The format >>> drivers do not generally link to external libraries, so modularizing >>> them is of no benefit from a performance perspective. >>> >>> All the necessary module information is located in a new structure found >>> in module_block.h >>> >>> Signed-off-by: Marc Marí <markmb@redhat.com> >>> Signed-off-by: Colin Lord <clord@redhat.com> >>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> >>> --- >>> Makefile | 3 --- >>> block.c | 62 +++++++++++++++++++++++++++++++++++++++++++++------ >>> block/Makefile.objs | 3 +-- >>> include/qemu/module.h | 3 +++ >>> util/module.c | 38 +++++++++---------------------- >>> 5 files changed, 70 insertions(+), 39 deletions(-) >>> >> >> [...] >> >>> diff --git a/block.c b/block.c >>> index 30d64e6..6c5e249 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 "module_block.h" >>> #include "qemu/module.h" >>> #include "qapi/qmp/qerror.h" >>> #include "qapi/qmp/qbool.h" >>> @@ -241,17 +242,40 @@ BlockDriverState *bdrv_new(void) >>> return bs; >>> } >>> >>> -BlockDriver *bdrv_find_format(const char *format_name) >>> +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 *drv1; >>> + size_t i; >>> + >>> + drv1 = bdrv_do_find_format(format_name); >>> + if (drv1) { >>> + return drv1; >>> + } >>> + >>> + /* 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); >>> +} >>> + >> >> Did you reintroduce this function for dmg? I thought Fam is taking care >> of that? I'm confused as to how Fam's patch for dmg and this series are >> supposed to interact; the fact that the script added in patch 2 breaks >> down with Fam's patch isn't exactly helping... >> >> Hm, so is this series now supposed to be applied without Fam's patch >> with the idea of sorting dmg out later on? >> >> Max >> >>> static int bdrv_is_whitelisted(BlockDriver *drv, bool read_only) >>> { >>> static const char *whitelist_rw[] = { >> > I'm not completely sure how Fam's patch is supposed to interact with > this series actually. I'm kind of hoping it can be done on top of my > patches at some future point, but in either case this revision was not > done with the dmg patch in mind. The change in find_format was actually > due to a bug I discovered in my patch series (I fixed it in v6, but you > may have missed that). > > Essentially, if a user specifies the driver explicitly as part of their > call to qemu, eg driver=gluster, there was a bug in v5 where if the > driver was modularized, it would not be found/loaded. So since gluster > was modularized, if you said driver=gluster on the command line, the > gluster module would not be found. The modules could be found by probing > perfectly fine, this only happened when the driver was specified > manually. The reason is because the drivers get searched based on the > format field if they're specified manually, which means bdrv_find_format > gets called when the driver is specified on the command line. This makes > it necessary for bdrv_find_format to take into account modularized > drivers even though the format drivers are not being modularized. That's > also why the format field was added to the module_block header file again. > > Colin > Oops, this was meant to be a reply to your response of patch 4/4, in case anyone gets confused. Colin
On 08/10/2016 03:24 PM, Colin Lord wrote: > On 08/10/2016 03:04 PM, Colin Lord wrote: >> On 08/10/2016 02:37 PM, Max Reitz wrote: >>> On 08.08.2016 20:07, 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. >>>> >>>> In addition, only the protocol drivers are being modularized, as they >>>> are the only ones which see significant performance benefits. The format >>>> drivers do not generally link to external libraries, so modularizing >>>> them is of no benefit from a performance perspective. >>>> >>>> All the necessary module information is located in a new structure found >>>> in module_block.h >>>> >>>> Signed-off-by: Marc Marí <markmb@redhat.com> >>>> Signed-off-by: Colin Lord <clord@redhat.com> >>>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> >>>> --- >>>> Makefile | 3 --- >>>> block.c | 62 +++++++++++++++++++++++++++++++++++++++++++++------ >>>> block/Makefile.objs | 3 +-- >>>> include/qemu/module.h | 3 +++ >>>> util/module.c | 38 +++++++++---------------------- >>>> 5 files changed, 70 insertions(+), 39 deletions(-) >>>> >>> >>> [...] >>> >>>> diff --git a/block.c b/block.c >>>> index 30d64e6..6c5e249 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 "module_block.h" >>>> #include "qemu/module.h" >>>> #include "qapi/qmp/qerror.h" >>>> #include "qapi/qmp/qbool.h" >>>> @@ -241,17 +242,40 @@ BlockDriverState *bdrv_new(void) >>>> return bs; >>>> } >>>> >>>> -BlockDriver *bdrv_find_format(const char *format_name) >>>> +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 *drv1; >>>> + size_t i; >>>> + >>>> + drv1 = bdrv_do_find_format(format_name); >>>> + if (drv1) { >>>> + return drv1; >>>> + } >>>> + >>>> + /* 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); >>>> +} >>>> + >>> >>> Did you reintroduce this function for dmg? I thought Fam is taking care >>> of that? I'm confused as to how Fam's patch for dmg and this series are >>> supposed to interact; the fact that the script added in patch 2 breaks >>> down with Fam's patch isn't exactly helping... >>> >>> Hm, so is this series now supposed to be applied without Fam's patch >>> with the idea of sorting dmg out later on? >>> >>> Max >>> >>>> static int bdrv_is_whitelisted(BlockDriver *drv, bool read_only) >>>> { >>>> static const char *whitelist_rw[] = { >>> >> I'm not completely sure how Fam's patch is supposed to interact with >> this series actually. I'm kind of hoping it can be done on top of my >> patches at some future point, but in either case this revision was not >> done with the dmg patch in mind. The change in find_format was actually >> due to a bug I discovered in my patch series (I fixed it in v6, but you >> may have missed that). >> >> Essentially, if a user specifies the driver explicitly as part of their >> call to qemu, eg driver=gluster, there was a bug in v5 where if the >> driver was modularized, it would not be found/loaded. So since gluster >> was modularized, if you said driver=gluster on the command line, the >> gluster module would not be found. The modules could be found by probing >> perfectly fine, this only happened when the driver was specified >> manually. The reason is because the drivers get searched based on the >> format field if they're specified manually, which means bdrv_find_format >> gets called when the driver is specified on the command line. This makes >> it necessary for bdrv_find_format to take into account modularized >> drivers even though the format drivers are not being modularized. That's >> also why the format field was added to the module_block header file again. >> >> Colin >> > Oops, this was meant to be a reply to your response of patch 4/4, in > case anyone gets confused. > > Colin > Nevermind, looks like I was confused. Seems like my eyes are playing tricks on me, sorry for the spam.
On Wed, 08/10 21:06, Max Reitz wrote: > On 10.08.2016 21:04, Colin Lord wrote: > > On 08/10/2016 02:37 PM, Max Reitz wrote: > >> On 08.08.2016 20:07, 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. > >>> > >>> In addition, only the protocol drivers are being modularized, as they > >>> are the only ones which see significant performance benefits. The format > >>> drivers do not generally link to external libraries, so modularizing > >>> them is of no benefit from a performance perspective. > >>> > >>> All the necessary module information is located in a new structure found > >>> in module_block.h > >>> > >>> Signed-off-by: Marc Marí <markmb@redhat.com> > >>> Signed-off-by: Colin Lord <clord@redhat.com> > >>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> > >>> --- > >>> Makefile | 3 --- > >>> block.c | 62 +++++++++++++++++++++++++++++++++++++++++++++------ > >>> block/Makefile.objs | 3 +-- > >>> include/qemu/module.h | 3 +++ > >>> util/module.c | 38 +++++++++---------------------- > >>> 5 files changed, 70 insertions(+), 39 deletions(-) > >>> > >> > >> [...] > >> > >>> diff --git a/block.c b/block.c > >>> index 30d64e6..6c5e249 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 "module_block.h" > >>> #include "qemu/module.h" > >>> #include "qapi/qmp/qerror.h" > >>> #include "qapi/qmp/qbool.h" > >>> @@ -241,17 +242,40 @@ BlockDriverState *bdrv_new(void) > >>> return bs; > >>> } > >>> > >>> -BlockDriver *bdrv_find_format(const char *format_name) > >>> +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 *drv1; > >>> + size_t i; > >>> + > >>> + drv1 = bdrv_do_find_format(format_name); > >>> + if (drv1) { > >>> + return drv1; > >>> + } > >>> + > >>> + /* 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); > >>> +} > >>> + > >> > >> Did you reintroduce this function for dmg? I thought Fam is taking care > >> of that? I'm confused as to how Fam's patch for dmg and this series are > >> supposed to interact; the fact that the script added in patch 2 breaks > >> down with Fam's patch isn't exactly helping... > >> > >> Hm, so is this series now supposed to be applied without Fam's patch > >> with the idea of sorting dmg out later on? > >> > >> Max > >> > >>> static int bdrv_is_whitelisted(BlockDriver *drv, bool read_only) > >>> { > >>> static const char *whitelist_rw[] = { > >> > > I'm not completely sure how Fam's patch is supposed to interact with > > this series actually. I'm kind of hoping it can be done on top of my > > patches at some future point, but in either case this revision was not > > done with the dmg patch in mind. The change in find_format was actually > > due to a bug I discovered in my patch series (I fixed it in v6, but you > > may have missed that). > > > > Essentially, if a user specifies the driver explicitly as part of their > > call to qemu, eg driver=gluster, there was a bug in v5 where if the > > driver was modularized, it would not be found/loaded. So since gluster > > was modularized, if you said driver=gluster on the command line, the > > gluster module would not be found. The modules could be found by probing > > perfectly fine, this only happened when the driver was specified > > manually. The reason is because the drivers get searched based on the > > format field if they're specified manually, which means bdrv_find_format > > gets called when the driver is specified on the command line. This makes > > it necessary for bdrv_find_format to take into account modularized > > drivers even though the format drivers are not being modularized. That's > > also why the format field was added to the module_block header file again. > > Ah, that makes sense, thanks for explaining. > > Patches 1-3: > > Reviewed-by: Max Reitz <mreitz@redhat.com> > If we apply this series first, there will be an intermediate state that the main QEMU binary is linked to libbz2. It doesn't hurt us, but it is better to make it clear, in case downstream backportings want to keep the library dependency intact. So, let's add a word to this commit message, at least. Fam
On 08/10/2016 11:23 PM, Fam Zheng wrote: > On Wed, 08/10 21:06, Max Reitz wrote: >> On 10.08.2016 21:04, Colin Lord wrote: >>> On 08/10/2016 02:37 PM, Max Reitz wrote: >>>> On 08.08.2016 20:07, 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. >>>>> >>>>> In addition, only the protocol drivers are being modularized, as they >>>>> are the only ones which see significant performance benefits. The format >>>>> drivers do not generally link to external libraries, so modularizing >>>>> them is of no benefit from a performance perspective. >>>>> >>>>> All the necessary module information is located in a new structure found >>>>> in module_block.h >>>>> >>>>> Signed-off-by: Marc Marí <markmb@redhat.com> >>>>> Signed-off-by: Colin Lord <clord@redhat.com> >>>>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> >>>>> --- >>>>> Makefile | 3 --- >>>>> block.c | 62 +++++++++++++++++++++++++++++++++++++++++++++------ >>>>> block/Makefile.objs | 3 +-- >>>>> include/qemu/module.h | 3 +++ >>>>> util/module.c | 38 +++++++++---------------------- >>>>> 5 files changed, 70 insertions(+), 39 deletions(-) >>>>> >>>> >>>> [...] >>>> >>>>> diff --git a/block.c b/block.c >>>>> index 30d64e6..6c5e249 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 "module_block.h" >>>>> #include "qemu/module.h" >>>>> #include "qapi/qmp/qerror.h" >>>>> #include "qapi/qmp/qbool.h" >>>>> @@ -241,17 +242,40 @@ BlockDriverState *bdrv_new(void) >>>>> return bs; >>>>> } >>>>> >>>>> -BlockDriver *bdrv_find_format(const char *format_name) >>>>> +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 *drv1; >>>>> + size_t i; >>>>> + >>>>> + drv1 = bdrv_do_find_format(format_name); >>>>> + if (drv1) { >>>>> + return drv1; >>>>> + } >>>>> + >>>>> + /* 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); >>>>> +} >>>>> + >>>> >>>> Did you reintroduce this function for dmg? I thought Fam is taking care >>>> of that? I'm confused as to how Fam's patch for dmg and this series are >>>> supposed to interact; the fact that the script added in patch 2 breaks >>>> down with Fam's patch isn't exactly helping... >>>> >>>> Hm, so is this series now supposed to be applied without Fam's patch >>>> with the idea of sorting dmg out later on? >>>> >>>> Max >>>> >>>>> static int bdrv_is_whitelisted(BlockDriver *drv, bool read_only) >>>>> { >>>>> static const char *whitelist_rw[] = { >>>> >>> I'm not completely sure how Fam's patch is supposed to interact with >>> this series actually. I'm kind of hoping it can be done on top of my >>> patches at some future point, but in either case this revision was not >>> done with the dmg patch in mind. The change in find_format was actually >>> due to a bug I discovered in my patch series (I fixed it in v6, but you >>> may have missed that). >>> >>> Essentially, if a user specifies the driver explicitly as part of their >>> call to qemu, eg driver=gluster, there was a bug in v5 where if the >>> driver was modularized, it would not be found/loaded. So since gluster >>> was modularized, if you said driver=gluster on the command line, the >>> gluster module would not be found. The modules could be found by probing >>> perfectly fine, this only happened when the driver was specified >>> manually. The reason is because the drivers get searched based on the >>> format field if they're specified manually, which means bdrv_find_format >>> gets called when the driver is specified on the command line. This makes >>> it necessary for bdrv_find_format to take into account modularized >>> drivers even though the format drivers are not being modularized. That's >>> also why the format field was added to the module_block header file again. >> >> Ah, that makes sense, thanks for explaining. >> >> Patches 1-3: >> >> Reviewed-by: Max Reitz <mreitz@redhat.com> >> > > If we apply this series first, there will be an intermediate state that the > main QEMU binary is linked to libbz2. It doesn't hurt us, but it is better to > make it clear, in case downstream backportings want to keep the library > dependency intact. > > So, let's add a word to this commit message, at least. > > Fam > > > I guess I'm a little confused about the issue. It seems to me the only difference between now and before is that if libbz2 is enabled, it will be linked to the main binary rather than a module. But before, the modules were always loaded unconditionally at startup anyway, so I'm not seeing how there is a difference. Either way libbz2 would be loaded. I'm just not really sure what sort of message I should be adding to the commit message to indicate this. Also, would you like me to try and port your patch for dmg to work on top of this series? I'd still prefer if this series was applied first because 1) if the dmg patch is done first, this series will have to change parts of that patch anyway since the block module objects aren't being loaded unconditionally anymore. That means the bz2 parts would have to be converted to being dynamically linked at runtime, so it makes sense to me to just do it that way the first time rather than going back to change it. And 2) I am leaving soon and may or may not have time to make this series work on top of the dmg patch. But I'm happy to try and make your patch to work on top of this series in the little time I have before I leave. thanks, Colin
On Thu, 08/11 12:03, Colin Lord wrote: > On 08/10/2016 11:23 PM, Fam Zheng wrote: > > On Wed, 08/10 21:06, Max Reitz wrote: > >> On 10.08.2016 21:04, Colin Lord wrote: > >>> On 08/10/2016 02:37 PM, Max Reitz wrote: > >>>> On 08.08.2016 20:07, 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. > >>>>> > >>>>> In addition, only the protocol drivers are being modularized, as they > >>>>> are the only ones which see significant performance benefits. The format > >>>>> drivers do not generally link to external libraries, so modularizing > >>>>> them is of no benefit from a performance perspective. > >>>>> > >>>>> All the necessary module information is located in a new structure found > >>>>> in module_block.h > >>>>> > >>>>> Signed-off-by: Marc Marí <markmb@redhat.com> > >>>>> Signed-off-by: Colin Lord <clord@redhat.com> > >>>>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> > >>>>> --- > >>>>> Makefile | 3 --- > >>>>> block.c | 62 +++++++++++++++++++++++++++++++++++++++++++++------ > >>>>> block/Makefile.objs | 3 +-- > >>>>> include/qemu/module.h | 3 +++ > >>>>> util/module.c | 38 +++++++++---------------------- > >>>>> 5 files changed, 70 insertions(+), 39 deletions(-) > >>>>> > >>>> > >>>> [...] > >>>> > >>>>> diff --git a/block.c b/block.c > >>>>> index 30d64e6..6c5e249 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 "module_block.h" > >>>>> #include "qemu/module.h" > >>>>> #include "qapi/qmp/qerror.h" > >>>>> #include "qapi/qmp/qbool.h" > >>>>> @@ -241,17 +242,40 @@ BlockDriverState *bdrv_new(void) > >>>>> return bs; > >>>>> } > >>>>> > >>>>> -BlockDriver *bdrv_find_format(const char *format_name) > >>>>> +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 *drv1; > >>>>> + size_t i; > >>>>> + > >>>>> + drv1 = bdrv_do_find_format(format_name); > >>>>> + if (drv1) { > >>>>> + return drv1; > >>>>> + } > >>>>> + > >>>>> + /* 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); > >>>>> +} > >>>>> + > >>>> > >>>> Did you reintroduce this function for dmg? I thought Fam is taking care > >>>> of that? I'm confused as to how Fam's patch for dmg and this series are > >>>> supposed to interact; the fact that the script added in patch 2 breaks > >>>> down with Fam's patch isn't exactly helping... > >>>> > >>>> Hm, so is this series now supposed to be applied without Fam's patch > >>>> with the idea of sorting dmg out later on? > >>>> > >>>> Max > >>>> > >>>>> static int bdrv_is_whitelisted(BlockDriver *drv, bool read_only) > >>>>> { > >>>>> static const char *whitelist_rw[] = { > >>>> > >>> I'm not completely sure how Fam's patch is supposed to interact with > >>> this series actually. I'm kind of hoping it can be done on top of my > >>> patches at some future point, but in either case this revision was not > >>> done with the dmg patch in mind. The change in find_format was actually > >>> due to a bug I discovered in my patch series (I fixed it in v6, but you > >>> may have missed that). > >>> > >>> Essentially, if a user specifies the driver explicitly as part of their > >>> call to qemu, eg driver=gluster, there was a bug in v5 where if the > >>> driver was modularized, it would not be found/loaded. So since gluster > >>> was modularized, if you said driver=gluster on the command line, the > >>> gluster module would not be found. The modules could be found by probing > >>> perfectly fine, this only happened when the driver was specified > >>> manually. The reason is because the drivers get searched based on the > >>> format field if they're specified manually, which means bdrv_find_format > >>> gets called when the driver is specified on the command line. This makes > >>> it necessary for bdrv_find_format to take into account modularized > >>> drivers even though the format drivers are not being modularized. That's > >>> also why the format field was added to the module_block header file again. > >> > >> Ah, that makes sense, thanks for explaining. > >> > >> Patches 1-3: > >> > >> Reviewed-by: Max Reitz <mreitz@redhat.com> > >> > > > > If we apply this series first, there will be an intermediate state that the > > main QEMU binary is linked to libbz2. It doesn't hurt us, but it is better to > > make it clear, in case downstream backportings want to keep the library > > dependency intact. > > > > So, let's add a word to this commit message, at least. > > > > Fam > > > > > > > I guess I'm a little confused about the issue. It seems to me the only > difference between now and before is that if libbz2 is enabled, it will > be linked to the main binary rather than a module. But before, the > modules were always loaded unconditionally at startup anyway, so I'm not > seeing how there is a difference. Either way libbz2 would be loaded. I'm > just not really sure what sort of message I should be adding to the > commit message to indicate this. What I propose to be added in the commit message is this: --- 8< --- This spoils the purpose of 5505e8b76f (block/dmg: make it modular). Before this patch, if module build is enabled, block-dmg.so is linked to libbz2, whereas the main binary is not. In downstream, theoretically, it means only the qemu-block-extra package depends on libbz2, while the main QEMU package needn't to. With this patch, we (temporarily) change the case so that the main QEMU depends on libbz2 again. --- >8 --- > > Also, would you like me to try and port your patch for dmg to work on > top of this series? I'd still prefer if this series was applied first > because 1) if the dmg patch is done first, this series will have to > change parts of that patch anyway since the block module objects aren't > being loaded unconditionally anymore. That means the bz2 parts would > have to be converted to being dynamically linked at runtime, so it makes > sense to me to just do it that way the first time rather than going back > to change it. And 2) I am leaving soon and may or may not have time to > make this series work on top of the dmg patch. But I'm happy to try and > make your patch to work on top of this series in the little time I have > before I leave. I think it is fine for me to rebase on top of yours because: 1) practically it probably has no impact - Debian and ArchLinux both put block-dmg.so in the main QEMU package; 2) it's not a bug to introduce an extra dependency (the only special thing is, though, in this case it is not absolutely necessary, but it makes the development easier); 3) we will fix it within the same release. Fam
On 08/11/2016 09:29 PM, Fam Zheng wrote: > On Thu, 08/11 12:03, Colin Lord wrote: >> On 08/10/2016 11:23 PM, Fam Zheng wrote: >>> On Wed, 08/10 21:06, Max Reitz wrote: >>>> On 10.08.2016 21:04, Colin Lord wrote: >>>>> On 08/10/2016 02:37 PM, Max Reitz wrote: >>>>>> On 08.08.2016 20:07, 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. >>>>>>> >>>>>>> In addition, only the protocol drivers are being modularized, as they >>>>>>> are the only ones which see significant performance benefits. The format >>>>>>> drivers do not generally link to external libraries, so modularizing >>>>>>> them is of no benefit from a performance perspective. >>>>>>> >>>>>>> All the necessary module information is located in a new structure found >>>>>>> in module_block.h >>>>>>> >>>>>>> Signed-off-by: Marc Marí <markmb@redhat.com> >>>>>>> Signed-off-by: Colin Lord <clord@redhat.com> >>>>>>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> >>>>>>> --- >>>>>>> Makefile | 3 --- >>>>>>> block.c | 62 +++++++++++++++++++++++++++++++++++++++++++++------ >>>>>>> block/Makefile.objs | 3 +-- >>>>>>> include/qemu/module.h | 3 +++ >>>>>>> util/module.c | 38 +++++++++---------------------- >>>>>>> 5 files changed, 70 insertions(+), 39 deletions(-) >>>>>>> >>>>>> >>>>>> [...] >>>>>> >>>>>>> diff --git a/block.c b/block.c >>>>>>> index 30d64e6..6c5e249 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 "module_block.h" >>>>>>> #include "qemu/module.h" >>>>>>> #include "qapi/qmp/qerror.h" >>>>>>> #include "qapi/qmp/qbool.h" >>>>>>> @@ -241,17 +242,40 @@ BlockDriverState *bdrv_new(void) >>>>>>> return bs; >>>>>>> } >>>>>>> >>>>>>> -BlockDriver *bdrv_find_format(const char *format_name) >>>>>>> +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 *drv1; >>>>>>> + size_t i; >>>>>>> + >>>>>>> + drv1 = bdrv_do_find_format(format_name); >>>>>>> + if (drv1) { >>>>>>> + return drv1; >>>>>>> + } >>>>>>> + >>>>>>> + /* 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); >>>>>>> +} >>>>>>> + >>>>>> >>>>>> Did you reintroduce this function for dmg? I thought Fam is taking care >>>>>> of that? I'm confused as to how Fam's patch for dmg and this series are >>>>>> supposed to interact; the fact that the script added in patch 2 breaks >>>>>> down with Fam's patch isn't exactly helping... >>>>>> >>>>>> Hm, so is this series now supposed to be applied without Fam's patch >>>>>> with the idea of sorting dmg out later on? >>>>>> >>>>>> Max >>>>>> >>>>>>> static int bdrv_is_whitelisted(BlockDriver *drv, bool read_only) >>>>>>> { >>>>>>> static const char *whitelist_rw[] = { >>>>>> >>>>> I'm not completely sure how Fam's patch is supposed to interact with >>>>> this series actually. I'm kind of hoping it can be done on top of my >>>>> patches at some future point, but in either case this revision was not >>>>> done with the dmg patch in mind. The change in find_format was actually >>>>> due to a bug I discovered in my patch series (I fixed it in v6, but you >>>>> may have missed that). >>>>> >>>>> Essentially, if a user specifies the driver explicitly as part of their >>>>> call to qemu, eg driver=gluster, there was a bug in v5 where if the >>>>> driver was modularized, it would not be found/loaded. So since gluster >>>>> was modularized, if you said driver=gluster on the command line, the >>>>> gluster module would not be found. The modules could be found by probing >>>>> perfectly fine, this only happened when the driver was specified >>>>> manually. The reason is because the drivers get searched based on the >>>>> format field if they're specified manually, which means bdrv_find_format >>>>> gets called when the driver is specified on the command line. This makes >>>>> it necessary for bdrv_find_format to take into account modularized >>>>> drivers even though the format drivers are not being modularized. That's >>>>> also why the format field was added to the module_block header file again. >>>> >>>> Ah, that makes sense, thanks for explaining. >>>> >>>> Patches 1-3: >>>> >>>> Reviewed-by: Max Reitz <mreitz@redhat.com> >>>> >>> >>> If we apply this series first, there will be an intermediate state that the >>> main QEMU binary is linked to libbz2. It doesn't hurt us, but it is better to >>> make it clear, in case downstream backportings want to keep the library >>> dependency intact. >>> >>> So, let's add a word to this commit message, at least. >>> >>> Fam >>> >>> >>> >> I guess I'm a little confused about the issue. It seems to me the only >> difference between now and before is that if libbz2 is enabled, it will >> be linked to the main binary rather than a module. But before, the >> modules were always loaded unconditionally at startup anyway, so I'm not >> seeing how there is a difference. Either way libbz2 would be loaded. I'm >> just not really sure what sort of message I should be adding to the >> commit message to indicate this. > > What I propose to be added in the commit message is this: > > --- 8< --- > > This spoils the purpose of 5505e8b76f (block/dmg: make it modular). > > Before this patch, if module build is enabled, block-dmg.so is linked to > libbz2, whereas the main binary is not. In downstream, theoretically, it means > only the qemu-block-extra package depends on libbz2, while the main QEMU > package needn't to. With this patch, we (temporarily) change the case so that > the main QEMU depends on libbz2 again. > > --- >8 --- > >> >> Also, would you like me to try and port your patch for dmg to work on >> top of this series? I'd still prefer if this series was applied first >> because 1) if the dmg patch is done first, this series will have to >> change parts of that patch anyway since the block module objects aren't >> being loaded unconditionally anymore. That means the bz2 parts would >> have to be converted to being dynamically linked at runtime, so it makes >> sense to me to just do it that way the first time rather than going back >> to change it. And 2) I am leaving soon and may or may not have time to >> make this series work on top of the dmg patch. But I'm happy to try and >> make your patch to work on top of this series in the little time I have >> before I leave. > > I think it is fine for me to rebase on top of yours because: 1) practically > it probably has no impact - Debian and ArchLinux both put block-dmg.so in the > main QEMU package; 2) it's not a bug to introduce an extra dependency (the only > special thing is, though, in this case it is not absolutely necessary, but it > makes the development easier); 3) we will fix it within the same release. > > Fam > Okay sounds good, I'll add the comment and resend to the mailing list. thanks, Colin
diff --git a/Makefile b/Makefile index 4b9384e..c7aa8cd 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 30d64e6..6c5e249 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 "module_block.h" #include "qemu/module.h" #include "qapi/qmp/qerror.h" #include "qapi/qmp/qbool.h" @@ -241,17 +242,40 @@ BlockDriverState *bdrv_new(void) return bs; } -BlockDriver *bdrv_find_format(const char *format_name) +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 *drv1; + size_t i; + + drv1 = bdrv_do_find_format(format_name); + if (drv1) { + return drv1; + } + + /* 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); +} + static int bdrv_is_whitelisted(BlockDriver *drv, bool read_only) { static const char *whitelist_rw[] = { @@ -460,6 +484,19 @@ static BlockDriver *find_hdev_driver(const char *filename) return drv; } +static BlockDriver *bdrv_do_find_protocol(const char *protocol) +{ + BlockDriver *drv1; + + QLIST_FOREACH(drv1, &bdrv_drivers, list) { + if (drv1->protocol_name && !strcmp(drv1->protocol_name, protocol)) { + return drv1; + } + } + + return NULL; +} + BlockDriver *bdrv_find_protocol(const char *filename, bool allow_protocol_prefix, Error **errp) @@ -468,6 +505,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 */ @@ -494,15 +532,25 @@ 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)) { - return drv1; + + drv1 = bdrv_do_find_protocol(protocol); + if (drv1) { + return drv1; + } + + 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); + break; } } - error_setg(errp, "Unknown protocol '%s'", protocol); - return NULL; + drv1 = bdrv_do_find_protocol(protocol); + if (!drv1) { + error_setg(errp, "Unknown protocol '%s'", protocol); + } + return drv1; } /* diff --git a/block/Makefile.objs b/block/Makefile.objs index 2593a2f..595f366 100644 --- a/block/Makefile.objs +++ b/block/Makefile.objs @@ -1,4 +1,4 @@ -block-obj-y += raw_bsd.o qcow.o vdi.o vmdk.o cloop.o bochs.o vpc.o vvfat.o +block-obj-y += raw_bsd.o qcow.o vdi.o vmdk.o cloop.o bochs.o vpc.o vvfat.o dmg.o block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o qcow2-cache.o block-obj-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o block-obj-y += qed-check.o @@ -39,7 +39,6 @@ gluster.o-libs := $(GLUSTERFS_LIBS) ssh.o-cflags := $(LIBSSH2_CFLAGS) ssh.o-libs := $(LIBSSH2_LIBS) archipelago.o-libs := $(ARCHIPELAGO_LIBS) -block-obj-m += dmg.o dmg.o-libs := $(BZIP2_LIBS) qcow.o-libs := -lz linux-aio.o-libs := -laio diff --git a/include/qemu/module.h b/include/qemu/module.h index 2370708..dc2c9d4 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 86e3f7a..a5f7fbd 100644 --- a/util/module.c +++ b/util/module.c @@ -87,14 +87,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) { @@ -145,6 +142,7 @@ static int module_load_file(const char *fname) ret = -EINVAL; } else { QTAILQ_FOREACH(e, &dso_init_list, node) { + e->init(); register_module_init(e->init, e->type); } ret = 0; @@ -159,14 +157,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; @@ -177,15 +171,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 ? : ""); @@ -194,16 +179,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; } }