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 <ak...@redhat.com>
> > ---
> > 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 <ak...@redhat.com>
> > + *
> > + * 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 <ebl...@redhat.com>
>
> --
> Eric Blake eblake redhat com +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
--
Amos.
