[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 10/11] block: add backing-file option to bloc
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v2 10/11] block: add backing-file option to block-stream |
Date: |
Tue, 27 May 2014 14:30:02 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.5.0 |
On 05/27/2014 08:28 AM, Jeff Cody wrote:
> On some image chains, QEMU may not always be able to resolve the
> filenames properly, when updating the backing file of an image
> after a block job.
>
> For instance, certain relative pathnames may fail, or drives may
> have been specified originally by file descriptor (e.g. /dev/fd/???),
> or a relative protocol pathname may have been used.
>
> In these instances, QEMU may lack the information to be able to make
> the correct choice, but the user or management layer most likely does
> have that knowledge.
>
> With this extension to the block-stream api, the user is able to change
> the backing file of the active layer as part of the block-stream
> operation.
>
> This allows the change to be 'safe', in the sense that if the attempt
> to write the active image metadata fails, then the block-stream
> operation returns failure, without disrupting the guest.
>
> If a backing file string is not specified in the command, the backing
> file string to use is determined in the same manner as it was
> previously.
>
> Signed-off-by: Jeff Cody <address@hidden>
> ---
> block/stream.c | 13 ++++++++-----
> blockdev.c | 12 +++++++++++-
> hmp-commands.hx | 2 +-
> hmp.c | 2 ++
> include/block/block_int.h | 2 +-
> qapi-schema.json | 18 +++++++++++++++++-
> qmp-commands.hx | 2 +-
> 7 files changed, 41 insertions(+), 10 deletions(-)
>
> @@ -220,7 +221,7 @@ void stream_start(BlockDriverState *bs, BlockDriverState
> *base,
> const char *base_id, int64_t speed,
> BlockdevOnError on_error,
> BlockDriverCompletionFunc *cb,
> - void *opaque, Error **errp)
> + void *opaque, const char *backing_file_str, Error **errp)
> {
Umm, aren't the 'base_id' and 'backing_file_str' arguments redundant
now? You only need one, not both, because there is only one backing
string to be written (or cleared) into the active file. [1]
> @@ -237,8 +238,10 @@ void stream_start(BlockDriverState *bs, BlockDriverState
> *base,
> }
>
> s->base = base;
> - if (base_id) {
> - pstrcpy(s->backing_file_id, sizeof(s->backing_file_id), base_id);
> + if (backing_file_str) {
> + s->backing_file_str = g_strdup(backing_file_str);
> + } else {
> + s->backing_file_str = g_strdup(base_id);
g_strdup(NULL) is safely NULL, correct? (You hit this case when base_id
is NULL, which happens when base is NULL).
> @@ -1941,8 +1942,17 @@ void qmp_block_stream(bool has_device, const char
> *device,
> return;
> }
>
> + /* if we are streaming from the bottommost base image, then specifying
> + * a backing file does not make sense, and is an error */
Misleading (back to the complaint in 9/11) - omitting base is different
from using the bottommost base image as base. I'd word that more like:
If we are streaming the entire chain, then the result will have no
backing file and specifying a backing name is an error
> + if (base_bs == NULL && has_backing_file) {
> + error_setg(errp, "backing file specified, but streaming from "
> + "bottommost base image");
> + return;
> + }
The 'if' condition is correct and necessary, but the error message could
use better wording; maybe:
backing file specified but no base image supplied
> stream_start(bs, base_bs, base_name, has_speed ? speed : 0,
> - on_error, block_job_cb, bs, &local_err);
> + on_error, block_job_cb, bs,
> + has_backing_file ? backing_file : NULL, &local_err);
[1] Again, how do 'base_name' and 'backing_file' differ at this point in
the game? Isn't it just simpler to do:
has_backing_file ? backing_file : base_name
and use a single parameter?
> +++ b/qapi-schema.json
> @@ -2611,6 +2611,21 @@
> # @base-node-name: #optional the block driver state node name of the
> # common backing file. (Since 2.1)
> #
> +# @backing-file: #optional The backing file string to write into the active
> +# layer. This filename is not validated.
> +#
> +# If a pathname string is such that it cannot be
> +# resolved be QEMU, that means that subsequent QMP
> or
Copy-and-paste strikes again :)
s/be/by/
> +++ b/qmp-commands.hx
> @@ -979,7 +979,7 @@ EQMP
>
> {
> .name = "block-stream",
> - .args_type =
> "device:B?,base:s?,base-node-name:s?,speed:o?,on-error:s?",
> + .args_type =
> "device:B?,base:s?,base-node-name:s?,speed:o?,backing-file:s?,on-error:s?",
> .mhandler.cmd_new = qmp_marshal_input_block_stream,
> },
Missing documentation of backing-file? Oh, the entire block-stream call
is missing documentation, when compared to the block-commit call. Oh
well, I can't fault you for that, although it might be nice to rectify
while we are improving it.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
- [Qemu-devel] [PATCH v2 08/11] block: extend block-commit to accept a string for the backing file, (continued)
- [Qemu-devel] [PATCH v2 08/11] block: extend block-commit to accept a string for the backing file, Jeff Cody, 2014/05/27
- [Qemu-devel] [PATCH v2 07/11] block: Accept node-name arguments for block-commit, Jeff Cody, 2014/05/27
- [Qemu-devel] [PATCH v2 09/11] block: add ability for block-stream to use node-name, Jeff Cody, 2014/05/27
- [Qemu-devel] [PATCH v2 10/11] block: add backing-file option to block-stream, Jeff Cody, 2014/05/27
- [Qemu-devel] [PATCH v2 11/11] block: add QAPI command to allow live backing file change, Jeff Cody, 2014/05/27