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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5823 bytes --]

On 03.05.2016 16:34, Kevin Wolf wrote:
> 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 <mreitz@redhat.com>
>>>>>
>>>>> 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.

Well, I don't particularly care either way. Thus, I'd be fine with
removing insignificant options even from full_open_options if you deem
that useful, but that is something we don't do already so we'd have to
implement it.

I guess I can implement it in this series if I decide to address the
issue you originally raised here ("Is this good enough?"). If it gets
too much, I'd rather handle it in a follow-up, though.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]