Re: [Qemu-devel] [PATCH 1/6] block/qapi: Add cache information to query-block

[Markus Armbruster]

Kevin Wolf <address@hidden> 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.