Re: [Qemu-devel] [RFC PATCH 11/11] qcow2: Add data file to ImageInfoSpecificQCow2

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2914 bytes --]

On 31.01.19 18:55, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  qapi/block-core.json | 1 +
>  block/qcow2.c        | 6 +++++-
>  2 files changed, 6 insertions(+), 1 deletion(-)

[...]

> diff --git a/block/qcow2.c b/block/qcow2.c
> index 4959bf16a4..e3427f9fcd 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1459,7 +1459,9 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
>      if (s->incompatible_features & QCOW2_INCOMPAT_DATA_FILE) {
>          s->data_file = bdrv_open_child(NULL, options, "data-file", bs,
>                                         &child_file, false, &local_err);
> -        if (!s->data_file) {
> +        if (s->data_file) {
> +            s->image_data_file = g_strdup(s->data_file->bs->filename);
> +        } else {
>              if (s->image_data_file) {
>                  error_free(local_err);
>                  local_err = NULL;

Ah, this is what I looked for in the last patch. :-)

(i.e. it should be in the last patch, not here)

I think as it is it is just wrong, though.  If I pass enough options at
runtime, this will overwrite the image header:

$ ./qemu-img create -f qcow2 -o data_file=foo.raw foo.qcow2 64M
$ ./qemu-img create -f raw bar.raw 64M
$ ./qemu-img info foo.qcow2
[...]
    data file: foo.raw
[...]
$ ./qemu-io --image-opts \
    file.filename=foo.qcow2,data-file.driver=file,\
data-file.filename=bar.raw,lazy-refcounts=on \
    -c 'write 0 64k'
# (The lazy-refcounts is so the image header is updated)
$ ./qemu-img info foo.qcow2
[...]
    data file: bar.raw
[...]

The right thing would probably to check whether the header extension
exists (i.e. if s->image_data_file is non-NULL) and if it does not (it
is NULL), s->image_data_file gets set; because there are no valid images
with the external data file flag set where there is no such header
extension.  So we must be in the process of creating the image right now.

But even then, I don't quite like setting it here and not creating the
header extension as part of qcow2_co_create().  I can see why you've
done it this way, but creating a "bad" image on purpose (one with the
external data file bit set, but no such header extension present yet) in
order to detect and rectify this case when it is first opened (and the
opening code assuming that any such broken image must be one that is
opened the first time) is a bit weird.

I suppose doing it right (if you agree with the paragraph before the
last one) and adding a comment would make it less weird
("s->image_data_file must be non-NULL for any valid image, so this image
must be one we are creating right now" or something like that).

But still, the issue you point out in your cover letter remains; which
is that the node's filename and the filename given by the user may be
two different things.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]