From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:52598)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <lcapitulino@redhat.com>) id 1X5fuo-0000Ld-UW
	for qemu-devel@nongnu.org; Fri, 11 Jul 2014 14:51:33 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <lcapitulino@redhat.com>) id 1X5fui-0003iH-Fu
	for qemu-devel@nongnu.org; Fri, 11 Jul 2014 14:51:26 -0400
Received: from mx1.redhat.com ([209.132.183.28]:57811)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <lcapitulino@redhat.com>) id 1X5fui-0003hp-8B
	for qemu-devel@nongnu.org; Fri, 11 Jul 2014 14:51:20 -0400
Date: Fri, 11 Jul 2014 14:51:15 -0400
From: Luiz Capitulino <lcapitulino@redhat.com>
Message-ID: <20140711145115.28849a0e@redhat.com>
In-Reply-To: <87tx6nsx3l.fsf@blackfin.pond.sub.org>
References: <20140708141728.412a3f1c@redhat.com>
	<87fvi9i8tx.fsf@blackfin.pond.sub.org>
	<20140710103641.31b658c0@redhat.com>
	<87vbr4vtwp.fsf@blackfin.pond.sub.org>
	<53C004F2.1000602@redhat.com>
	<87tx6nsx3l.fsf@blackfin.pond.sub.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH] scripts: qapi-event.py: support vendor
 extension
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Markus Armbruster <armbru@redhat.com>
Cc: qemu-devel <qemu-devel@nongnu.org>, wenchaoqemu@gmail.com

On Fri, 11 Jul 2014 18:01:50 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Eric Blake <eblake@redhat.com> writes:
> 
> > On 07/11/2014 08:42 AM, Markus Armbruster wrote:
> >
> >>>> Can anybody think of a use of c_var() that needs '.' preserved?
> >>>
> >>> Doing the replace in c_var() breaks some struct accesses in the generated
> >>> code. I didn't look deeper to determine the users though.
> >> 
> >> Feels like a misuse of c_var() to me.
> >> 
> >> Dig, dig...  aha.  generate_visit_struct_fields() joins QAPI names
> >> separated by '.', and passes the result to c_var().  I expect such code
> >> to break when one of the names contains '.'.
> >> 
> >> It does indeed; try the appended patch to see it yourself.  It generates
> >> 
> >> struct VersionInfo
> >> {
> >>     struct 
> >>     {
> >>         int64_t major;
> >>         int64_t minor;
> >>         int64_t micro;
> >>     } qemu;
> >
> > Wait a minute.  Isn't this one of the three cases of nested structs,
> > where we were already arguing that nested structs are evil if we are
> > going to introduce a fuller syntax for optional argument defaults?
> 
> Yes.
> 
> >>     struct 
> >>     {
> >>         int64_t major;
> >>         int64_t minor;
> >>         int64_t micro;
> >>     } __com.redhat.crap;
> >>     char *package;
> >> };
> >> 
> >> Conclusion: this is simply a bug that needs fixing.
> >> 
> >> 
> >> diff --git a/qapi/common.json b/qapi/common.json
> >> index 4e9a21f..74ccde3 100644
> >> --- a/qapi/common.json
> >> +++ b/qapi/common.json
> >> @@ -52,6 +52,7 @@
> >>  ##
> >>  { 'type': 'VersionInfo',
> >>    'data': {'qemu': {'major': 'int', 'minor': 'int', 'micro': 'int'},
> >> +           '__com.redhat.crap': {'major': 'int', 'minor': 'int', 'micro': 'int'},
> >>             'package': 'str'} }
> >
> > And the fix may be as simple as ditching support for nested structs in
> > the first place, and rewriting this as:
> >
> > { 'type': 'VersionDetails',
> >   'data': { major': 'int', 'minor': 'int', 'micro': 'int'} }
> > { 'type': 'VersionInfo',
> >   'data': {'qemu': 'VersionDetails',
> >            '__com.redhat.crap': 'VersionDetails',
> >            'package': 'str' } }
> >
> > But the fact that we are still discussing makes it obvious - this is 2.2
> > material.
> 
> Agree.  Let's ditch nested structs and see whether there are any misuses
> of c_var() left.

This is an honest question: do we really want to drop nested struct support,
wasn't it added by the block layer or am I just confused?