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

[Kevin Wolf]

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.

Kevin

> 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

pgpABksq9aPG9.pgp
Description: PGP signature