Re: [Qemu-devel] [PATCH v2 1/2] qapi: change qapi to convert schema json

[Amos Kong]

On Fri, Jul 19, 2013 at 06:27:12AM -0600, Eric Blake wrote:
> On 07/16/2013 04:37 AM, Amos Kong wrote:
> > QMP schema is defined in a json file, it will be parsed by
> > qapi scripts and generate C files.
> > 
> > We want to return the schema information to management,
> > this patch converts the json file to a string table in a
> > C head file, then we can use the json content.
> > 
> > eg:
> >   const char *const qmp_schema_table[] = {
> >     "{ 'type': 'NameInfo', 'data': {'*name': 'str'} }",
> >     "{ 'command': 'query-name', 'returns': 'NameInfo' }",
> >     ...
> >   }
> > 
> > Signed-off-by: Amos Kong <address@hidden>
> > ---
> >  Makefile                 |  5 ++++-
> >  scripts/qapi-commands.py |  2 +-
> >  scripts/qapi-types.py    | 47 
> > ++++++++++++++++++++++++++++++++++++++++++++---
> >  scripts/qapi-visit.py    |  2 +-
> >  scripts/qapi.py          |  4 +++-
> >  5 files changed, 53 insertions(+), 7 deletions(-)
> 
> My python is weak, but I'll attempt a review anyway (how else do you
> learn a new language? :)
> 
> > @@ -223,6 +223,9 @@ $(SRC_PATH)/qapi-schema.json 
> > $(SRC_PATH)/scripts/qapi-visit.py $(qapi-py)
> >  qmp-commands.h qmp-marshal.c :\
> >  $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-commands.py 
> > $(qapi-py)
> >     $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py 
> > $(gen-out-type) -m -o "." < $<, "  GEN   $@")
> > +qmp-schema.h:\
> > +$(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-types.py $(qapi-py)
> > +   $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py 
> > $(gen-out-type) -o "." -i "$@" < $<, "  GEN   $@")
> 
> Copy-and-paste, but any reason there is so much space around GEN?
> 
> > -exprs = parse_schema(sys.stdin)
> > +exprs_all = parse_schema(sys.stdin)
> > +
> > +schema_table = """/* AUTOMATICALLY GENERATED, DO NOT MODIFY */
> 
> Is it worth stating what file this was generated from?  If I open a
> generated file to try and make an edit, I like to have it tell me what
> the real source file is.
> 
> > +
> > +/*
> > + * Schema json string table converted from qapi-schema.json
> 
> Well, this is close, but as we are asking you to do multiple
> qapi-schema.json files (one for qemu's QMP monitor, one for qemu-ga), a
> relative path to the file within the overall qemu.git might be nicer.

I will use another split file qga-schema.h for qemu-ga.
> 
> > + *
> > + * Copyright (c) 2013 Red Hat, Inc.
> 
> This copyright won't auto-update as years change.  Should it?
> 
> Then again, this is probably copy-and-paste from other files the
> generator already spits out, so cleanups to one generated header should
> probably done to all generated headers, and in a separate cleanup patch,
> if at all.
> 
> > + *
> > + * Authors:
> > + *  Amos Kong <address@hidden>
> > + *
> > + * This work is licensed under the terms of the GNU LGPL, version 2.1 or 
> > later.
> > + * See the COPYING.LIB file in the top-level directory.
> > + *
> > + */
> > +
> > +const char *const qmp_schema_table[] = {
> > +"""
> > +
> > +if introspect_file:
> > +    for line in exprs_all[1]:
> > +        line = re.sub(r'\n', ' ', line.strip())
> > +        line = re.sub(r' +', ' ', line)
> > +        schema_table += '  "%s",\n' % (line)
> 
> Do we ever need to worry about someone using { "command" ...} instead of
> the current style of { 'command' ...} in the qapi-schema.json file?  If
> they do, then you would be generating invalid C code here by not
> escaping the " properly.

We didn't allow to use " in qapi-schema.json, you can check 
scripts/qapi.py:tokenize().

>  Likewise, should you be asserting that there
> are no other problematic characters like backslash?  Ideally, we will
> never encounter such problems, but being robust might save some
> head-scratching if someone introduces a typo.
>
> I can live with what you have:
> Reviewed-by: Eric Blake <address@hidden>
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org

-- 
                        Amos.