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 <kw...@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)
[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
