Re: [Qemu-devel] [PATCH 2/3] block: Add QMP support for streaming to an intermediate layer
On Thu, Mar 12, 2015 at 04:45:17PM +0100, Kevin Wolf wrote: One issue that I'm finding is that when we move the block-stream job to an intermediate node, where the device name is empty, we get messages like Device '' is busy. My first thought was then make it 'Source/Target device is busy' without mentioning any name. In the context of any given command, it would still be clear which BDS is meant. In fact, I have argued before that mentioning the device name in an error to a command that refers to a specific device is redundant and should be avoided. The problem here is that it's not stream_start() that generates the error, but block_job_create(), which doesn't know which role it's bs argument has for the block job. So it can't decide whether to say source device, target device or something completely different. The problem is actually not there. The error message generated by block_job_create() is block device is in use by block job: stream. It's bdrv_op_is_blocked() that adds the extra Device '' is busy. error_setg(errp, Device '%s' is busy: %s, bdrv_get_device_name(bs), error_get_pretty(blocker-reason)); I can use the same approach as in the BlockJobInfo case and fall back to the node name if the device name is empty, but the problem is that bdrv_get_device_name() is used all over the place, so this probably needs a more general solution. Even at the moment the backing blocker set by bdrv_set_backing_hd() has problems: error_setg(bs-backing_blocker, device is used as backing hd of '%s', bdrv_get_device_name(bs)); This only works if 'bs' is a root node, but if you try to perform an operation on the backing image of another backing image, you get a device is used as backing hd of ''. Error messages aside, I would probably need to check all uses of bdrv_get_device_name() because there could be more surprises if the node is not at the root. Berto
Re: [Qemu-devel] [PATCH 2/3] block: Add QMP support for streaming to an intermediate layer
On Thu, Mar 12, 2015 at 04:45:17PM +0100, Kevin Wolf wrote: One issue that I'm finding is that when we move the block-stream job to an intermediate node, where the device name is empty, we get messages like Device '' is busy. I can use node names instead, but they are also not guaranteed to exist. My first thought was then make it 'Source/Target device is busy' without mentioning any name. In the context of any given command, it would still be clear which BDS is meant. There's a related problem that I discussed on IRC with Kevin and Eric but that I think needs further deliberation. The BlockJobInfo object returned by query-block-jobs identifies the owner of the job using the 'device' field. If jobs can be in any intermediate node then we cannot simply rely on the device name. We also cannot simply replace it with a node name because 1) it might not exist and 2) existing libvirt versions expect a device name. So I see several alternatives: a) Add a new 'node-name' field to BlockJobInfo. It's simple, 'device' keeps the current semantics so we don't break compatibility. b) Make 'device' return the device name as it currently does, or the node name if it's not present. The main problem is that libvirt cannot easily know what to expect. On the other hand since both device and node-name share the same namespace the returned value is not ambiguous. c) Make 'device' return the same name that was used when the job was created. It's maybe simpler for libvirt than option b), but it would require us to remember how the job was created, possibly in the BlockJob structure. This is personally my least favorite option. d) Create a new query command that returns a different data structure. I would opt for a) or b), but I'd like to hear if you have a different opinion. Regarding the 'block-stream' command, I think the current option to reuse the 'device' parameter to refer to either a device or a node name is ok, so I'll go forward with that one. On the other hand, having an owner BDS for a block job is considered a mistake meanwhile because there is no clear rule which BDS to pick when the job involves more than one. Does it really matter as long as all the operations blockers are correctly set? In fact, without tying a job to a BDS, it could be just a background job instead of specifically a block job. I don't understand what you mean by this. Berto
Re: [Qemu-devel] [PATCH 2/3] block: Add QMP support for streaming to an intermediate layer
On Tue, Mar 17, 2015 at 09:22:55AM -0600, Eric Blake wrote: The BlockJobInfo object returned by query-block-jobs identifies the owner of the job using the 'device' field. If jobs can be in any intermediate node then we cannot simply rely on the device name. We also cannot simply replace it with a node name because 1) it might not exist and 2) existing libvirt versions expect a device name. So I see several alternatives: a) Add a new 'node-name' field to BlockJobInfo. It's simple, 'device' keeps the current semantics so we don't break compatibility. b) Make 'device' return the device name as it currently does, or the node name if it's not present. The main problem is that libvirt cannot easily know what to expect. On the other hand since both device and node-name share the same namespace the returned value is not ambiguous. If libvirt is new enough to create the block job via node name instead of device name, then it is also new enough to expect a node name instead of device name in the returned job information. That is clear. c) Make 'device' return the same name that was used when the job was created. It's maybe simpler for libvirt than option b), but it would require us to remember how the job was created, possibly in the BlockJob structure. This is personally my least favorite option. If you're going to reuse 'device' on the creation, then reuse it on the reporting. The problem with c) is that the name is only needed early in the operation to get a BlockDriverState, we don't use it afterwards. So returning the same name that was used to request the operation would force us to keep that information internally, because in the case of a job owned by a BlockDriverState with both device name 'virtio0' and node name 'node0' it's otherwise impossible to know if the job was requested using 'virtio0' or 'node0'. d) Create a new query command that returns a different data structure. I would opt for a) or b), but I'd like to hear if you have a different opinion. I'm kind of leaning towards b). But note that in the example I just mentioned, if you create a job using 'node0' to refer to the node, you would still get 'virtio0' in return, and not 'node0'. With b) you only get 'node0' if the node does not have a device name. Berto
Re: [Qemu-devel] [PATCH 2/3] block: Add QMP support for streaming to an intermediate layer
Am 17.03.2015 um 16:00 hat Alberto Garcia geschrieben: On Thu, Mar 12, 2015 at 04:45:17PM +0100, Kevin Wolf wrote: One issue that I'm finding is that when we move the block-stream job to an intermediate node, where the device name is empty, we get messages like Device '' is busy. I can use node names instead, but they are also not guaranteed to exist. My first thought was then make it 'Source/Target device is busy' without mentioning any name. In the context of any given command, it would still be clear which BDS is meant. There's a related problem that I discussed on IRC with Kevin and Eric but that I think needs further deliberation. The BlockJobInfo object returned by query-block-jobs identifies the owner of the job using the 'device' field. If jobs can be in any intermediate node then we cannot simply rely on the device name. We also cannot simply replace it with a node name because 1) it might not exist and 2) existing libvirt versions expect a device name. So I see several alternatives: a) Add a new 'node-name' field to BlockJobInfo. It's simple, 'device' keeps the current semantics so we don't break compatibility. b) Make 'device' return the device name as it currently does, or the node name if it's not present. The main problem is that libvirt cannot easily know what to expect. On the other hand since both device and node-name share the same namespace the returned value is not ambiguous. c) Make 'device' return the same name that was used when the job was created. It's maybe simpler for libvirt than option b), but it would require us to remember how the job was created, possibly in the BlockJob structure. This is personally my least favorite option. d) Create a new query command that returns a different data structure. I would opt for a) or b), but I'd like to hear if you have a different opinion. e) Considering that we want to generalise block jobs into background jobs, make 'device' optional and fill it only if the owner BDS has a device name. If not, the field is omitted. If e) is possible for libvirt (Eric?), I would vote for that. Otherwise, I think b) would be nicest. Regarding the 'block-stream' command, I think the current option to reuse the 'device' parameter to refer to either a device or a node name is ok, so I'll go forward with that one. Great! On the other hand, having an owner BDS for a block job is considered a mistake meanwhile because there is no clear rule which BDS to pick when the job involves more than one. Does it really matter as long as all the operations blockers are correctly set? No, it doesn't actively break anything, it's just a little arbitrary. In fact, without tying a job to a BDS, it could be just a background job instead of specifically a block job. I don't understand what you mean by this. We could have other long-running operations in the background that could make use of the same infrastructure if it weren't tied to block devices (for no reason, the actual block job infrastructure doesn't need anything block-like). One example that came up in the past is live migration. This is not directly related to your series, but some background to keep in mind while evolving the external APIs. Kevin
Re: [Qemu-devel] [PATCH 2/3] block: Add QMP support for streaming to an intermediate layer
On 03/17/2015 09:00 AM, Alberto Garcia wrote: The BlockJobInfo object returned by query-block-jobs identifies the owner of the job using the 'device' field. If jobs can be in any intermediate node then we cannot simply rely on the device name. We also cannot simply replace it with a node name because 1) it might not exist and 2) existing libvirt versions expect a device name. So I see several alternatives: a) Add a new 'node-name' field to BlockJobInfo. It's simple, 'device' keeps the current semantics so we don't break compatibility. b) Make 'device' return the device name as it currently does, or the node name if it's not present. The main problem is that libvirt cannot easily know what to expect. On the other hand since both device and node-name share the same namespace the returned value is not ambiguous. If libvirt is new enough to create the block job via node name instead of device name, then it is also new enough to expect a node name instead of device name in the returned job information. That is, I'm okay with either: old libvirt: creates job using 'device' as a device name, status about the job is reported with 'device' as the device name new libvirt: creates job using 'device' as a node name, status about the job is reported with 'device' as the node name (no new parameter, 'device' is used on both creation and query as a [poorly-named] device-or-node, back-compat is obvious) or with: old libvirt: creates job using 'device' as a device name, status about the job is reported with 'device' as the device name new libvirt: creates job using 'node' as a node name, status about the job is reported with 'node' as the node name (new parameter; old usage remains the same, and new usage has proper naming, but now we have to track which name is in use) c) Make 'device' return the same name that was used when the job was created. It's maybe simpler for libvirt than option b), but it would require us to remember how the job was created, possibly in the BlockJob structure. This is personally my least favorite option. If you're going to reuse 'device' on the creation, then reuse it on the reporting. d) Create a new query command that returns a different data structure. I would opt for a) or b), but I'd like to hear if you have a different opinion. I'm kind of leaning towards b). Regarding the 'block-stream' command, I think the current option to reuse the 'device' parameter to refer to either a device or a node name is ok, so I'll go forward with that one. Particularly if we don't have two parameters for starting the job, then we don't need two parameters for reporting it. On the other hand, having an owner BDS for a block job is considered a mistake meanwhile because there is no clear rule which BDS to pick when the job involves more than one. Does it really matter as long as all the operations blockers are correctly set? In fact, without tying a job to a BDS, it could be just a background job instead of specifically a block job. I don't understand what you mean by this. Berto -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 2/3] block: Add QMP support for streaming to an intermediate layer
Am 11.03.2015 um 17:38 hat Alberto Garcia geschrieben:
On Thu, Mar 05, 2015 at 03:09:58PM +0100, Kevin Wolf wrote:
{ 'command': 'block-stream',
- 'data': { 'device': 'str', '*base': 'str', '*backing-file': 'str',
-'*speed': 'int', '*on-error': 'BlockdevOnError' } }
+ 'data': { 'device': 'str', '*base': 'str', '*top': 'str',
+'*backing-file': 'str', '*speed': 'int',
+'*on-error': 'BlockdevOnError' } }
There is no point in specifying some root node as 'device' that
isn't actually involved in the operation; worse, it isn't even
possible in the general case because 'top' could have multiple
users/parents.
A better interface would probably be to allow node names for
'device' and leave everything else as it is.
Ok, I changed the code and it does make the implementation simpler.
One issue that I'm finding is that when we move the block-stream
job to an intermediate node, where the device name is empty, we get
messages like Device '' is busy.
I can use node names instead, but they are also not guaranteed to
exist. I heard there was a plan to auto-generate names, and searching
the archives I found this patch by Jeff Cody:
http://lists.gnu.org/archive/html/qemu-devel/2014-06/msg04057.html
But it seems that it was never merged?
If we are going to have a scenario where a parameter can mean either a
device or a node name, we need a clear way to identify that node.
Yes, autogenerated node names were not merged yet. And if they were,
they wouldn't make for very good error messages either.
My first thought was then make it 'Source/Target device is busy'
without mentioning any name. In the context of any given command, it
would still be clear which BDS is meant. In fact, I have argued before
that mentioning the device name in an error to a command that refers to
a specific device is redundant and should be avoided.
The problem here is that it's not stream_start() that generates the
error, but block_job_create(), which doesn't know which role it's bs
argument has for the block job. So it can't decide whether to say
source device, target device or something completely different.
On the other hand, having an owner BDS for a block job is considered a
mistake meanwhile because there is no clear rule which BDS to pick when
the job involves more than one. In fact, without tying a job to a BDS,
it could be just a background job instead of specifically a block job.
I'm not saying that this conversion should be done now, but just to give
you some background about the direction we're generally taking.
So in the light of this, it might be reasonable to move the bs-job
check with the error check to the callers.
Another, less invasive, option would be to replace the error_set() call
in block_job_create() by error_copy(bs-job-blocker). We're not really
op blockers code here, so might be somewhat ugly, but I think eventually
the check is going to be fully replaced by op blockers anyway, so using
the same message now could make sense.
Jeff, as you are working on op blockers, do you have an opinion on this?
Kevin
Re: [Qemu-devel] [PATCH 2/3] block: Add QMP support for streaming to an intermediate layer
On Thu, Mar 05, 2015 at 03:09:58PM +0100, Kevin Wolf wrote:
{ 'command': 'block-stream',
- 'data': { 'device': 'str', '*base': 'str', '*backing-file': 'str',
-'*speed': 'int', '*on-error': 'BlockdevOnError' } }
+ 'data': { 'device': 'str', '*base': 'str', '*top': 'str',
+'*backing-file': 'str', '*speed': 'int',
+'*on-error': 'BlockdevOnError' } }
There is no point in specifying some root node as 'device' that
isn't actually involved in the operation; worse, it isn't even
possible in the general case because 'top' could have multiple
users/parents.
A better interface would probably be to allow node names for
'device' and leave everything else as it is.
Ok, I changed the code and it does make the implementation simpler.
One issue that I'm finding is that when we move the block-stream
job to an intermediate node, where the device name is empty, we get
messages like Device '' is busy.
I can use node names instead, but they are also not guaranteed to
exist. I heard there was a plan to auto-generate names, and searching
the archives I found this patch by Jeff Cody:
http://lists.gnu.org/archive/html/qemu-devel/2014-06/msg04057.html
But it seems that it was never merged?
If we are going to have a scenario where a parameter can mean either a
device or a node name, we need a clear way to identify that node.
Berto
Re: [Qemu-devel] [PATCH 2/3] block: Add QMP support for streaming to an intermediate layer
On Thu, Mar 05, 2015 at 03:09:58PM +0100, Kevin Wolf wrote:
{ 'command': 'block-stream',
- 'data': { 'device': 'str', '*base': 'str', '*backing-file': 'str',
-'*speed': 'int', '*on-error': 'BlockdevOnError' } }
+ 'data': { 'device': 'str', '*base': 'str', '*top': 'str',
+'*backing-file': 'str', '*speed': 'int',
+'*on-error': 'BlockdevOnError' } }
A better interface would probably be to allow node names for
'device' and leave everything else as it is.
That's possible, but if the API is the same how does libvirt know if
streaming to an intermediate node is possible or not?
There is no point in specifying some root node as 'device' that
isn't actually involved in the operation; worse, it isn't even
possible in the general case because 'top' could have multiple
users/parents.
The latter is actually a good point, if 'top' is used by 2+ parents
then it makes sense that the ownership of the block job in in 'top',
not in the root node.
I think I will need to investigate the consequences of that.
Berto
Re: [Qemu-devel] [PATCH 2/3] block: Add QMP support for streaming to an intermediate layer
Am 20.02.2015 um 14:53 hat Alberto Garcia geschrieben:
This adds the 'top' parameter to the 'block-stream' QMP command and
checks that its value is valid before passing it to stream_start().
Signed-off-by: Alberto Garcia be...@igalia.com
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1013,6 +1013,9 @@
# with query-block-jobs. The operation can be stopped before it has
completed
# using the block-job-cancel command.
#
+# Data is copied to the top image, which defaults to the active layer if no
other
+# file is selected.
+#
# If a base file is specified then sectors are not copied from that base
file and
# its backing chain. When streaming completes the image file will have the
base
# file as its backing file. This can be used to stream a subset of the
backing
@@ -1025,8 +1028,14 @@
#
# @base: #optional the common backing file name
#
-# @backing-file: #optional The backing file string to write into the active
-# layer. This filename is not validated.
+# @top:#optional Top image, only sectors below this image are streamed
+#into it.
+#
+#If not specified, the top image is the active layer.
+#(Since 2.3)
+#
+# @backing-file: #optional The backing file string to write into the top
+# image. This filename is not validated.
#
# If a pathname string is such that it cannot be
# resolved by QEMU, that means that subsequent QMP
or
@@ -1052,8 +1061,9 @@
# Since: 1.1
##
{ 'command': 'block-stream',
- 'data': { 'device': 'str', '*base': 'str', '*backing-file': 'str',
-'*speed': 'int', '*on-error': 'BlockdevOnError' } }
+ 'data': { 'device': 'str', '*base': 'str', '*top': 'str',
+'*backing-file': 'str', '*speed': 'int',
+'*on-error': 'BlockdevOnError' } }
While in patch 1 it would only be nice to avoid the additional argument,
I think we absolutely have to avoid it here in the external interface.
There is no point in specifying some root node as 'device' that isn't
actually involved in the operation; worse, it isn't even possible in the
general case because 'top' could have multiple users/parents.
A better interface would probably be to allow node names for 'device'
and leave everything else as it is.
Kevin
Re: [Qemu-devel] [PATCH 2/3] block: Add QMP support for streaming to an intermediate layer
Kevin Wolf kw...@redhat.com writes:
Am 23.02.2015 um 13:23 hat Alberto Garcia geschrieben:
On Fri, Feb 20, 2015 at 03:38:04PM -0700, Eric Blake wrote:
+if (has_top) {
+top_bs = bdrv_find_backing_image(bs, top);
+if (top_bs == NULL) {
+error_set(errp, QERR_TOP_NOT_FOUND, top);
+goto out;
+}
If I understand correctly, bdrv_find_backing_image has problems for
backing nodes that don't have a file name. Given our shift towards
node names, I think we really want to target node names rather than
file names when specifying which node we will use as the top bound
receiving the stream operations.
Sure I can change that, but note that the 'base' parameter also
receives a file name and uses bdrv_find_backing_image, so I guess it
makes sense to change it in both sides.
Yes, using the file name for identifying nodes was a mistake. We're
going to replace all occurrences of it sooner or later. Not sure if
someone is actively working on this currently - Markus?
Not currently, sorry. I agree it needs doing.
For your patch series, I think it's good enough to use node names for
the new parameter. Converting old parameters is a separate issue.
Makes sense.
Re: [Qemu-devel] [PATCH 2/3] block: Add QMP support for streaming to an intermediate layer
On Fri, Feb 20, 2015 at 03:38:04PM -0700, Eric Blake wrote:
+if (has_top) {
+top_bs = bdrv_find_backing_image(bs, top);
+if (top_bs == NULL) {
+error_set(errp, QERR_TOP_NOT_FOUND, top);
+goto out;
+}
If I understand correctly, bdrv_find_backing_image has problems for
backing nodes that don't have a file name. Given our shift towards
node names, I think we really want to target node names rather than
file names when specifying which node we will use as the top bound
receiving the stream operations.
Sure I can change that, but note that the 'base' parameter also
receives a file name and uses bdrv_find_backing_image, so I guess it
makes sense to change it in both sides.
+#define QERR_TOP_NOT_FOUND \
+ERROR_CLASS_GENERIC_ERROR, Top '%s' not found
+
Please don't. Just use error_setg() at the right place with the
direct message (existing QERR_ macros are a legacy holdover, and we
shouldn't be creating more of them).
Ok, I'll fix that.
I'll wait for more comments regarding the top / base parameters before
resubmitting the patches.
Thanks,
Berto
Re: [Qemu-devel] [PATCH 2/3] block: Add QMP support for streaming to an intermediate layer
Am 23.02.2015 um 13:23 hat Alberto Garcia geschrieben:
On Fri, Feb 20, 2015 at 03:38:04PM -0700, Eric Blake wrote:
+if (has_top) {
+top_bs = bdrv_find_backing_image(bs, top);
+if (top_bs == NULL) {
+error_set(errp, QERR_TOP_NOT_FOUND, top);
+goto out;
+}
If I understand correctly, bdrv_find_backing_image has problems for
backing nodes that don't have a file name. Given our shift towards
node names, I think we really want to target node names rather than
file names when specifying which node we will use as the top bound
receiving the stream operations.
Sure I can change that, but note that the 'base' parameter also
receives a file name and uses bdrv_find_backing_image, so I guess it
makes sense to change it in both sides.
Yes, using the file name for identifying nodes was a mistake. We're
going to replace all occurrences of it sooner or later. Not sure if
someone is actively working on this currently - Markus?
For your patch series, I think it's good enough to use node names for
the new parameter. Converting old parameters is a separate issue.
Kevin
[Qemu-devel] [PATCH 2/3] block: Add QMP support for streaming to an intermediate layer
This adds the 'top' parameter to the 'block-stream' QMP command and
checks that its value is valid before passing it to stream_start().
Signed-off-by: Alberto Garcia be...@igalia.com
---
blockdev.c| 19 +++
hmp.c | 2 +-
include/qapi/qmp/qerror.h | 3 +++
qapi/block-core.json | 18 ++
qmp-commands.hx | 2 +-
5 files changed, 34 insertions(+), 10 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 06628ca..2404f89 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2099,6 +2099,7 @@ static void block_job_cb(void *opaque, int ret)
void qmp_block_stream(const char *device,
bool has_base, const char *base,
+ bool has_top, const char *top,
bool has_backing_file, const char *backing_file,
bool has_speed, int64_t speed,
bool has_on_error, BlockdevOnError on_error,
@@ -2106,6 +2107,7 @@ void qmp_block_stream(const char *device,
{
BlockDriverState *bs;
BlockDriverState *base_bs = NULL;
+BlockDriverState *top_bs;
AioContext *aio_context;
Error *local_err = NULL;
const char *base_name = NULL;
@@ -2114,7 +2116,7 @@ void qmp_block_stream(const char *device,
on_error = BLOCKDEV_ON_ERROR_REPORT;
}
-bs = bdrv_find(device);
+top_bs = bs = bdrv_find(device);
if (!bs) {
error_set(errp, QERR_DEVICE_NOT_FOUND, device);
return;
@@ -2123,12 +2125,21 @@ void qmp_block_stream(const char *device,
aio_context = bdrv_get_aio_context(bs);
aio_context_acquire(aio_context);
-if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_STREAM, errp)) {
+if (has_top) {
+top_bs = bdrv_find_backing_image(bs, top);
+if (top_bs == NULL) {
+error_set(errp, QERR_TOP_NOT_FOUND, top);
+goto out;
+}
+assert(bdrv_get_aio_context(top_bs) == aio_context);
+}
+
+if (bdrv_op_is_blocked(top_bs, BLOCK_OP_TYPE_STREAM, errp)) {
goto out;
}
if (has_base) {
-base_bs = bdrv_find_backing_image(bs, base);
+base_bs = bdrv_find_backing_image(top_bs, base);
if (base_bs == NULL) {
error_set(errp, QERR_BASE_NOT_FOUND, base);
goto out;
@@ -2148,7 +2159,7 @@ void qmp_block_stream(const char *device,
/* backing_file string overrides base bs filename */
base_name = has_backing_file ? backing_file : base_name;
-stream_start(bs, NULL, base_bs, base_name, has_speed ? speed : 0,
+stream_start(bs, top_bs, base_bs, base_name, has_speed ? speed : 0,
on_error, block_job_cb, bs, local_err);
if (local_err) {
error_propagate(errp, local_err);
diff --git a/hmp.c b/hmp.c
index b47f331..28f7adb 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1239,7 +1239,7 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict)
const char *base = qdict_get_try_str(qdict, base);
int64_t speed = qdict_get_try_int(qdict, speed, 0);
-qmp_block_stream(device, base != NULL, base, false, NULL,
+qmp_block_stream(device, base != NULL, base, false, NULL, false, NULL,
qdict_haskey(qdict, speed), speed,
true, BLOCKDEV_ON_ERROR_REPORT, error);
diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
index 986260f..d075f78 100644
--- a/include/qapi/qmp/qerror.h
+++ b/include/qapi/qmp/qerror.h
@@ -127,6 +127,9 @@ void qerror_report_err(Error *err);
#define QERR_SET_PASSWD_FAILED \
ERROR_CLASS_GENERIC_ERROR, Could not set password
+#define QERR_TOP_NOT_FOUND \
+ERROR_CLASS_GENERIC_ERROR, Top '%s' not found
+
#define QERR_UNDEFINED_ERROR \
ERROR_CLASS_GENERIC_ERROR, An undefined error has occurred
diff --git a/qapi/block-core.json b/qapi/block-core.json
index a3fdaf0..3073be6 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1013,6 +1013,9 @@
# with query-block-jobs. The operation can be stopped before it has completed
# using the block-job-cancel command.
#
+# Data is copied to the top image, which defaults to the active layer if no
other
+# file is selected.
+#
# If a base file is specified then sectors are not copied from that base file
and
# its backing chain. When streaming completes the image file will have the
base
# file as its backing file. This can be used to stream a subset of the backing
@@ -1025,8 +1028,14 @@
#
# @base: #optional the common backing file name
#
-# @backing-file: #optional The backing file string to write into the active
-# layer. This filename is not validated.
+# @top:#optional Top image, only sectors below this image are streamed
+#into it.
+#
+#If not specified, the top image is the active layer.
+#(Since 2.3)
+#
+# @backing-file: #optional The backing file string to write into the top
+# image.
Re: [Qemu-devel] [PATCH 2/3] block: Add QMP support for streaming to an intermediate layer
On 02/20/2015 06:53 AM, Alberto Garcia wrote:
This adds the 'top' parameter to the 'block-stream' QMP command and
checks that its value is valid before passing it to stream_start().
Signed-off-by: Alberto Garcia be...@igalia.com
---
blockdev.c| 19 +++
hmp.c | 2 +-
include/qapi/qmp/qerror.h | 3 +++
qapi/block-core.json | 18 ++
qmp-commands.hx | 2 +-
5 files changed, 34 insertions(+), 10 deletions(-)
@@ -2123,12 +2125,21 @@ void qmp_block_stream(const char *device,
aio_context = bdrv_get_aio_context(bs);
aio_context_acquire(aio_context);
-if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_STREAM, errp)) {
+if (has_top) {
+top_bs = bdrv_find_backing_image(bs, top);
+if (top_bs == NULL) {
+error_set(errp, QERR_TOP_NOT_FOUND, top);
+goto out;
+}
If I understand correctly, bdrv_find_backing_image has problems for
backing nodes that don't have a file name. Given our shift towards node
names, I think we really want to target node names rather than file
names when specifying which node we will use as the top bound receiving
the stream operations.
+++ b/include/qapi/qmp/qerror.h
@@ -127,6 +127,9 @@ void qerror_report_err(Error *err);
#define QERR_SET_PASSWD_FAILED \
ERROR_CLASS_GENERIC_ERROR, Could not set password
+#define QERR_TOP_NOT_FOUND \
+ERROR_CLASS_GENERIC_ERROR, Top '%s' not found
+
Please don't. Just use error_setg() at the right place with the direct
message (existing QERR_ macros are a legacy holdover, and we shouldn't
be creating more of them).
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
