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 <mre...@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
signature.asc
Description: OpenPGP digital signature
