Am 18.09.2014 um 13:04 hat Markus Armbruster geschrieben: > Before this patch, QAPI type BlockdevCacheOptions is used only as a > member of BlockdevOptionsBase. > > BlockdevOptionsBase is a collection configuration settings. > Consequently, it's a complex type whose members are optional exactly > when the configuration setting is optional. Makes sense. > > Complication: a few options are wrapped in another complex type, > BlockdevCacheOptions. It's optional, but that's not sufficient, it's > members are all optional, too. > > BlockdevCacheOptions is the only complex member of BlockdevOptionsBase. > The others are all simple or enum. > > In this patch, you reuse BlockdevCacheOptions for a purpose other than > configuration: *querying* configuration. In the query result, neither > the complex type nor its members are optional. The schema reflects the > former (the patch hunk has 'cache', not '*cache'), but not the latter. > > Do we want the schema to reflect reality accurately? > > If no, we should still document the places where it doesn't, like this > one.
That's a fair requirement. If anything is optional in a return value, we should specify the conditions under which it is there or missing. Including, of course, if it's always or never there. > If yes, how can we fix this particular inaccuracy? > > The obvious solution is not to reuse BlockdevCacheOptions. Create a > second type, identical except for its members aren't optional. I can introduce a BlockdevCacheInfo instead. While I'm not completely excited about having two structs for each option dict (one for configuring, one for querying), there's precedence for this and it's at least a small struct this time. > Another idea is to add means to the schema to declare a member "deep > optional": not just the member is optional, but if it's present, its > members are optional, too. Begs the question whether its member's > member's are optional. Hmm. "deep optional" doesn't sound like it makes a lot of sense conceptually, even if it might happen to be a hack that gets us the right result in this one specific case. > Yet another idea is to declare nesting configuration options stupid, and > just not do it, but it may be too late for that. > > Other ideas? I think the choice is between adding BlockdevCacheInfo and changing documentation while reusing BlockdevCacheOptions. Both are fine with me. Which one do you prefer? Kevin
