From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50651) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XUaHd-000892-3C for qemu-devel@nongnu.org; Thu, 18 Sep 2014 07:54:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XUaHW-0007Ge-UL for qemu-devel@nongnu.org; Thu, 18 Sep 2014 07:53:57 -0400 Received: from mx1.redhat.com ([209.132.183.28]:1808) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XUaHW-0007G7-Mj for qemu-devel@nongnu.org; Thu, 18 Sep 2014 07:53:50 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s8IBriZd028451 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Thu, 18 Sep 2014 07:53:44 -0400 Date: Thu, 18 Sep 2014 13:53:42 +0200 From: Kevin Wolf Message-ID: <20140918115342.GC3761@noname.redhat.com> References: <1410881796-18452-1-git-send-email-kwolf@redhat.com> <1410881796-18452-2-git-send-email-kwolf@redhat.com> <87a95xw5on.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87a95xw5on.fsf@blackfin.pond.sub.org> Subject: Re: [Qemu-devel] [PATCH 1/6] block/qapi: Add cache information to query-block List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: qemu-devel@nongnu.org, stefanha@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