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

[Kevin Wolf]

Am 22.02.2019 um 17:13 hat Max Reitz geschrieben:
> On 22.02.19 16:57, Kevin Wolf wrote:
> > Am 22.02.2019 um 14:51 hat Max Reitz geschrieben:
> >> On 19.02.19 10:17, Kevin Wolf wrote:
> >>> 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
> >>
> >> Sorry, I meant the previous one.
> >>
> >>>> 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.
> >>
> >> Of course it's bad because it doesn't adhere to the specification (which
> >> you could amend, of course, since you add it with this series).  The
> >> spec says "If this bit is set, an external data file name header
> >> extension must be present as well."  Which it isn't until the image is
> >> opened with the data-file option.
> > 
> > Hm, I wonder whether that's a good requirement to make or whether we
> > should indeed change the spec. It wouldn't be so bad to have images that
> > require the data-file runtime option.
> > 
> > I guess we could lift the restriction later if we want to make use of
> > it. But the QEMU code is already written in a way that this works, so
> > maybe just allow it.
> 
> OK for me.
> 
> >>>> 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.
> >>
> >> I'm not talking about ImageInfo here, though, I'm talking about the
> >> image creation process.  The hunk I've quoted should be in the previous
> >> patch, not in this one.
> >>
> >> Which doesn't make wrong what you're saying, though, the ImageInfo
> >> should print what's in the header.
> >>
> >>> Image creation would just manually set s->image_data_file before
> >>> updating the header.
> >>
> >> It should, but currently it does that rather indirectly (by setting the
> >> data-file option which then makes qcow2_do_open() copy it into
> >> s->image_data_file).
> > 
> > I'm not exactly sure what detail in the image creation process you are
> > talking about.
> 
> Mostly the fact that when enough data-file options are provided, the
> header string is overwritten with the filename taken from the node
> that's been specified by those runtime options.
> 
> (Which is what the creation process uses to get the filename into the
> header the first time.)

Okay. That's the obvious one that is fixed with the solution I said I
was planning to use (only storing the actual image header value in
s->image_data_file).

I guess we were just talking past each other then.

> > I confirmed that this way of getting the filename into the header is
> > broken, and it was a known problem when I sent the series, spelt out in
> > the cover letter and in fact fixed in my git branch by now.
> 
> No need to be a bit upset.  As long as we agree, it's all good -- and as
> far as I remember I did acknowledge in my first response that you wrote
> something about this in your cover letter.
> 
> I just wasn't sure what exactly you meant in the cover letter (to me it
> read more like the filename written the first time may be different from
> what the user has specified, thanks to the magic of
> bdrv_refresh_filename() and other things), and that you didn't say how
> you intended to fix it.  So I just wanted to discuss that.

I'm not really upset, I was just worried that you're insisting because
there's something else wrong and I'm just too dense to understand what
it is. Seems this is actually not the case, even better.

> > Is there anything else about the image creation process that needs
> > fixing?
> 
> It was my impression that I went already into too much detail in my
> review of this series as an RFC. :-)

No, this is actually very helpful. After all, I want to do as few
iterations as possible.

> Also, I have to admit that I cannot guarantee a review in a fashion that
> once all my requested changes are incorporated, no further versions will
> be required.

That's probably unavoidable in any review.

Kevin

signature.asc
Description: PGP signature