Re: [Qemu-devel] [RFC PATCH 10/11] qcow2: Store data file name in the image

[Kevin Wolf <kwolf@redhat.com>]

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

Am 22.02.2019 um 16:43 hat Max Reitz geschrieben:
> On 22.02.19 16:35, Kevin Wolf wrote:
> > Am 22.02.2019 um 15:16 hat Max Reitz geschrieben:
> >> On 19.02.19 10:04, Kevin Wolf wrote:
> >>> Am 19.02.2019 um 01:18 hat Max Reitz geschrieben:
> >>>> On 31.01.19 18:55, Kevin Wolf wrote:
> >>>>> Rather than requiring that the external data file node is passed
> >>>>> explicitly when creating the qcow2 node, store the filename in the
> >>>>> designated header extension during .bdrv_create and read it from there
> >>>>> as a default during .bdrv_open.
> >>>>>
> >>>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> >>>>> ---
> >>>>>  block/qcow2.h              |  1 +
> >>>>>  block/qcow2.c              | 69 +++++++++++++++++++++++++++++++++++++-
> >>>>>  tests/qemu-iotests/082.out | 27 +++++++++++++++
> >>>>>  3 files changed, 96 insertions(+), 1 deletion(-)
> >>>>
> >>>> [...]
> >>>>
> >>>>> diff --git a/block/qcow2.c b/block/qcow2.c
> >>>>> index 6cf862e8b9..4959bf16a4 100644
> >>>>> --- a/block/qcow2.c
> >>>>> +++ b/block/qcow2.c
> >>>>> @@ -398,6 +398,20 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
> >>>>>  #endif
> >>>>>              break;
> >>>>>  
> >>>>> +        case QCOW2_EXT_MAGIC_DATA_FILE:
> >>>>> +        {
> >>>>> +            s->image_data_file = g_malloc0(ext.len + 1);
> >>>>> +            ret = bdrv_pread(bs->file, offset, s->image_data_file, ext.len);
> >>>>> +            if (ret < 0) {
> >>>>> +                error_setg_errno(errp, -ret, "ERROR: Could not data file name");
> >>>>
> >>>> I think you accidentally a word.
> >>>>
> >>>>> +                return ret;
> >>>>> +            }
> >>>>> +#ifdef DEBUG_EXT
> >>>>> +            printf("Qcow2: Got external data file %s\n", s->image_data_file);
> >>>>> +#endif
> >>>>> +            break;
> >>>>> +        }
> >>>>> +
> >>>>>          default:
> >>>>>              /* unknown magic - save it in case we need to rewrite the header */
> >>>>>              /* If you add a new feature, make sure to also update the fast
> >>>>> @@ -1444,7 +1458,18 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
> >>>>>      /* Open external data file */
> >>>>>      if (s->incompatible_features & QCOW2_INCOMPAT_DATA_FILE) {
> >>>>>          s->data_file = bdrv_open_child(NULL, options, "data-file", bs,
> >>>>> -                                       &child_file, false, errp);
> >>>>> +                                       &child_file, false, &local_err);
> >>>>> +        if (!s->data_file) {
> >>>>> +            if (s->image_data_file) {
> >>>>> +                error_free(local_err);
> >>>>> +                local_err = NULL;
> >>>>
> >>>> This looked a bit weird to me at first because I was wondering why you
> >>>> wouldn't just pass allow_none=true and then handle errors (by passing
> >>>> them on).  But right, we want to retry with a filename set, maybe that
> >>>> makes more sense of the options.
> >>>
> >>> I think we want the normal error message for the !s->image_data_file
> >>> case. With allow_none=true, we would have to come up with a new one here
> >>> (in the else branch below).
> >>>
> >>>> Hm.  But then again, do we really?  It matches what we do with backing
> >>>> files, but that does give at least me headaches from time to time.  How
> >>>> bad would it be to allow either passing all valid options through
> >>>> @options (which would make qcow2 ignore the string in the header), or
> >>>> use the filename given in the header alone?
> >>>
> >>> You mean offering only one of the two ways to configure the node?
> >>
> >> Either just the filename from the image header, or ignore that and take
> >> all options from the user (who'd have to give a syntactically complete
> >> QAPI BlockdevRef object).
> >>
> >>> The 'data-file' runtime option is a must so that libvirt can build the
> >>> graph node by node (and possibly use file descriptor passing one day).
> >>> But having to specify the option every time is very unfriendly for human
> >>> users, so I think allowing to store the file name in the header is a
> >>> must, too.
> >>
> >> Sure.  But I don't know whether we have to support taking the filename
> >> from the image header, and the user overriding some of the node's
> >> options (e.g. caching).
> > 
> > So essentially this would mean passing NULL instead of options to
> > bdrv_open_child() when we retry with the filename from the header.
> > 
> > But it's inconsistent with all other places, which comes with two
> 
> "all other places"?  Really it's just backing files, as everywhere else
> there is no filename that doesn't come from the command line.
> 
> Yes, you can use -drive file=foo.qcow2,file.locking=off, but I consider
> that case a bit different.  Although maybe it really isn't. *shrug*

Why would it be different? At least as a user, I consider them the same.
It's true that bs->file and bs->backing come with some additional magic,
but I don't think it makes a difference for this aspect.

I'll add the third example I can think of and I'm sure you'll love this:

$ x86_64-softmmu/qemu-system-x86_64 \
  -drive file=/tmp/test.vmdk,extents.0.cache.no-flush=on

So we have three examples that work this way. Can you think of any
existing counterexample?

> > problems. It's confusing for users who are used to overriding just that
> > one option of a child. And do we actually spare you any headaches or do
> > we create new ones because we have now two different behaviours of
> > bdrv_open_child() callers that we must preserve in the future?
> 
> It means I can act out on my pain by being angry on how .backing
> behaves.  That's better for my health than having to keep it in because
> it's the same behavior everywhere and it's officially me who's in the wrong.

You're officially wrong. ;-)

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]