| Message ID | 1467893497-2434-2-git-send-email-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu 07 Jul 2016 02:11:27 PM CEST, Kevin Wolf wrote: > In order to remove the necessity to use BlockBackend names in the > external API, we want to allow node-names everywhere. This converts > block-stream to accept a node-name without lifting the restriction that > we're operating at a root node. > > In case of an invalid device name, the command returns the GenericError > error class now instead of DeviceNotFound, because this is what > qmp_get_root_bs() returns. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > blockdev.c | 32 ++++++++++++++++++++------------ > qapi/block-core.json | 5 +---- > qmp-commands.hx | 2 +- > tests/qemu-iotests/030 | 2 +- > 4 files changed, 23 insertions(+), 18 deletions(-) > > diff --git a/blockdev.c b/blockdev.c > index 0f8065c..01e57c9 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -1172,6 +1172,23 @@ fail: > return dinfo; > } > > +static BlockDriverState *qmp_get_root_bs(const char *name, Error **errp) > +{ > + BlockDriverState *bs; > + > + bs = bdrv_lookup_bs(name, name, errp); > + if (bs == NULL) { > + return NULL; > + } > + > + if (!bdrv_has_blk(bs)) { > + error_setg(errp, "Need a root block node"); > + return NULL; > + } Since b6d2e59995f when you create a block job a new BlockBackend is created on top of the BDS. What happens with this check if we allow creating jobs in a non-root BDS? Berto
Am 07.07.2016 um 14:59 hat Alberto Garcia geschrieben: > On Thu 07 Jul 2016 02:11:27 PM CEST, Kevin Wolf wrote: > > In order to remove the necessity to use BlockBackend names in the > > external API, we want to allow node-names everywhere. This converts > > block-stream to accept a node-name without lifting the restriction that > > we're operating at a root node. > > > > In case of an invalid device name, the command returns the GenericError > > error class now instead of DeviceNotFound, because this is what > > qmp_get_root_bs() returns. > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > --- > > blockdev.c | 32 ++++++++++++++++++++------------ > > qapi/block-core.json | 5 +---- > > qmp-commands.hx | 2 +- > > tests/qemu-iotests/030 | 2 +- > > 4 files changed, 23 insertions(+), 18 deletions(-) > > > > diff --git a/blockdev.c b/blockdev.c > > index 0f8065c..01e57c9 100644 > > --- a/blockdev.c > > +++ b/blockdev.c > > @@ -1172,6 +1172,23 @@ fail: > > return dinfo; > > } > > > > +static BlockDriverState *qmp_get_root_bs(const char *name, Error **errp) > > +{ > > + BlockDriverState *bs; > > + > > + bs = bdrv_lookup_bs(name, name, errp); > > + if (bs == NULL) { > > + return NULL; > > + } > > + > > + if (!bdrv_has_blk(bs)) { > > + error_setg(errp, "Need a root block node"); > > + return NULL; > > + } > > Since b6d2e59995f when you create a block job a new BlockBackend is > created on top of the BDS. What happens with this check if we allow > creating jobs in a non-root BDS? Hm, you mean I'd start first an intermediate streaming job and then I can call commands on the target node that I shouldn't be able to call? It's a good point, but I think op blockers would prevent that this actually works. If we wanted to keep exactly the old set of nodes that is allowed, we could make qmp_get_root_bs() look for a _named_ BlockBackend. But that would kind of defeat the purpose of this series, which wants to allow these commands on named nodes that are directly used for -device. Another option - and maybe that makes more sense than the old rule anyway because you already can have a BB anywhere in the middle of the graph - would be to check that the node doesn't have any non-BB parent. This would restrict some cases that are possible today. Or, considering that requiring a device name didn't really work as a check because you can have BBs anywhere, just leave it. Sooner or later we'll want all commands to work on any node anyway. I think the main reason to require root nodes today is just op blockers. Kevin
On Thu 07 Jul 2016 04:17:21 PM CEST, Kevin Wolf wrote: >> > +static BlockDriverState *qmp_get_root_bs(const char *name, Error **errp) >> > +{ >> > + BlockDriverState *bs; >> > + >> > + bs = bdrv_lookup_bs(name, name, errp); >> > + if (bs == NULL) { >> > + return NULL; >> > + } >> > + >> > + if (!bdrv_has_blk(bs)) { >> > + error_setg(errp, "Need a root block node"); >> > + return NULL; >> > + } >> >> Since b6d2e59995f when you create a block job a new BlockBackend is >> created on top of the BDS. What happens with this check if we allow >> creating jobs in a non-root BDS? > > Hm, you mean I'd start first an intermediate streaming job and then I > can call commands on the target node that I shouldn't be able to call? > It's a good point, but I think op blockers would prevent that this > actually works. Yes, they would but a) the user would get a misleading error message ("you cannot start that job because the device is temporarily being used" rather than "you cannot start that block job there at all"). Probably not so important. b) all the code after the qmp_get_root_bs() call would run under the (incorrect) assumption that the node is a root BDS. This probably doesn't break anything at the moment but leaves a door open for surprises. > Another option - and maybe that makes more sense than the old rule > anyway because you already can have a BB anywhere in the middle of the > graph - would be to check that the node doesn't have any non-BB > parent. This would restrict some cases that are possible today. Which ones? Berto
Am 07.07.2016 um 16:39 hat Alberto Garcia geschrieben: > On Thu 07 Jul 2016 04:17:21 PM CEST, Kevin Wolf wrote: > >> > +static BlockDriverState *qmp_get_root_bs(const char *name, Error **errp) > >> > +{ > >> > + BlockDriverState *bs; > >> > + > >> > + bs = bdrv_lookup_bs(name, name, errp); > >> > + if (bs == NULL) { > >> > + return NULL; > >> > + } > >> > + > >> > + if (!bdrv_has_blk(bs)) { > >> > + error_setg(errp, "Need a root block node"); > >> > + return NULL; > >> > + } > >> > >> Since b6d2e59995f when you create a block job a new BlockBackend is > >> created on top of the BDS. What happens with this check if we allow > >> creating jobs in a non-root BDS? > > > > Hm, you mean I'd start first an intermediate streaming job and then I > > can call commands on the target node that I shouldn't be able to call? > > It's a good point, but I think op blockers would prevent that this > > actually works. > > Yes, they would but > > a) the user would get a misleading error message ("you cannot start that > job because the device is temporarily being used" rather than "you > cannot start that block job there at all"). Probably not so > important. > > b) all the code after the qmp_get_root_bs() call would run under the > (incorrect) assumption that the node is a root BDS. This probably > doesn't break anything at the moment but leaves a door open for > surprises. Yes, I understand that. The truth is that our current op blockers just don't quite cut it and we need to move away from them so that we can finally allow jobs on every node without giving up safety. > > Another option - and maybe that makes more sense than the old rule > > anyway because you already can have a BB anywhere in the middle of the > > graph - would be to check that the node doesn't have any non-BB > > parent. This would restrict some cases that are possible today. > > Which ones? -drive if=none,file=backing.qcow2,id=backing -drive if=virtio,file=overlay.qcow2,backing=backing You got a BB name for the backing file node, so can run any command that wants a root node on it. The backing file blocker will still prevent most actions, but that's the same situation as with node names. So I guess the new surprises won't be any worse. :-) Kevin
On 07/07/2016 06:11 AM, Kevin Wolf wrote: > In order to remove the necessity to use BlockBackend names in the > external API, we want to allow node-names everywhere. This converts > block-stream to accept a node-name without lifting the restriction that > we're operating at a root node. > > In case of an invalid device name, the command returns the GenericError > error class now instead of DeviceNotFound, because this is what > qmp_get_root_bs() returns. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > blockdev.c | 32 ++++++++++++++++++++------------ > qapi/block-core.json | 5 +---- > qmp-commands.hx | 2 +- > tests/qemu-iotests/030 | 2 +- > 4 files changed, 23 insertions(+), 18 deletions(-) > The interface change looks okay; but due to Berto's comments, I'm not sure it is worth giving R-b yet if you plan on changing the check for whether a node name properly qualifies as a root name.
Am 08.07.2016 um 00:45 hat Eric Blake geschrieben: > On 07/07/2016 06:11 AM, Kevin Wolf wrote: > > In order to remove the necessity to use BlockBackend names in the > > external API, we want to allow node-names everywhere. This converts > > block-stream to accept a node-name without lifting the restriction that > > we're operating at a root node. > > > > In case of an invalid device name, the command returns the GenericError > > error class now instead of DeviceNotFound, because this is what > > qmp_get_root_bs() returns. > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > --- > > blockdev.c | 32 ++++++++++++++++++++------------ > > qapi/block-core.json | 5 +---- > > qmp-commands.hx | 2 +- > > tests/qemu-iotests/030 | 2 +- > > 4 files changed, 23 insertions(+), 18 deletions(-) > > > > The interface change looks okay; but due to Berto's comments, I'm not > sure it is worth giving R-b yet if you plan on changing the check for > whether a node name properly qualifies as a root name. Initially I intended to address the comment with some change, but since I realised that you already can put a BB everywhere and therefore this doesn't protect anything against intentional actions anyway, I'm not so sure any more. Do you have an opintion on this? More input would be appreciated. Kevin
On 07/08/2016 04:01 AM, Kevin Wolf wrote: > Am 08.07.2016 um 00:45 hat Eric Blake geschrieben: >> On 07/07/2016 06:11 AM, Kevin Wolf wrote: >>> In order to remove the necessity to use BlockBackend names in the >>> external API, we want to allow node-names everywhere. This converts >>> block-stream to accept a node-name without lifting the restriction that >>> we're operating at a root node. >>> >>> In case of an invalid device name, the command returns the GenericError >>> error class now instead of DeviceNotFound, because this is what >>> qmp_get_root_bs() returns. >>> >>> Signed-off-by: Kevin Wolf <kwolf@redhat.com> >>> --- >>> blockdev.c | 32 ++++++++++++++++++++------------ >>> qapi/block-core.json | 5 +---- >>> qmp-commands.hx | 2 +- >>> tests/qemu-iotests/030 | 2 +- >>> 4 files changed, 23 insertions(+), 18 deletions(-) >>> >> >> The interface change looks okay; but due to Berto's comments, I'm not >> sure it is worth giving R-b yet if you plan on changing the check for >> whether a node name properly qualifies as a root name. > > Initially I intended to address the comment with some change, but since > I realised that you already can put a BB everywhere and therefore this > doesn't protect anything against intentional actions anyway, I'm not so > sure any more. > > Do you have an opintion on this? More input would be appreciated. I still need to re-read the other sub-thread closely, but yes, I'll try to chime in after I've had a chance to think about implications.
Am 07.07.2016 um 16:17 hat Kevin Wolf geschrieben: > Am 07.07.2016 um 14:59 hat Alberto Garcia geschrieben: > > On Thu 07 Jul 2016 02:11:27 PM CEST, Kevin Wolf wrote: > > > In order to remove the necessity to use BlockBackend names in the > > > external API, we want to allow node-names everywhere. This converts > > > block-stream to accept a node-name without lifting the restriction that > > > we're operating at a root node. > > > > > > In case of an invalid device name, the command returns the GenericError > > > error class now instead of DeviceNotFound, because this is what > > > qmp_get_root_bs() returns. > > > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > > --- > > > blockdev.c | 32 ++++++++++++++++++++------------ > > > qapi/block-core.json | 5 +---- > > > qmp-commands.hx | 2 +- > > > tests/qemu-iotests/030 | 2 +- > > > 4 files changed, 23 insertions(+), 18 deletions(-) > > > > > > diff --git a/blockdev.c b/blockdev.c > > > index 0f8065c..01e57c9 100644 > > > --- a/blockdev.c > > > +++ b/blockdev.c > > > @@ -1172,6 +1172,23 @@ fail: > > > return dinfo; > > > } > > > > > > +static BlockDriverState *qmp_get_root_bs(const char *name, Error **errp) > > > +{ > > > + BlockDriverState *bs; > > > + > > > + bs = bdrv_lookup_bs(name, name, errp); > > > + if (bs == NULL) { > > > + return NULL; > > > + } > > > + > > > + if (!bdrv_has_blk(bs)) { > > > + error_setg(errp, "Need a root block node"); > > > + return NULL; > > > + } > > > > Since b6d2e59995f when you create a block job a new BlockBackend is > > created on top of the BDS. What happens with this check if we allow > > creating jobs in a non-root BDS? > > Hm, you mean I'd start first an intermediate streaming job and then I > can call commands on the target node that I shouldn't be able to call? > It's a good point, but I think op blockers would prevent that this > actually works. > > If we wanted to keep exactly the old set of nodes that is allowed, we > could make qmp_get_root_bs() look for a _named_ BlockBackend. But that > would kind of defeat the purpose of this series, which wants to allow > these commands on named nodes that are directly used for -device. > > Another option - and maybe that makes more sense than the old rule > anyway because you already can have a BB anywhere in the middle of the > graph - would be to check that the node doesn't have any non-BB parent. This is what I'm implementing now. The reason for this is that bdrv_has_blk() obviously rejects configurations where you have only a node name, but no BB. And the whole point of the series is to move towards a model without named BBs, so this would mean that you can only run block job on attached nodes, which doesn't make a lot of sense (and gives qemu-iotests some trouble). With this option implemented, a node that isn't attached anywhere can be used for root node commands, as it should. Kevin
diff --git a/blockdev.c b/blockdev.c index 0f8065c..01e57c9 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1172,6 +1172,23 @@ fail: return dinfo; } +static BlockDriverState *qmp_get_root_bs(const char *name, Error **errp) +{ + BlockDriverState *bs; + + bs = bdrv_lookup_bs(name, name, errp); + if (bs == NULL) { + return NULL; + } + + if (!bdrv_has_blk(bs)) { + error_setg(errp, "Need a root block node"); + return NULL; + } + + return bs; +} + void hmp_commit(Monitor *mon, const QDict *qdict) { const char *device = qdict_get_str(qdict, "device"); @@ -3011,7 +3028,6 @@ void qmp_block_stream(const char *device, bool has_on_error, BlockdevOnError on_error, Error **errp) { - BlockBackend *blk; BlockDriverState *bs; BlockDriverState *base_bs = NULL; AioContext *aio_context; @@ -3022,22 +3038,14 @@ void qmp_block_stream(const char *device, on_error = BLOCKDEV_ON_ERROR_REPORT; } - blk = blk_by_name(device); - if (!blk) { - error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, - "Device '%s' not found", device); + bs = qmp_get_root_bs(device, errp); + if (!bs) { return; } - aio_context = blk_get_aio_context(blk); + aio_context = bdrv_get_aio_context(bs); aio_context_acquire(aio_context); - if (!blk_is_available(blk)) { - error_setg(errp, "Device '%s' has no medium", device); - goto out; - } - bs = blk_bs(blk); - if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_STREAM, errp)) { goto out; } diff --git a/qapi/block-core.json b/qapi/block-core.json index ac8f5f6..f069e40 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -1413,7 +1413,7 @@ # On successful completion the image file is updated to drop the backing file # and the BLOCK_JOB_COMPLETED event is emitted. # -# @device: the device name +# @device: the device name or node-name of a root node # # @base: #optional the common backing file name # @@ -1438,9 +1438,6 @@ # 'stop' and 'enospc' can only be used if the block device # supports io-status (see BlockInfo). Since 1.3. # -# Returns: Nothing on success -# If @device does not exist, DeviceNotFound -# # Since: 1.1 ## { 'command': 'block-stream', diff --git a/qmp-commands.hx b/qmp-commands.hx index 6937e83..0588fd6 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -1118,7 +1118,7 @@ Copy data from a backing file into a block device. Arguments: -- "device": The device's ID, must be unique (json-string) +- "device": The device name or node-name of a root node (json-string) - "base": The file name of the backing image above which copying starts (json-string, optional) - "backing-file": The backing file string to write into the active layer. This diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030 index 3ac2443..107049b 100755 --- a/tests/qemu-iotests/030 +++ b/tests/qemu-iotests/030 @@ -126,7 +126,7 @@ class TestSingleDrive(iotests.QMPTestCase): def test_device_not_found(self): result = self.vm.qmp('block-stream', device='nonexistent') - self.assert_qmp(result, 'error/class', 'DeviceNotFound') + self.assert_qmp(result, 'error/class', 'GenericError') class TestSmallerBackingFile(iotests.QMPTestCase):
In order to remove the necessity to use BlockBackend names in the external API, we want to allow node-names everywhere. This converts block-stream to accept a node-name without lifting the restriction that we're operating at a root node. In case of an invalid device name, the command returns the GenericError error class now instead of DeviceNotFound, because this is what qmp_get_root_bs() returns. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- blockdev.c | 32 ++++++++++++++++++++------------ qapi/block-core.json | 5 +---- qmp-commands.hx | 2 +- tests/qemu-iotests/030 | 2 +- 4 files changed, 23 insertions(+), 18 deletions(-)