| Message ID | 1423501897-30410-2-git-send-email-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 02/09/2015 10:11 AM, Max Reitz wrote: > If the "id" field is missing from the options given to blockdev-add, > just omit the BlockBackend and create the BlockDriverState tree alone. > > However, if "id" is missing, "node-name" must be specified; otherwise, > the BDS tree would no longer be accessible. > Well, if we ever revived Jeff Cody's attempt at auto-assigning node names (so that we never have an unnamed node), then this patch will have to be partially reverted at that time (omitting id and node-name then results in a BDS with an auto-assigned node name and no BB). But that's a decision for that series (if we ever revive it); for now, your policy is just fine. > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > blockdev.c | 44 +++++++++++++++++++++++++++++++------------- > qapi/block-core.json | 13 +++++++++---- > tests/qemu-iotests/087 | 2 +- > tests/qemu-iotests/087.out | 4 ++-- > 4 files changed, 43 insertions(+), 20 deletions(-) Reviewed-by: Eric Blake <eblake@redhat.com> > +++ b/qapi/block-core.json > @@ -1260,9 +1260,12 @@ > # > # @driver: block driver name > # @id: #optional id by which the new block device can be referred to. > -# This is a required option on the top level of blockdev-add, and > -# currently not allowed on any other level. > -# @node-name: #optional the name of a block driver state node (Since 2.0) > +# This option is only allowed on the top level of blockdev-add. > +# A BlockBackend will be created by blockdev-add if and only if > +# this option is given. I know what you mean here, but it feels a tiny bit like we are leaking implementation details. Would it be any better to state that: "A guest-visible device will be created by blockdev-add if and only if this option is given"? That is, instead of BlockDriverState and BlockBackend (which are internal naming conventions), should our documentation be favoring "node within a tree of host-accessible resources that provide the media content to a guest device" and "guest-visible device"? But just in typing that out, it gets tedious, and even if we do make such a change in documentation, it would be better to do it over all existing .json files rather than just this patch. Furthermore, we may use BlockBackend for things like NBD fleecing operations, which really aren't guest-visible devices. So my idle ramblings here don't affect my R-b for the patch as-is.
On 2015-02-09 at 13:17, Eric Blake wrote: > On 02/09/2015 10:11 AM, Max Reitz wrote: >> If the "id" field is missing from the options given to blockdev-add, >> just omit the BlockBackend and create the BlockDriverState tree alone. >> >> However, if "id" is missing, "node-name" must be specified; otherwise, >> the BDS tree would no longer be accessible. >> > Well, if we ever revived Jeff Cody's attempt at auto-assigning node > names (so that we never have an unnamed node), then this patch will have > to be partially reverted at that time (omitting id and node-name then > results in a BDS with an auto-assigned node name and no BB). But that's > a decision for that series (if we ever revive it); for now, your policy > is just fine. > >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> blockdev.c | 44 +++++++++++++++++++++++++++++++------------- >> qapi/block-core.json | 13 +++++++++---- >> tests/qemu-iotests/087 | 2 +- >> tests/qemu-iotests/087.out | 4 ++-- >> 4 files changed, 43 insertions(+), 20 deletions(-) > Reviewed-by: Eric Blake <eblake@redhat.com> > > >> +++ b/qapi/block-core.json >> @@ -1260,9 +1260,12 @@ >> # >> # @driver: block driver name >> # @id: #optional id by which the new block device can be referred to. >> -# This is a required option on the top level of blockdev-add, and >> -# currently not allowed on any other level. >> -# @node-name: #optional the name of a block driver state node (Since 2.0) >> +# This option is only allowed on the top level of blockdev-add. >> +# A BlockBackend will be created by blockdev-add if and only if >> +# this option is given. > I know what you mean here, but it feels a tiny bit like we are leaking > implementation details. Would it be any better to state that: "A > guest-visible device will be created by blockdev-add if and only if this > option is given"? Well, it's not a guest-visible device; a BlockBackend is the connector between a BDS tree and a guest-visible device. However, I thought about the same thing, but there are already occurrences of "block driver state", so I decided to just go with it (I mean, we could use "block backend", but I don't know if that's really better). Max > That is, instead of BlockDriverState and BlockBackend > (which are internal naming conventions), should our documentation be > favoring "node within a tree of host-accessible resources that provide > the media content to a guest device" and "guest-visible device"? But > just in typing that out, it gets tedious, and even if we do make such a > change in documentation, it would be better to do it over all existing > .json files rather than just this patch. Furthermore, we may use > BlockBackend for things like NBD fleecing operations, which really > aren't guest-visible devices. So my idle ramblings here don't affect my > R-b for the patch as-is.
Am 09.02.2015 um 18:11 hat Max Reitz geschrieben: > If the "id" field is missing from the options given to blockdev-add, > just omit the BlockBackend and create the BlockDriverState tree alone. > > However, if "id" is missing, "node-name" must be specified; otherwise, > the BDS tree would no longer be accessible. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > blockdev.c | 44 +++++++++++++++++++++++++++++++------------- > qapi/block-core.json | 13 +++++++++---- > tests/qemu-iotests/087 | 2 +- > tests/qemu-iotests/087.out | 4 ++-- > 4 files changed, 43 insertions(+), 20 deletions(-) > > diff --git a/blockdev.c b/blockdev.c > index 6eedcf5..6d67c80 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -2822,17 +2822,12 @@ out: > void qmp_blockdev_add(BlockdevOptions *options, Error **errp) > { > QmpOutputVisitor *ov = qmp_output_visitor_new(); > - BlockBackend *blk; > + BlockDriverState *bs; > + BlockBackend *blk = NULL; > QObject *obj; > QDict *qdict; > Error *local_err = NULL; > > - /* Require an ID in the top level */ > - if (!options->has_id) { > - error_setg(errp, "Block device needs an ID"); > - goto fail; > - } > - > /* TODO Sort it out in raw-posix and drive_new(): Reject aio=native with > * cache.direct=false instead of silently switching to aio=threads, except > * when called from drive_new(). > @@ -2860,14 +2855,37 @@ void qmp_blockdev_add(BlockdevOptions *options, Error **errp) > > qdict_flatten(qdict); > > - blk = blockdev_init(NULL, qdict, &local_err); > - if (local_err) { > - error_propagate(errp, local_err); > - goto fail; > + if (options->has_id) { > + blk = blockdev_init(NULL, qdict, &local_err); > + if (local_err) { > + error_propagate(errp, local_err); > + goto fail; > + } > + > + bs = blk_bs(blk); > + } else { > + int ret; > + > + if (!qdict_get_try_str(qdict, "node-name")) { > + error_setg(errp, "'id' and/or 'node-name' need to be specified for " > + "the root node"); > + goto fail; > + } > + > + bs = NULL; > + ret = bdrv_open(&bs, NULL, NULL, qdict, BDRV_O_RDWR | BDRV_O_CACHE_WB, > + NULL, errp); Now all the qdict entries that aren't parsed by bdrv_open() but converted into flags by blockdev_init() are broken if you don't give an id. This includes read-only, discard, the cache options - in other words, enough to make this "support" completely useless. I guess I need to dig out my cache mode series and get it ready to be merged. Once this is in, the parsing of the other necessary options should be easy to move into bdrv_open(). Unless you have a good reason why we should merge this patch in its hardly functional state now, I would consider those conversions a dependency of this one. Kevin
On 2015-03-04 at 08:39, Kevin Wolf wrote: > Am 09.02.2015 um 18:11 hat Max Reitz geschrieben: >> If the "id" field is missing from the options given to blockdev-add, >> just omit the BlockBackend and create the BlockDriverState tree alone. >> >> However, if "id" is missing, "node-name" must be specified; otherwise, >> the BDS tree would no longer be accessible. >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> blockdev.c | 44 +++++++++++++++++++++++++++++++------------- >> qapi/block-core.json | 13 +++++++++---- >> tests/qemu-iotests/087 | 2 +- >> tests/qemu-iotests/087.out | 4 ++-- >> 4 files changed, 43 insertions(+), 20 deletions(-) >> >> diff --git a/blockdev.c b/blockdev.c >> index 6eedcf5..6d67c80 100644 >> --- a/blockdev.c >> +++ b/blockdev.c >> @@ -2822,17 +2822,12 @@ out: >> void qmp_blockdev_add(BlockdevOptions *options, Error **errp) >> { >> QmpOutputVisitor *ov = qmp_output_visitor_new(); >> - BlockBackend *blk; >> + BlockDriverState *bs; >> + BlockBackend *blk = NULL; >> QObject *obj; >> QDict *qdict; >> Error *local_err = NULL; >> >> - /* Require an ID in the top level */ >> - if (!options->has_id) { >> - error_setg(errp, "Block device needs an ID"); >> - goto fail; >> - } >> - >> /* TODO Sort it out in raw-posix and drive_new(): Reject aio=native with >> * cache.direct=false instead of silently switching to aio=threads, except >> * when called from drive_new(). >> @@ -2860,14 +2855,37 @@ void qmp_blockdev_add(BlockdevOptions *options, Error **errp) >> >> qdict_flatten(qdict); >> >> - blk = blockdev_init(NULL, qdict, &local_err); >> - if (local_err) { >> - error_propagate(errp, local_err); >> - goto fail; >> + if (options->has_id) { >> + blk = blockdev_init(NULL, qdict, &local_err); >> + if (local_err) { >> + error_propagate(errp, local_err); >> + goto fail; >> + } >> + >> + bs = blk_bs(blk); >> + } else { >> + int ret; >> + >> + if (!qdict_get_try_str(qdict, "node-name")) { >> + error_setg(errp, "'id' and/or 'node-name' need to be specified for " >> + "the root node"); >> + goto fail; >> + } >> + >> + bs = NULL; >> + ret = bdrv_open(&bs, NULL, NULL, qdict, BDRV_O_RDWR | BDRV_O_CACHE_WB, >> + NULL, errp); > Now all the qdict entries that aren't parsed by bdrv_open() but > converted into flags by blockdev_init() are broken if you don't give an > id. This includes read-only, discard, the cache options - in other > words, enough to make this "support" completely useless. See patch 23. Max > > I guess I need to dig out my cache mode series and get it ready to be > merged. Once this is in, the parsing of the other necessary options > should be easy to move into bdrv_open(). > > Unless you have a good reason why we should merge this patch in its > hardly functional state now, I would consider those conversions a > dependency of this one. > > Kevin
Am 04.03.2015 um 15:04 hat Max Reitz geschrieben: > On 2015-03-04 at 08:39, Kevin Wolf wrote: > >Am 09.02.2015 um 18:11 hat Max Reitz geschrieben: > >>If the "id" field is missing from the options given to blockdev-add, > >>just omit the BlockBackend and create the BlockDriverState tree alone. > >> > >>However, if "id" is missing, "node-name" must be specified; otherwise, > >>the BDS tree would no longer be accessible. > >> > >>Signed-off-by: Max Reitz <mreitz@redhat.com> > >>--- > >> blockdev.c | 44 +++++++++++++++++++++++++++++++------------- > >> qapi/block-core.json | 13 +++++++++---- > >> tests/qemu-iotests/087 | 2 +- > >> tests/qemu-iotests/087.out | 4 ++-- > >> 4 files changed, 43 insertions(+), 20 deletions(-) > >> > >>diff --git a/blockdev.c b/blockdev.c > >>index 6eedcf5..6d67c80 100644 > >>--- a/blockdev.c > >>+++ b/blockdev.c > >>@@ -2822,17 +2822,12 @@ out: > >> void qmp_blockdev_add(BlockdevOptions *options, Error **errp) > >> { > >> QmpOutputVisitor *ov = qmp_output_visitor_new(); > >>- BlockBackend *blk; > >>+ BlockDriverState *bs; > >>+ BlockBackend *blk = NULL; > >> QObject *obj; > >> QDict *qdict; > >> Error *local_err = NULL; > >>- /* Require an ID in the top level */ > >>- if (!options->has_id) { > >>- error_setg(errp, "Block device needs an ID"); > >>- goto fail; > >>- } > >>- > >> /* TODO Sort it out in raw-posix and drive_new(): Reject aio=native with > >> * cache.direct=false instead of silently switching to aio=threads, except > >> * when called from drive_new(). > >>@@ -2860,14 +2855,37 @@ void qmp_blockdev_add(BlockdevOptions *options, Error **errp) > >> qdict_flatten(qdict); > >>- blk = blockdev_init(NULL, qdict, &local_err); > >>- if (local_err) { > >>- error_propagate(errp, local_err); > >>- goto fail; > >>+ if (options->has_id) { > >>+ blk = blockdev_init(NULL, qdict, &local_err); > >>+ if (local_err) { > >>+ error_propagate(errp, local_err); > >>+ goto fail; > >>+ } > >>+ > >>+ bs = blk_bs(blk); > >>+ } else { > >>+ int ret; > >>+ > >>+ if (!qdict_get_try_str(qdict, "node-name")) { > >>+ error_setg(errp, "'id' and/or 'node-name' need to be specified for " > >>+ "the root node"); > >>+ goto fail; > >>+ } > >>+ > >>+ bs = NULL; > >>+ ret = bdrv_open(&bs, NULL, NULL, qdict, BDRV_O_RDWR | BDRV_O_CACHE_WB, > >>+ NULL, errp); > >Now all the qdict entries that aren't parsed by bdrv_open() but > >converted into flags by blockdev_init() are broken if you don't give an > >id. This includes read-only, discard, the cache options - in other > >words, enough to make this "support" completely useless. > > See patch 23. No matter what I'll find there (okay, I've cheated and already quickly looked at it before writing this), this answer tells me that you're doing things in the wrong order. Kevin
On 2015-03-04 at 09:15, Kevin Wolf wrote: > Am 04.03.2015 um 15:04 hat Max Reitz geschrieben: >> On 2015-03-04 at 08:39, Kevin Wolf wrote: >>> Am 09.02.2015 um 18:11 hat Max Reitz geschrieben: >>>> If the "id" field is missing from the options given to blockdev-add, >>>> just omit the BlockBackend and create the BlockDriverState tree alone. >>>> >>>> However, if "id" is missing, "node-name" must be specified; otherwise, >>>> the BDS tree would no longer be accessible. >>>> >>>> Signed-off-by: Max Reitz <mreitz@redhat.com> >>>> --- >>>> blockdev.c | 44 +++++++++++++++++++++++++++++++------------- >>>> qapi/block-core.json | 13 +++++++++---- >>>> tests/qemu-iotests/087 | 2 +- >>>> tests/qemu-iotests/087.out | 4 ++-- >>>> 4 files changed, 43 insertions(+), 20 deletions(-) >>>> >>>> diff --git a/blockdev.c b/blockdev.c >>>> index 6eedcf5..6d67c80 100644 >>>> --- a/blockdev.c >>>> +++ b/blockdev.c >>>> @@ -2822,17 +2822,12 @@ out: >>>> void qmp_blockdev_add(BlockdevOptions *options, Error **errp) >>>> { >>>> QmpOutputVisitor *ov = qmp_output_visitor_new(); >>>> - BlockBackend *blk; >>>> + BlockDriverState *bs; >>>> + BlockBackend *blk = NULL; >>>> QObject *obj; >>>> QDict *qdict; >>>> Error *local_err = NULL; >>>> - /* Require an ID in the top level */ >>>> - if (!options->has_id) { >>>> - error_setg(errp, "Block device needs an ID"); >>>> - goto fail; >>>> - } >>>> - >>>> /* TODO Sort it out in raw-posix and drive_new(): Reject aio=native with >>>> * cache.direct=false instead of silently switching to aio=threads, except >>>> * when called from drive_new(). >>>> @@ -2860,14 +2855,37 @@ void qmp_blockdev_add(BlockdevOptions *options, Error **errp) >>>> qdict_flatten(qdict); >>>> - blk = blockdev_init(NULL, qdict, &local_err); >>>> - if (local_err) { >>>> - error_propagate(errp, local_err); >>>> - goto fail; >>>> + if (options->has_id) { >>>> + blk = blockdev_init(NULL, qdict, &local_err); >>>> + if (local_err) { >>>> + error_propagate(errp, local_err); >>>> + goto fail; >>>> + } >>>> + >>>> + bs = blk_bs(blk); >>>> + } else { >>>> + int ret; >>>> + >>>> + if (!qdict_get_try_str(qdict, "node-name")) { >>>> + error_setg(errp, "'id' and/or 'node-name' need to be specified for " >>>> + "the root node"); >>>> + goto fail; >>>> + } >>>> + >>>> + bs = NULL; >>>> + ret = bdrv_open(&bs, NULL, NULL, qdict, BDRV_O_RDWR | BDRV_O_CACHE_WB, >>>> + NULL, errp); >>> Now all the qdict entries that aren't parsed by bdrv_open() but >>> converted into flags by blockdev_init() are broken if you don't give an >>> id. This includes read-only, discard, the cache options - in other >>> words, enough to make this "support" completely useless. >> See patch 23. > No matter what I'll find there (okay, I've cheated and already quickly > looked at it before writing this), this answer tells me that you're > doing things in the wrong order. To cite the cover letter of v1 regarding the patch order, here for patches 1 and 2: > Patches 35 [22] and 36 [23] are kind of a follow-up to these; but > patch 35 [22] depends on patch 34 [21] which is the reason why there > is a large gap between patch 2 and 35 [22]. I remember needing patch 2 before the rest. Maybe I don't which means I can move it back. I'll see. My reasoning is the following: Yes, blockdev-add for BB-less BDS trees is nearly unusable after this. But it's enough for patch 2; and it does not break anything, because trying that simply always threw an error before this patch. Afterwards, only most usages will result in an error, but the ones introduced in patch 2 are fine. And patch 23 then "unlocks" all useful usages (that I can see). So nothing breaks at any point, we're only slowly allowing more use cases (and everything contained within a single series). Max
On 2015-03-04 at 08:39, Kevin Wolf wrote: > Am 09.02.2015 um 18:11 hat Max Reitz geschrieben: >> If the "id" field is missing from the options given to blockdev-add, >> just omit the BlockBackend and create the BlockDriverState tree alone. >> >> However, if "id" is missing, "node-name" must be specified; otherwise, >> the BDS tree would no longer be accessible. >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> blockdev.c | 44 +++++++++++++++++++++++++++++++------------- >> qapi/block-core.json | 13 +++++++++---- >> tests/qemu-iotests/087 | 2 +- >> tests/qemu-iotests/087.out | 4 ++-- >> 4 files changed, 43 insertions(+), 20 deletions(-) >> >> diff --git a/blockdev.c b/blockdev.c >> index 6eedcf5..6d67c80 100644 >> --- a/blockdev.c >> +++ b/blockdev.c >> @@ -2822,17 +2822,12 @@ out: >> void qmp_blockdev_add(BlockdevOptions *options, Error **errp) >> { >> QmpOutputVisitor *ov = qmp_output_visitor_new(); >> - BlockBackend *blk; >> + BlockDriverState *bs; >> + BlockBackend *blk = NULL; >> QObject *obj; >> QDict *qdict; >> Error *local_err = NULL; >> >> - /* Require an ID in the top level */ >> - if (!options->has_id) { >> - error_setg(errp, "Block device needs an ID"); >> - goto fail; >> - } >> - >> /* TODO Sort it out in raw-posix and drive_new(): Reject aio=native with >> * cache.direct=false instead of silently switching to aio=threads, except >> * when called from drive_new(). >> @@ -2860,14 +2855,37 @@ void qmp_blockdev_add(BlockdevOptions *options, Error **errp) >> >> qdict_flatten(qdict); >> >> - blk = blockdev_init(NULL, qdict, &local_err); >> - if (local_err) { >> - error_propagate(errp, local_err); >> - goto fail; >> + if (options->has_id) { >> + blk = blockdev_init(NULL, qdict, &local_err); >> + if (local_err) { >> + error_propagate(errp, local_err); >> + goto fail; >> + } >> + >> + bs = blk_bs(blk); >> + } else { >> + int ret; >> + >> + if (!qdict_get_try_str(qdict, "node-name")) { >> + error_setg(errp, "'id' and/or 'node-name' need to be specified for " >> + "the root node"); >> + goto fail; >> + } >> + >> + bs = NULL; >> + ret = bdrv_open(&bs, NULL, NULL, qdict, BDRV_O_RDWR | BDRV_O_CACHE_WB, >> + NULL, errp); > Now all the qdict entries that aren't parsed by bdrv_open() but > converted into flags by blockdev_init() are broken if you don't give an > id. This includes read-only, discard, the cache options - in other > words, enough to make this "support" completely useless. > > I guess I need to dig out my cache mode series and get it ready to be > merged. Once this is in, the parsing of the other necessary options > should be easy to move into bdrv_open(). > > Unless you have a good reason why we should merge this patch in its > hardly functional state now, I would consider those conversions a > dependency of this one. Because I will not send out another version of this series for the next two weeks, I'll just give an overview of where I'm currently at (for each of the patches you replied to): We need this patch before "block: Make bdrv_is_inserted() recursive", because that patch makes the problems apparent which are fixed in my bdrv_close_all() series (071 fails, because flushing the qcow2 metadata suddenly no longer results in an error; this is because bdrv_is_inserted() now returns false because the file BDS under qcow2 has been closed (because it has a BB), so it's no longer inserted). Therefore, we need patch 2 before said patch to work around the problem (until it's fixed by the respective series, which depends on this one, however). I could probably split up patch 22 and pull the first part up front, along with patch 23, but this would require work, which I don't deem necessary. As I said before, in my opinion this patch does not break any existing valid usage of blockdev-add, it's just that there are some use cases we want to have after this patch that do not work yet. But they will be added in a later patch in this series, so I don't find this an issue. Therefore, for this patch, I'll add a note into the commit message stating that there will be a follow-up patch allowing options that are not parsed by bdrv_open() (like caching) to be given. Max
diff --git a/blockdev.c b/blockdev.c index 6eedcf5..6d67c80 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2822,17 +2822,12 @@ out: void qmp_blockdev_add(BlockdevOptions *options, Error **errp) { QmpOutputVisitor *ov = qmp_output_visitor_new(); - BlockBackend *blk; + BlockDriverState *bs; + BlockBackend *blk = NULL; QObject *obj; QDict *qdict; Error *local_err = NULL; - /* Require an ID in the top level */ - if (!options->has_id) { - error_setg(errp, "Block device needs an ID"); - goto fail; - } - /* TODO Sort it out in raw-posix and drive_new(): Reject aio=native with * cache.direct=false instead of silently switching to aio=threads, except * when called from drive_new(). @@ -2860,14 +2855,37 @@ void qmp_blockdev_add(BlockdevOptions *options, Error **errp) qdict_flatten(qdict); - blk = blockdev_init(NULL, qdict, &local_err); - if (local_err) { - error_propagate(errp, local_err); - goto fail; + if (options->has_id) { + blk = blockdev_init(NULL, qdict, &local_err); + if (local_err) { + error_propagate(errp, local_err); + goto fail; + } + + bs = blk_bs(blk); + } else { + int ret; + + if (!qdict_get_try_str(qdict, "node-name")) { + error_setg(errp, "'id' and/or 'node-name' need to be specified for " + "the root node"); + goto fail; + } + + bs = NULL; + ret = bdrv_open(&bs, NULL, NULL, qdict, BDRV_O_RDWR | BDRV_O_CACHE_WB, + NULL, errp); + if (ret < 0) { + goto fail; + } } - if (bdrv_key_required(blk_bs(blk))) { - blk_unref(blk); + if (bs && bdrv_key_required(bs)) { + if (blk) { + blk_unref(blk); + } else { + bdrv_unref(bs); + } error_setg(errp, "blockdev-add doesn't support encrypted devices"); goto fail; } diff --git a/qapi/block-core.json b/qapi/block-core.json index a3fdaf0..e85ef40 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -1260,9 +1260,12 @@ # # @driver: block driver name # @id: #optional id by which the new block device can be referred to. -# This is a required option on the top level of blockdev-add, and -# currently not allowed on any other level. -# @node-name: #optional the name of a block driver state node (Since 2.0) +# This option is only allowed on the top level of blockdev-add. +# A BlockBackend will be created by blockdev-add if and only if +# this option is given. +# @node-name: #optional the name of a block driver state node (Since 2.0). +# This option is required on the top level of blockdev-add if +# the @id option is not given there. # @discard: #optional discard-related options (default: ignore) # @cache: #optional cache-related options # @aio: #optional AIO backend (default: threads) @@ -1717,7 +1720,9 @@ ## # @blockdev-add: # -# Creates a new block device. +# Creates a new block device. If the @id option is given at the top level, a +# BlockBackend will be created; otherwise, @node-name is mandatory at the top +# level and no BlockBackend will be created. # # @options: block device options for the new device # diff --git a/tests/qemu-iotests/087 b/tests/qemu-iotests/087 index 8694749..af44299 100755 --- a/tests/qemu-iotests/087 +++ b/tests/qemu-iotests/087 @@ -54,7 +54,7 @@ size=128M _make_test_img $size echo -echo === Missing ID === +echo === Missing ID and node-name === echo run_qemu <<EOF diff --git a/tests/qemu-iotests/087.out b/tests/qemu-iotests/087.out index 0ba2e43..b0aa06d 100644 --- a/tests/qemu-iotests/087.out +++ b/tests/qemu-iotests/087.out @@ -1,12 +1,12 @@ QA output created by 087 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 -=== Missing ID === +=== Missing ID and node-name === Testing: QMP_VERSION {"return": {}} -{"error": {"class": "GenericError", "desc": "Block device needs an ID"}} +{"error": {"class": "GenericError", "desc": "'id' and/or 'node-name' need to be specified for the root node"}} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN"} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "ide1-cd0", "tray-open": true}}
If the "id" field is missing from the options given to blockdev-add, just omit the BlockBackend and create the BlockDriverState tree alone. However, if "id" is missing, "node-name" must be specified; otherwise, the BDS tree would no longer be accessible. Signed-off-by: Max Reitz <mreitz@redhat.com> --- blockdev.c | 44 +++++++++++++++++++++++++++++++------------- qapi/block-core.json | 13 +++++++++---- tests/qemu-iotests/087 | 2 +- tests/qemu-iotests/087.out | 4 ++-- 4 files changed, 43 insertions(+), 20 deletions(-)