Kevin Wolf <kw...@redhat.com> writes: > 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.
Writing documentation saying an optional member is in fact not optional feels weird, and saying it's never there feels even weirder :) >> 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. I'm not excited about the additional types, either. >> 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? I'm leaning towards adding BlockdevCacheInfo, because you say there's precedence, and because I like the schema to be accurate more than I dislike the additional type.
