[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/6] block/qapi: Add cache information to query-
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH 1/6] block/qapi: Add cache information to query-block |
Date: |
Thu, 18 Sep 2014 13:53:42 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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
- [Qemu-devel] [PATCH 0/6] block: Single-device query-block(-node) and cache info, Kevin Wolf, 2014/09/16
- [Qemu-devel] [PATCH 2/6] block: Add optional device argument to query-block, Kevin Wolf, 2014/09/16
- [Qemu-devel] [PATCH 3/6] block: Introduce query-block-node, Kevin Wolf, 2014/09/16
- [Qemu-devel] [PATCH 4/6] block/hmp: Factor out print_block_info(), Kevin Wolf, 2014/09/16
- [Qemu-devel] [PATCH 5/6] block/hmp: Allow info = NULL in print_block_info(), Kevin Wolf, 2014/09/16
- [Qemu-devel] [PATCH 6/6] block: Allow node-name in HMP 'info block', Kevin Wolf, 2014/09/16