Re: [Qemu-devel] [PATCH 4/4] qapi: Rename ChardevBackend member "memory" to "ringbuf"

[Markus Armbruster]

Amos Kong <address@hidden> writes:

> On Mon, Jul 01, 2013 at 11:28:58AM +0200, Markus Armbruster wrote:
>> Amos Kong <address@hidden> writes:
>> 
>> > On Mon, Jul 01, 2013 at 10:10:40AM +0200, Markus Armbruster wrote:
>> >> Amos Kong <address@hidden> writes:
>> >> 
>> >> > On Fri, Jun 28, 2013 at 07:19:39PM +0200, Markus Armbruster wrote:
>> >> >> Eric Blake <address@hidden> writes:
>> >> >> 
>> >> >> > On 06/27/2013 08:22 AM, Markus Armbruster wrote:
>> >> >> >> Commit 1da48c6 called the new member "memory" after commit 3949e59
>> >> >> >> standardized "ringbuf".  Rename for consistency.
>> >> >> >> 
>> >> >> >> However, member name "memory" is visible in QMP since 1.5.  It's
>> >> >> >> undocumented just like the driver name.  Keep it working anyway.
>> >> >> >> 
>> >> >> >> Cc: address@hidden
>> >> >> >> Signed-off-by: Markus Armbruster <address@hidden>
>> >> >> >> ---
>> >> >> >>  qapi-schema.json |  6 ++++--
>> >> >> >>  qemu-char.c      | 11 ++++++-----
>> >> >> >>  2 files changed, 10 insertions(+), 7 deletions(-)
>> >> >> >> 
>> >> >> >> diff --git a/qapi-schema.json b/qapi-schema.json
>> >> >> >> index 6445da6..b3df8a5 100644
>> >> >> >> --- a/qapi-schema.json
>> >> >> >> +++ b/qapi-schema.json
>> >> >> >> @@ -3277,9 +3277,9 @@
>> >> >> >>  ##
>> >> >> >>  # @ChardevRingbuf:
>> >> >> >>  #
>> >> >> >> -# Configuration info for memory chardevs
>> >> >> >> +# Configuration info for ring buffer chardevs.
>> >> >> >>  #
>> >> >> >> -# @size: #optional Ringbuffer size, must be power of two, default 
>> >> >> >> is 65536
>> >> >> >> +# @size: #optional ring buffer size, must be power of two, default 
>> >> >> >> is 65536
>> >> >> >>  #
>> >> >> >>  # Since: 1.5
>> >> >> >>  ##
>> >> >> >> @@ -3310,6 +3310,8 @@
>> >> >> >>                                         'spicevmc' : 
>> >> >> >> 'ChardevSpiceChannel',
>> >> >> >>                                         'spiceport' : 
>> >> >> >> 'ChardevSpicePort',
>> >> >> >>                                         'vc'     : 'ChardevVC',
>> >> >> >> +                                       'ringbuf': 'ChardevRingbuf',
>> >> >> >> +                                       # next one is just for 
>> >> >> >> compatibility
>> >> >> >>                                         'memory' : 'ChardevRingbuf' 
>> >> >> >> } }
>> >> >> >
>> >> >> > Does JSON allow comments in the middle of content?  Is this going to
>> >> >> > screw up Amos' work on introspection?  You may need to instead have a
>> >> >> > comment before the open '{' stating that 'memory' is an
>> >> >> > alias within the
>> >> >> > union for back-compat reasons.
>> >> >
>> >> > I didn't parse the json file by myself. I just used the parsed
>> >> > dictionary. So it only needs to make qapi.py happy.
>> >> >
>> >> >> RFC 4627 doesn't do comments at all.
>> >> >> 
>> >> >> This file is parsed by scripts/qapi.py, which as far as I can tell
>> >> >> ignores lines starting with '#' anywhere in the input.
>> >> >
>> >> > Not anywhere, only start with '#'
>> >> 
>> >> Isn't that what I said?
>> >> 
>> >> > | def parse_schema(fp):
>> >> > |     exprs = []
>> >> > |     raw_exprs = []
>> >> > |     expr = ''
>> >> > |     expr_eval = None
>> >> > | 
>> >> > |     for line in fp:
>> >> > |         if line.startswith('#') or line == '\n':
>> >> >
>> >> >       # ignores lines starting with '#' anywhere
>> >> >           if line.strip().startswith('#')   
>> >> >
>> >> > |             continue
>> >> > | 
>> >> >
>> >> >
>> >> > So we should not add this kind of comment for back-compat.
>> >> 
>> >> Now I'm confused.  My patch adds a line that starts with '#'.
>> >> parse_schema() ignores it.  Works as designed.  Why do you think we
>> >> shouldn't do that?
>> >
>> > The comment line in your patch doen't start with '#', it starts with
>> > blank-space. If we want qapi.py to process it, we need to do strip()
>> > first.
>> 
>> Aha.  I agree with your reading of parse_schema().  However, I get
>> exactly identical generated files with and without this comment.
>
> I tried to add a '#hello' prefix for 'serial' item, but 'serial' can still
> be parsed as before. The '#hello' prefix will be ignored in qapi.py:tokenize()
>
>   { 'union': 'ChardevBackend', 'data': { 'file'   : 'ChardevFile',
>                                          #hello 'serial' : 'ChardevHostdev',
>                                          'parallel': 'ChardevHostdev',

Tokenize silently ignores characters it doesn't recognize.  Pass the
crackpipe, pal!

[...]