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