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.
> + *
> + * 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. 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
signature.asc
Description: OpenPGP digital signature
