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

[Max Reitz]

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.

However, instead of extending this function, it may make more sense then
to introduce a new field to the BlockDriver struct which is a
NULL-terminated array of significant option names, or something like
that. If .bdrv_refresh_filename is NULL but that array pointer is not,
then the default implementation could behave accordingly. But this is
something I'd defer to the future, too, unless you can point out a
current block driver that would benefit from this functionality (I don't
think Quorum does, as I said above).

Max

signature.asc
Description: OpenPGP digital signature