Re: [Qemu-block] [PATCH 06/19] block: Make bdrv_default_refresh_format_filename public

[Kevin Wolf]

Am 03.05.2016 um 15:48 hat Max Reitz geschrieben:
> On 03.05.2016 15:31, Kevin Wolf wrote:
> > Am 03.05.2016 um 13:19 hat Max Reitz geschrieben:
> >> On 02.05.2016 17:36, Kevin Wolf wrote:
> >>> Am 26.04.2016 um 23:32 hat Max Reitz geschrieben:
> >>>> In order to allow block drivers to use that function, it needs to be
> >>>> public. In order to be useful, it needs to take a parameter which allows
> >>>> the caller to specify whether the runtime options allowed by the block
> >>>> driver are actually significant for the guest-visible BDS content.
> >>>>
> >>>> Signed-off-by: Max Reitz <address@hidden>
> >>>
> >>> Is this actually good enough? I expect that many drivers will have some
> >>> options that are significant and other options that aren't. We already
> >>> have some (Quorum: children are significant, rewrite-corrupted isn't),
> >>> but as we convert more things to proper options, we'll get more of them
> >>> (raw-posix: filename is significant, aio=native isn't).
> >>>
> >>> We might actually need to pass a list of significant fields instead that
> >>> append_open_options() can use.
> >>
> >> Well, in theory, every driver with insignificant options would just
> >> implement .bdrv_refresh_filename() however it's needed. Making
> >> bdrv_default_refresh_format_filename() function public is just a way of
> >> keeping that implementation very simple for some drivers that only have
> >> insignificant options.
> >>
> >> I'm not opposed to extending this function in the future when it
> >> actually makes sense. Right now I don't think it does. The only thing
> >> that changes if a significant option is detected is that no plain
> >> filename is generated; however, for Quorum we can never generate such a
> >> filename. Therefore, we cannot use this function for Quorum anyway.
> > 
> > If you integrate it into append_open_options(), I suppose it would also
> > mean that insignificant options are dropped from the json: description,
> > i.e. Quorum would return a json: object with all children, but not the
> > rewrite-corrupted setting. Which I think would be a good thing.
> 
> I'm not sure I do. :-)
> 
> At least right now the JSON version is supposed to contain all options,
> be they significant or not. Let me try to remember my reasoning:
> 
> Ideally, we want to get a filename which *exactly* results in the same
> BDS that we have.

Here I'm not sure we do. :-)

We already don't do this. We filter out any options that are parsed by
the block layer. For example, we don't include the node name or caching
options. If we really wanted to represend the BDS as exactly as we can,
this wouldn't be right and we'd have to fix it.

But as I see it, what we were really after when we implemented things
this way was that we distinguish options that are conceptually part of
some address that points to the image data (which I thought matches your
"significant" options) and other options that just influence our access
patterns and what we do with the image at this address.

The filename (json: or not) consists then only of the address part, as
the other options can differ between qemu invocations without actually
changing which image we see. I don't expect something called a filename
(json: exists just because a plain filename can't represent everything)
to contain various runtime configuration settings, but just a pure
pointer to the image.

> This should always be possible if instead of a plain
> filename one specifies options, e.g. using a JSON filename. However,
> such a JSON filename (or giving options using the dot syntax or as JSON
> with blockdev-add) is cumbersome.
> 
> In many simple cases, we can (re-)construct a plain filename which
> yields exactly the same BDS, though. That's nice so that's what we try
> to do first.
> 
> In some cases, it is impossible to construct a plain filename which
> yields a BDS that will return the same data when accessed. Then, we just
> cannot give such a filename and have to stick to a JSON filename,
> there's no way around this.
> 
> However, sometimes we are in a gray area. We can construct a plain
> filename which yields a slightly different BDS than the one we have; but
> it will return the same data when accessed and thus it is "close
> enough". We then have to make a tradeoff between getting exactly the
> same BDS and having a nice and simple filename. I opted for the latter.

I can imagine that there are use cases for some mechanism to return the
JSON object that creates exactly the same BDS, like you seem to be
envisioning here. I just doubt that it's useful in those cases where we
really wanted a filename and have to go for JSON because we can't do
anything more user friendly.

> However, if we do have to emit a JSON filename at some point in the tree
> I think we've basically "lost" already. If we get to that point, we may
> as well just emit all the options that have been used to construct the
> BDS, even if they don't change the data it yields.

In places where you want a filename (which is mostly, if not
exclusively, messages for the user), emitting everything may just make
an already unfriendly message even worse.

Kevin

pgpzBVNx6ndt4.pgp
Description: PGP signature