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

[Kevin Wolf <kwolf@redhat.com>]

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