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 <kw...@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
signature.asc
Description: PGP signature
