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

[Kevin Wolf]

Am 19.02.2019 um 01:47 hat Max Reitz geschrieben:
> On 31.01.19 18:55, Kevin Wolf wrote:
> > Signed-off-by: Kevin Wolf <address@hidden>
> > ---
> >  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)

[RFC PATCH 11/11] qcow2: Add data file to ImageInfoSpecificQCow2

This is the last patch. :-P

> 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.

It's not really a bad image, just one that's a bit cumbersome to use
because you need to specify the 'data-file' option manually.

> 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.

I think what I was planning to do was leaving the path from the image
header in s->image_data_file even when a runtime option overrides it.
After all, ImageInfo is about the image, not about the runtime state.

Image creation would just manually set s->image_data_file before
updating the header.

Kevin

signature.asc
Description: PGP signature