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

[Max Reitz]

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

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.

Max

signature.asc
Description: OpenPGP digital signature