| Message ID | 49b845c4a362ffd64ec78bbe0b165cd7addd2a4b.1424439295.git.berto@igalia.com |
|---|---|
| State | New |
| Headers | show |
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 <berto@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).
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
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
Kevin Wolf <kwolf@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.
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 <berto@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
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
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
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
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
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 > >
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
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
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
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. 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' } } ## # @block-job-set-speed: diff --git a/qmp-commands.hx b/qmp-commands.hx index a85d847..a02f19f 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -978,7 +978,7 @@ EQMP { .name = "block-stream", - .args_type = "device:B,base:s?,speed:o?,backing-file:s?,on-error:s?", + .args_type = "device:B,base:s?,top:s?,speed:o?,backing-file:s?,on-error:s?", .mhandler.cmd_new = qmp_marshal_input_block_stream, },
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 <berto@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(-)