| Message ID | 1410891148-28849-5-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 16.09.2014 20:12, Markus Armbruster wrote: > Make the BlockBackend own the DriveInfo. Change blockdev_init() to > return the BlockBackend instead of the DriveInfo. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > block.c | 2 -- > block/block-backend.c | 38 ++++++++++++++++++++++++ > blockdev.c | 73 ++++++++++++++++++++++++----------------------- > include/sysemu/blockdev.h | 4 +++ > 4 files changed, 79 insertions(+), 38 deletions(-) [snip] > diff --git a/blockdev.c b/blockdev.c > index 5da6028..722d083 100644 > --- a/blockdev.c > +++ b/blockdev.c [snip] > @@ -920,19 +922,18 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type) > } > > /* Actual block device init: Functionality shared with blockdev-add */ > - dinfo = blockdev_init(filename, bs_opts, &local_err); > + blk = blockdev_init(filename, bs_opts, &local_err); > bs_opts = NULL; > - if (dinfo == NULL) { > - if (local_err) { > - error_report("%s", error_get_pretty(local_err)); > - error_free(local_err); > - } > + if (!blk) { > + error_report("%s", error_get_pretty(local_err)); > + error_free(local_err); > goto fail; Not all of blockdev_init() sets errp on failure. Try "qemu-system-x86_64 -drive format=help" and you'll see a segfault with this patch applied. So either you can make blockdev_init() do things right, which is, set errp for format=help instead of dumping the list to stdout (but I'm not even sure whether that's really correct, because it's not really an error), or you keep the "if" around error_report() and error_free() here. Looks good otherwise, though. Max
Max Reitz <mreitz@redhat.com> writes: > On 16.09.2014 20:12, Markus Armbruster wrote: >> Make the BlockBackend own the DriveInfo. Change blockdev_init() to >> return the BlockBackend instead of the DriveInfo. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> block.c | 2 -- >> block/block-backend.c | 38 ++++++++++++++++++++++++ >> blockdev.c | 73 ++++++++++++++++++++++++----------------------- >> include/sysemu/blockdev.h | 4 +++ >> 4 files changed, 79 insertions(+), 38 deletions(-) > > [snip] > >> diff --git a/blockdev.c b/blockdev.c >> index 5da6028..722d083 100644 >> --- a/blockdev.c >> +++ b/blockdev.c > > [snip] > >> @@ -920,19 +922,18 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type) >> } >> /* Actual block device init: Functionality shared with >> blockdev-add */ >> - dinfo = blockdev_init(filename, bs_opts, &local_err); >> + blk = blockdev_init(filename, bs_opts, &local_err); >> bs_opts = NULL; >> - if (dinfo == NULL) { >> - if (local_err) { >> - error_report("%s", error_get_pretty(local_err)); >> - error_free(local_err); >> - } >> + if (!blk) { >> + error_report("%s", error_get_pretty(local_err)); >> + error_free(local_err); >> goto fail; > > Not all of blockdev_init() sets errp on failure. Try > "qemu-system-x86_64 -drive format=help" and you'll see a segfault with > this patch applied. Good catch! The common contract is: 1. On success, return the new object and leave *errp alone. 2. On failure, return null and set *errp. blockdev_init() adds: 3. On help, return null and leave *errp alone. Fine, but it really needs a function comment. More so since the unusual case is buried in the middle of a 200+ line function. Separate patch, outside the scope of this series. > So either you can make blockdev_init() do things > right, which is, set errp for format=help instead of dumping the list > to stdout (but I'm not even sure whether that's really correct, > because it's not really an error), or you keep the "if" around > error_report() and error_free() here. Your doubts are justified: help is not an error. How to ask for help with -drive is obvious enough: -drive format=help Is the a way to ask for help with blockdev-add? Hard to see, as its arguments meander through QAPI-generated types, QDicts and QemuOpts. When I try, I either get "QMP input object member 'format' is unexpected", or crash in visitors; suspecting the bug fixed by "qapi: fix crash in dealloc visitor for union types". We'll need one Once we expose blockdev-add on the command line and in the human monitor. Since printing help to a QMP monitor isn't at all helpful, we'll have to either catch and reject the attempt to ask for it there, or print help with error_printf_unless_qmp(). I figure the cleanest solution would be to lift format help out of blockdev_init() into drive_new() now and later on the command line code. For this series, I'll simply refrain from breaking the existing logic. > Looks good otherwise, though. Thanks!
Am 16.09.2014 um 20:12 hat Markus Armbruster geschrieben: > Make the BlockBackend own the DriveInfo. Change blockdev_init() to > return the BlockBackend instead of the DriveInfo. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > block.c | 2 -- > block/block-backend.c | 38 ++++++++++++++++++++++++ > blockdev.c | 73 ++++++++++++++++++++++++----------------------- > include/sysemu/blockdev.h | 4 +++ > 4 files changed, 79 insertions(+), 38 deletions(-) > > diff --git a/block.c b/block.c > index 7ccf443..5f7dc45 100644 > --- a/block.c > +++ b/block.c > @@ -29,7 +29,6 @@ > #include "qemu/module.h" > #include "qapi/qmp/qjson.h" > #include "sysemu/sysemu.h" > -#include "sysemu/blockdev.h" /* FIXME layering violation */ > #include "qemu/notify.h" > #include "block/coroutine.h" > #include "block/qapi.h" > @@ -2124,7 +2123,6 @@ static void bdrv_delete(BlockDriverState *bs) > /* remove from list, if necessary */ > bdrv_make_anon(bs); > > - drive_info_del(drive_get_by_blockdev(bs)); > g_free(bs); > } > > diff --git a/block/block-backend.c b/block/block-backend.c > index a12215a..9ee57c7 100644 > --- a/block/block-backend.c > +++ b/block/block-backend.c > @@ -12,11 +12,13 @@ > > #include "sysemu/block-backend.h" > #include "block/block_int.h" > +#include "sysemu/blockdev.h" > > struct BlockBackend { > char *name; > int refcnt; > BlockDriverState *bs; > + DriveInfo *legacy_dinfo; > QTAILQ_ENTRY(BlockBackend) link; /* for blk_backends */ > }; > > @@ -87,6 +89,7 @@ static void blk_delete(BlockBackend *blk) > QTAILQ_REMOVE(&blk_backends, blk, link); > } > g_free(blk->name); > + drive_info_del(blk->legacy_dinfo); > g_free(blk); > } > > @@ -167,6 +170,41 @@ BlockDriverState *blk_bs(BlockBackend *blk) > } > > /* > + * Return @blk's DriveInfo if any, else null. > + */ > +DriveInfo *blk_legacy_dinfo(BlockBackend *blk) > +{ > + return blk->legacy_dinfo; > +} > + > +/* > + * Set @blk's DriveInfo to @dinfo, and return it. > + * @blk must not have a DriveInfo set already. > + * No other BlockBackend may have the same DriveInfo set. > + */ > +DriveInfo *blk_set_legacy_dinfo(BlockBackend *blk, DriveInfo *dinfo) > +{ > + assert(!blk->legacy_dinfo); > + return blk->legacy_dinfo = dinfo; Ugh, I don't like assignments in a return statement much more than setters that return something. Fortunately, nobody uses the return value, so this can become a void function. > +} > +/* > + * Return the BlockBackend with DriveInfo @dinfo. > + * It must exist. > + */ > +BlockBackend *blk_by_legacy_dinfo(DriveInfo *dinfo) > +{ > + BlockBackend *blk; > + > + QTAILQ_FOREACH(blk, &blk_backends, link) { > + if (blk->legacy_dinfo == dinfo) { > + return blk; > + } > + } > + assert(0); I'm surprised that the compiler doesn't complain here. Seems it understands that the condition is always false and the libc header sets the noreturn attribute for the internal function handling a failure. With NDEBUG set, this wouldn't work any more. I think abort() is better. > +} > + > +/* > * Hide @blk. > * @blk must not have been hidden already. > * Make attached BlockDriverState, if any, anonymous. > diff --git a/blockdev.c b/blockdev.c > index 5da6028..722d083 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -47,8 +47,6 @@ > #include "trace.h" > #include "sysemu/arch_init.h" > > -static QTAILQ_HEAD(drivelist, DriveInfo) drives = QTAILQ_HEAD_INITIALIZER(drives); > - > static const char *const if_name[IF_COUNT] = { > [IF_NONE] = "none", > [IF_IDE] = "ide", > @@ -89,7 +87,8 @@ static const int if_max_devs[IF_COUNT] = { > */ > void blockdev_mark_auto_del(BlockDriverState *bs) > { > - DriveInfo *dinfo = drive_get_by_blockdev(bs); > + BlockBackend *blk = bs->blk; > + DriveInfo *dinfo = blk_legacy_dinfo(blk); > > if (dinfo && !dinfo->enable_auto_del) { > return; > @@ -105,7 +104,8 @@ void blockdev_mark_auto_del(BlockDriverState *bs) > > void blockdev_auto_del(BlockDriverState *bs) > { > - DriveInfo *dinfo = drive_get_by_blockdev(bs); > + BlockBackend *blk = bs->blk; > + DriveInfo *dinfo = blk_legacy_dinfo(blk); > > if (dinfo && dinfo->auto_del) { > drive_del(dinfo); > @@ -153,15 +153,15 @@ QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file, > > DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit) > { > + BlockBackend *blk; > DriveInfo *dinfo; > > - /* seek interface, bus and unit */ > - > - QTAILQ_FOREACH(dinfo, &drives, next) { > - if (dinfo->type == type && > - dinfo->bus == bus && > - dinfo->unit == unit) > + for (blk = blk_next(NULL); blk; blk = blk_next(blk)) { > + dinfo = blk_legacy_dinfo(blk); > + if (dinfo && dinfo->type == type > + && dinfo->bus == bus && dinfo->unit == unit) { > return dinfo; > + } > } > > return NULL; > @@ -177,13 +177,15 @@ DriveInfo *drive_get_by_index(BlockInterfaceType type, int index) > int drive_get_max_bus(BlockInterfaceType type) > { > int max_bus; > + BlockBackend *blk; > DriveInfo *dinfo; > > max_bus = -1; > - QTAILQ_FOREACH(dinfo, &drives, next) { > - if(dinfo->type == type && > - dinfo->bus > max_bus) > + for (blk = blk_next(NULL); blk; blk = blk_next(blk)) { > + dinfo = blk_legacy_dinfo(blk); > + if (dinfo && dinfo->type == type && dinfo->bus > max_bus) { > max_bus = dinfo->bus; > + } > } > return max_bus; > } > @@ -200,11 +202,11 @@ DriveInfo *drive_get_next(BlockInterfaceType type) > > DriveInfo *drive_get_by_blockdev(BlockDriverState *bs) > { > - DriveInfo *dinfo; > + BlockBackend *blk; > > - QTAILQ_FOREACH(dinfo, &drives, next) { > - if (dinfo->bdrv == bs) { > - return dinfo; > + for (blk = blk_next(NULL); blk; blk = blk_next(blk)) { > + if (blk_bs(blk) == bs) { > + return blk_legacy_dinfo(blk); > } > } > return NULL; Why don't you use bs->blk here? Also, why replace calls to drive_get_by_blockdev() above when the function stays? Wouldn't it make more sense to remove all callers in a single patch instead of doing part of it in an unrelated patch? > @@ -218,6 +220,7 @@ static void bdrv_format_print(void *opaque, const char *name) > void drive_del(DriveInfo *dinfo) > { > bdrv_unref(dinfo->bdrv); > + blk_unref(blk_by_legacy_dinfo(dinfo)); > } Hmm, let's see. drive_del() has four callers: * do_drive_del(): A blk_unref() is removed there, so the code remains equivalent. Good. * blockdev_auto_del(): Here, the blk_unref() is new. If I'm reading the code right, unplugging a device doesn't only automagically destroy the DriveInfo now, but also the BlockBackend. Is this really magic that we want BlockBackend to inherit? (I don't think so.) * drive_hot_add(): This is an error paths that frees a previously leaked BlockBackend created by an indirect drive_new() call. Silent bug fix? * pci_piix3_xen_ide_unplug(): Xen magic I don't fully understand. The guest can tell us to remove the IDE drives (because it sees the same image through the PV driver, I think). Whether or not we want to remove the BlockBackend, I'm not completely sure. Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 16.09.2014 um 20:12 hat Markus Armbruster geschrieben: >> Make the BlockBackend own the DriveInfo. Change blockdev_init() to >> return the BlockBackend instead of the DriveInfo. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> block.c | 2 -- >> block/block-backend.c | 38 ++++++++++++++++++++++++ >> blockdev.c | 73 ++++++++++++++++++++++++----------------------- >> include/sysemu/blockdev.h | 4 +++ >> 4 files changed, 79 insertions(+), 38 deletions(-) >> >> diff --git a/block.c b/block.c >> index 7ccf443..5f7dc45 100644 >> --- a/block.c >> +++ b/block.c >> @@ -29,7 +29,6 @@ >> #include "qemu/module.h" >> #include "qapi/qmp/qjson.h" >> #include "sysemu/sysemu.h" >> -#include "sysemu/blockdev.h" /* FIXME layering violation */ >> #include "qemu/notify.h" >> #include "block/coroutine.h" >> #include "block/qapi.h" >> @@ -2124,7 +2123,6 @@ static void bdrv_delete(BlockDriverState *bs) >> /* remove from list, if necessary */ >> bdrv_make_anon(bs); >> >> - drive_info_del(drive_get_by_blockdev(bs)); >> g_free(bs); >> } >> >> diff --git a/block/block-backend.c b/block/block-backend.c >> index a12215a..9ee57c7 100644 >> --- a/block/block-backend.c >> +++ b/block/block-backend.c >> @@ -12,11 +12,13 @@ >> >> #include "sysemu/block-backend.h" >> #include "block/block_int.h" >> +#include "sysemu/blockdev.h" >> >> struct BlockBackend { >> char *name; >> int refcnt; >> BlockDriverState *bs; >> + DriveInfo *legacy_dinfo; >> QTAILQ_ENTRY(BlockBackend) link; /* for blk_backends */ >> }; >> >> @@ -87,6 +89,7 @@ static void blk_delete(BlockBackend *blk) >> QTAILQ_REMOVE(&blk_backends, blk, link); >> } >> g_free(blk->name); >> + drive_info_del(blk->legacy_dinfo); >> g_free(blk); >> } >> >> @@ -167,6 +170,41 @@ BlockDriverState *blk_bs(BlockBackend *blk) >> } >> >> /* >> + * Return @blk's DriveInfo if any, else null. >> + */ >> +DriveInfo *blk_legacy_dinfo(BlockBackend *blk) >> +{ >> + return blk->legacy_dinfo; >> +} >> + >> +/* >> + * Set @blk's DriveInfo to @dinfo, and return it. >> + * @blk must not have a DriveInfo set already. >> + * No other BlockBackend may have the same DriveInfo set. >> + */ >> +DriveInfo *blk_set_legacy_dinfo(BlockBackend *blk, DriveInfo *dinfo) >> +{ >> + assert(!blk->legacy_dinfo); >> + return blk->legacy_dinfo = dinfo; > > Ugh, I don't like assignments in a return statement much more than > setters that return something. It's an assignment. In C, assignments return a value. > Fortunately, nobody uses the return value, so this can become a void > function. > >> +} >> +/* >> + * Return the BlockBackend with DriveInfo @dinfo. >> + * It must exist. >> + */ >> +BlockBackend *blk_by_legacy_dinfo(DriveInfo *dinfo) >> +{ >> + BlockBackend *blk; >> + >> + QTAILQ_FOREACH(blk, &blk_backends, link) { >> + if (blk->legacy_dinfo == dinfo) { >> + return blk; >> + } >> + } >> + assert(0); > > I'm surprised that the compiler doesn't complain here. Seems it > understands that the condition is always false and the libc header sets > the noreturn attribute for the internal function handling a failure. > With NDEBUG set, this wouldn't work any more. > > I think abort() is better. Okay. >> +} >> + >> +/* >> * Hide @blk. >> * @blk must not have been hidden already. >> * Make attached BlockDriverState, if any, anonymous. >> diff --git a/blockdev.c b/blockdev.c >> index 5da6028..722d083 100644 >> --- a/blockdev.c >> +++ b/blockdev.c >> @@ -47,8 +47,6 @@ >> #include "trace.h" >> #include "sysemu/arch_init.h" >> >> -static QTAILQ_HEAD(drivelist, DriveInfo) drives = QTAILQ_HEAD_INITIALIZER(drives); >> - >> static const char *const if_name[IF_COUNT] = { >> [IF_NONE] = "none", >> [IF_IDE] = "ide", >> @@ -89,7 +87,8 @@ static const int if_max_devs[IF_COUNT] = { >> */ >> void blockdev_mark_auto_del(BlockDriverState *bs) >> { >> - DriveInfo *dinfo = drive_get_by_blockdev(bs); >> + BlockBackend *blk = bs->blk; >> + DriveInfo *dinfo = blk_legacy_dinfo(blk); >> >> if (dinfo && !dinfo->enable_auto_del) { >> return; >> @@ -105,7 +104,8 @@ void blockdev_mark_auto_del(BlockDriverState *bs) >> >> void blockdev_auto_del(BlockDriverState *bs) >> { >> - DriveInfo *dinfo = drive_get_by_blockdev(bs); >> + BlockBackend *blk = bs->blk; >> + DriveInfo *dinfo = blk_legacy_dinfo(blk); >> >> if (dinfo && dinfo->auto_del) { >> drive_del(dinfo); >> @@ -153,15 +153,15 @@ QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file, >> >> DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit) >> { >> + BlockBackend *blk; >> DriveInfo *dinfo; >> >> - /* seek interface, bus and unit */ >> - >> - QTAILQ_FOREACH(dinfo, &drives, next) { >> - if (dinfo->type == type && >> - dinfo->bus == bus && >> - dinfo->unit == unit) >> + for (blk = blk_next(NULL); blk; blk = blk_next(blk)) { >> + dinfo = blk_legacy_dinfo(blk); >> + if (dinfo && dinfo->type == type >> + && dinfo->bus == bus && dinfo->unit == unit) { >> return dinfo; >> + } >> } >> >> return NULL; >> @@ -177,13 +177,15 @@ DriveInfo *drive_get_by_index(BlockInterfaceType type, int index) >> int drive_get_max_bus(BlockInterfaceType type) >> { >> int max_bus; >> + BlockBackend *blk; >> DriveInfo *dinfo; >> >> max_bus = -1; >> - QTAILQ_FOREACH(dinfo, &drives, next) { >> - if(dinfo->type == type && >> - dinfo->bus > max_bus) >> + for (blk = blk_next(NULL); blk; blk = blk_next(blk)) { >> + dinfo = blk_legacy_dinfo(blk); >> + if (dinfo && dinfo->type == type && dinfo->bus > max_bus) { >> max_bus = dinfo->bus; >> + } >> } >> return max_bus; >> } >> @@ -200,11 +202,11 @@ DriveInfo *drive_get_next(BlockInterfaceType type) >> >> DriveInfo *drive_get_by_blockdev(BlockDriverState *bs) >> { >> - DriveInfo *dinfo; >> + BlockBackend *blk; >> >> - QTAILQ_FOREACH(dinfo, &drives, next) { >> - if (dinfo->bdrv == bs) { >> - return dinfo; >> + for (blk = blk_next(NULL); blk; blk = blk_next(blk)) { >> + if (blk_bs(blk) == bs) { >> + return blk_legacy_dinfo(blk); >> } >> } >> return NULL; > > Why don't you use bs->blk here? Beats me; I guess I let my fingers pick it. I'll switch to bs->blk. > Also, why replace calls to drive_get_by_blockdev() above when the > function stays? Wouldn't it make more sense to remove all callers in a > single patch instead of doing part of it in an unrelated patch? The general idea is to convert users of drive_get_by_blockdev() to blk_legacy_dinfo() as soon as they use BlockBackend. PATCH 04 (this one) converts do_drive_del(), because it's already using BlockBackend. It also converts blockdev_mark_auto_del() and blockdev_auto_del() to BlockBackend. I admit that's not yet necessary at this point of the series. I did it because I don't like having both old drive_get_by_blockdev() and new blk_legacy_dinfo() used in the same file. Even when the file is still providing drive_get_by_blockdev() to other users, which get converted to BB only in PATCH 14. PATCH 14 "hw: Convert from BlockDriverState to BlockBackend, mostly" converts all the other callers. >> @@ -218,6 +220,7 @@ static void bdrv_format_print(void *opaque, const char *name) >> void drive_del(DriveInfo *dinfo) >> { >> bdrv_unref(dinfo->bdrv); >> + blk_unref(blk_by_legacy_dinfo(dinfo)); >> } > > Hmm, let's see. drive_del() has four callers: > > * do_drive_del(): A blk_unref() is removed there, so the code remains > equivalent. Good. > > * blockdev_auto_del(): Here, the blk_unref() is new. If I'm reading the > code right, unplugging a device doesn't only automagically destroy the > DriveInfo now, but also the BlockBackend. Is this really magic that we > want BlockBackend to inherit? (I don't think so.) Let me explain this stuff, not least because it helps me think. "Unplug of device model destroys any block backends it's using" is a misfeature we want to confine to backends created "the old way". Where "the old way" really means whatever we want to remain misfeature- compatible. Definitely not blockdev-add. My working hypothesis for "old way" is "anything that goes through drive_new()". Both old and new way create a tree of BDS. Before this series, they both create a DriveInfo. Afterwards, they both create a BlockBackend, but only -drive & friends creates a DriveInfo. blockdev_auto_del() is supposed to *nothing* for a "new way" argument. Its condition guarding drive_del() must ensure that. On device unplug, blockdev_auto_del() runs for all its backends. For an "old way" backend, it destroys. This patch makes it destroy the BlockBackend, too. Which means PATCH 02 leaked it. Meh. I can try to avoid the temporary leak in v4. For a "new way" backend, it does nothing. No change with this patch. > * drive_hot_add(): This is an error paths that frees a previously leaked > BlockBackend created by an indirect drive_new() call. Silent bug fix? Similar temporary leak. Can try to avoid in v4. > * pci_piix3_xen_ide_unplug(): Xen magic I don't fully understand. The > guest can tell us to remove the IDE drives (because it sees the same > image through the PV driver, I think). Whether or not we want to > remove the BlockBackend, I'm not completely sure. The general idea is to delete a BB exactly when the corresponding root BDS gets deleted. PATCH 02 doesn't get that completely right, but this patch should heal it. Getting it completely right in PATCH 02 is impractical because HMP command drive_del is so nasty. Getting it less wrong would be nice, and I'm going to try. Thanks!
diff --git a/block.c b/block.c index 7ccf443..5f7dc45 100644 --- a/block.c +++ b/block.c @@ -29,7 +29,6 @@ #include "qemu/module.h" #include "qapi/qmp/qjson.h" #include "sysemu/sysemu.h" -#include "sysemu/blockdev.h" /* FIXME layering violation */ #include "qemu/notify.h" #include "block/coroutine.h" #include "block/qapi.h" @@ -2124,7 +2123,6 @@ static void bdrv_delete(BlockDriverState *bs) /* remove from list, if necessary */ bdrv_make_anon(bs); - drive_info_del(drive_get_by_blockdev(bs)); g_free(bs); } diff --git a/block/block-backend.c b/block/block-backend.c index a12215a..9ee57c7 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -12,11 +12,13 @@ #include "sysemu/block-backend.h" #include "block/block_int.h" +#include "sysemu/blockdev.h" struct BlockBackend { char *name; int refcnt; BlockDriverState *bs; + DriveInfo *legacy_dinfo; QTAILQ_ENTRY(BlockBackend) link; /* for blk_backends */ }; @@ -87,6 +89,7 @@ static void blk_delete(BlockBackend *blk) QTAILQ_REMOVE(&blk_backends, blk, link); } g_free(blk->name); + drive_info_del(blk->legacy_dinfo); g_free(blk); } @@ -167,6 +170,41 @@ BlockDriverState *blk_bs(BlockBackend *blk) } /* + * Return @blk's DriveInfo if any, else null. + */ +DriveInfo *blk_legacy_dinfo(BlockBackend *blk) +{ + return blk->legacy_dinfo; +} + +/* + * Set @blk's DriveInfo to @dinfo, and return it. + * @blk must not have a DriveInfo set already. + * No other BlockBackend may have the same DriveInfo set. + */ +DriveInfo *blk_set_legacy_dinfo(BlockBackend *blk, DriveInfo *dinfo) +{ + assert(!blk->legacy_dinfo); + return blk->legacy_dinfo = dinfo; +} + +/* + * Return the BlockBackend with DriveInfo @dinfo. + * It must exist. + */ +BlockBackend *blk_by_legacy_dinfo(DriveInfo *dinfo) +{ + BlockBackend *blk; + + QTAILQ_FOREACH(blk, &blk_backends, link) { + if (blk->legacy_dinfo == dinfo) { + return blk; + } + } + assert(0); +} + +/* * Hide @blk. * @blk must not have been hidden already. * Make attached BlockDriverState, if any, anonymous. diff --git a/blockdev.c b/blockdev.c index 5da6028..722d083 100644 --- a/blockdev.c +++ b/blockdev.c @@ -47,8 +47,6 @@ #include "trace.h" #include "sysemu/arch_init.h" -static QTAILQ_HEAD(drivelist, DriveInfo) drives = QTAILQ_HEAD_INITIALIZER(drives); - static const char *const if_name[IF_COUNT] = { [IF_NONE] = "none", [IF_IDE] = "ide", @@ -89,7 +87,8 @@ static const int if_max_devs[IF_COUNT] = { */ void blockdev_mark_auto_del(BlockDriverState *bs) { - DriveInfo *dinfo = drive_get_by_blockdev(bs); + BlockBackend *blk = bs->blk; + DriveInfo *dinfo = blk_legacy_dinfo(blk); if (dinfo && !dinfo->enable_auto_del) { return; @@ -105,7 +104,8 @@ void blockdev_mark_auto_del(BlockDriverState *bs) void blockdev_auto_del(BlockDriverState *bs) { - DriveInfo *dinfo = drive_get_by_blockdev(bs); + BlockBackend *blk = bs->blk; + DriveInfo *dinfo = blk_legacy_dinfo(blk); if (dinfo && dinfo->auto_del) { drive_del(dinfo); @@ -153,15 +153,15 @@ QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file, DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit) { + BlockBackend *blk; DriveInfo *dinfo; - /* seek interface, bus and unit */ - - QTAILQ_FOREACH(dinfo, &drives, next) { - if (dinfo->type == type && - dinfo->bus == bus && - dinfo->unit == unit) + for (blk = blk_next(NULL); blk; blk = blk_next(blk)) { + dinfo = blk_legacy_dinfo(blk); + if (dinfo && dinfo->type == type + && dinfo->bus == bus && dinfo->unit == unit) { return dinfo; + } } return NULL; @@ -177,13 +177,15 @@ DriveInfo *drive_get_by_index(BlockInterfaceType type, int index) int drive_get_max_bus(BlockInterfaceType type) { int max_bus; + BlockBackend *blk; DriveInfo *dinfo; max_bus = -1; - QTAILQ_FOREACH(dinfo, &drives, next) { - if(dinfo->type == type && - dinfo->bus > max_bus) + for (blk = blk_next(NULL); blk; blk = blk_next(blk)) { + dinfo = blk_legacy_dinfo(blk); + if (dinfo && dinfo->type == type && dinfo->bus > max_bus) { max_bus = dinfo->bus; + } } return max_bus; } @@ -200,11 +202,11 @@ DriveInfo *drive_get_next(BlockInterfaceType type) DriveInfo *drive_get_by_blockdev(BlockDriverState *bs) { - DriveInfo *dinfo; + BlockBackend *blk; - QTAILQ_FOREACH(dinfo, &drives, next) { - if (dinfo->bdrv == bs) { - return dinfo; + for (blk = blk_next(NULL); blk; blk = blk_next(blk)) { + if (blk_bs(blk) == bs) { + return blk_legacy_dinfo(blk); } } return NULL; @@ -218,6 +220,7 @@ static void bdrv_format_print(void *opaque, const char *name) void drive_del(DriveInfo *dinfo) { bdrv_unref(dinfo->bdrv); + blk_unref(blk_by_legacy_dinfo(dinfo)); } void drive_info_del(DriveInfo *dinfo) @@ -228,9 +231,7 @@ void drive_info_del(DriveInfo *dinfo) if (dinfo->opts) { qemu_opts_del(dinfo->opts); } - g_free(dinfo->id); - QTAILQ_REMOVE(&drives, dinfo, next); g_free(dinfo->serial); g_free(dinfo); } @@ -302,8 +303,8 @@ static bool check_throttle_config(ThrottleConfig *cfg, Error **errp) typedef enum { MEDIA_DISK, MEDIA_CDROM } DriveMediaType; /* Takes the ownership of bs_opts */ -static DriveInfo *blockdev_init(const char *file, QDict *bs_opts, - Error **errp) +static BlockBackend *blockdev_init(const char *file, QDict *bs_opts, + Error **errp) { const char *buf; int ro = 0; @@ -486,7 +487,7 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts, dinfo = g_malloc0(sizeof(*dinfo)); dinfo->id = g_strdup(qemu_opts_id(opts)); dinfo->bdrv = bs; - QTAILQ_INSERT_TAIL(&drives, dinfo, next); + blk_set_legacy_dinfo(blk, dinfo); if (!file || !*file) { if (has_driver_specific_opts) { @@ -494,7 +495,7 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts, } else { QDECREF(bs_opts); qemu_opts_del(opts); - return dinfo; + return blk; } } if (snapshot) { @@ -531,7 +532,7 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts, QDECREF(bs_opts); qemu_opts_del(opts); - return dinfo; + return blk; err: bdrv_unref(bs); @@ -638,6 +639,7 @@ QemuOptsList qemu_legacy_drive_opts = { DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type) { const char *value; + BlockBackend *blk; DriveInfo *dinfo = NULL; QDict *bs_opts; QemuOpts *legacy_opts; @@ -920,19 +922,18 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type) } /* Actual block device init: Functionality shared with blockdev-add */ - dinfo = blockdev_init(filename, bs_opts, &local_err); + blk = blockdev_init(filename, bs_opts, &local_err); bs_opts = NULL; - if (dinfo == NULL) { - if (local_err) { - error_report("%s", error_get_pretty(local_err)); - error_free(local_err); - } + if (!blk) { + error_report("%s", error_get_pretty(local_err)); + error_free(local_err); goto fail; } else { assert(!local_err); } /* Set legacy DriveInfo fields */ + dinfo = blk_legacy_dinfo(blk); dinfo->enable_auto_del = true; dinfo->opts = all_opts; @@ -1762,7 +1763,7 @@ int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data) } bs = blk_bs(blk); - dinfo = drive_get_by_blockdev(bs); + dinfo = blk_legacy_dinfo(blk); if (dinfo && !dinfo->enable_auto_del) { error_report("Deleting device added with blockdev-add" " is not supported"); @@ -1797,7 +1798,6 @@ int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data) /* FIXME bs->blk leaked when bs dies */ } else { drive_del(dinfo); - blk_unref(blk); } aio_context_release(aio_context); @@ -2489,7 +2489,7 @@ void qmp_change_backing_file(const char *device, void qmp_blockdev_add(BlockdevOptions *options, Error **errp) { QmpOutputVisitor *ov = qmp_output_visitor_new(); - DriveInfo *dinfo; + BlockBackend *blk; QObject *obj; QDict *qdict; Error *local_err = NULL; @@ -2527,14 +2527,15 @@ void qmp_blockdev_add(BlockdevOptions *options, Error **errp) qdict_flatten(qdict); - dinfo = blockdev_init(NULL, qdict, &local_err); + blk = blockdev_init(NULL, qdict, &local_err); if (local_err) { error_propagate(errp, local_err); goto fail; } - if (bdrv_key_required(dinfo->bdrv)) { - drive_del(dinfo); + if (bdrv_key_required(blk_bs(blk))) { + bdrv_unref(blk_bs(blk)); + blk_unref(blk); error_setg(errp, "blockdev-add doesn't support encrypted devices"); goto fail; } diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h index abec381..1dc5906 100644 --- a/include/sysemu/blockdev.h +++ b/include/sysemu/blockdev.h @@ -45,6 +45,10 @@ struct DriveInfo { QTAILQ_ENTRY(DriveInfo) next; }; +DriveInfo *blk_legacy_dinfo(BlockBackend *blk); +DriveInfo *blk_set_legacy_dinfo(BlockBackend *blk, DriveInfo *dinfo); +BlockBackend *blk_by_legacy_dinfo(DriveInfo *dinfo); + DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit); DriveInfo *drive_get_by_index(BlockInterfaceType type, int index); int drive_get_max_bus(BlockInterfaceType type);
Make the BlockBackend own the DriveInfo. Change blockdev_init() to return the BlockBackend instead of the DriveInfo. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- block.c | 2 -- block/block-backend.c | 38 ++++++++++++++++++++++++ blockdev.c | 73 ++++++++++++++++++++++++----------------------- include/sysemu/blockdev.h | 4 +++ 4 files changed, 79 insertions(+), 38 deletions(-)