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

[Eric Blake]

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.

> + *
> + * 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.  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

signature.asc
Description: OpenPGP digital signature