From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34546) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V09mH-0002M1-RX for qemu-devel@nongnu.org; Fri, 19 Jul 2013 08:27:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V09mF-0006lz-Kx for qemu-devel@nongnu.org; Fri, 19 Jul 2013 08:27:17 -0400 Received: from mx1.redhat.com ([209.132.183.28]:62115) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V09mF-0006lT-BU for qemu-devel@nongnu.org; Fri, 19 Jul 2013 08:27:15 -0400 Message-ID: <51E930A0.20500@redhat.com> Date: Fri, 19 Jul 2013 06:27:12 -0600 From: Eric Blake MIME-Version: 1.0 References: <1373971062-28909-1-git-send-email-akong@redhat.com> <1373971062-28909-2-git-send-email-akong@redhat.com> In-Reply-To: <1373971062-28909-2-git-send-email-akong@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="IJRLhHDISa1A83KV7MdQU0pChEXmt38wI" Subject: Re: [Qemu-devel] [PATCH v2 1/2] qapi: change qapi to convert schema json List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Amos Kong Cc: pbonzini@redhat.com, aliguori@us.ibm.com, lcapitulino@redhat.com, qemu-devel@nongnu.org, armbru@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --IJRLhHDISa1A83KV7MdQU0pChEXmt38wI Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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. >=20 > 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. >=20 > eg: > const char *const qmp_schema_table[] =3D { > "{ 'type': 'NameInfo', 'data': {'*name': 'str'} }", > "{ 'command': 'query-name', 'returns': 'NameInfo' }", > ... > } >=20 > Signed-off-by: Amos Kong > --- > 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/qa= pi-visit.py $(qapi-py) > qmp-commands.h qmp-marshal.c :\ > $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-commands.py $(qa= pi-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 $(ge= n-out-type) -o "." -i "$@" < $<, " GEN $@") Copy-and-paste, but any reason there is so much space around GEN? > -exprs =3D parse_schema(sys.stdin) > +exprs_all =3D parse_schema(sys.stdin) > + > +schema_table =3D """/* 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 > + * > + * 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[] =3D { > +""" > + > +if introspect_file: > + for line in exprs_all[1]: > + line =3D re.sub(r'\n', ' ', line.strip()) > + line =3D re.sub(r' +', ' ', line) > + schema_table +=3D ' "%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 --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --IJRLhHDISa1A83KV7MdQU0pChEXmt38wI Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.13 (GNU/Linux) Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJR6TCgAAoJEKeha0olJ0NqUu4IAJmjLpiMnKW+y0XdawXCv9gc Dekbu9jPZJHHUxKu3FxhI+enJD6Iil50nqf10oSFufFmJZlihRcfjhPvgfXie+tH gOelfxc9DvWc4pOcFX8nQi/18oU6yoeg4JpDpo8kLVmSBgkfS+tMos/WdF4jpxmC +v4KDwFQjelPBaC6v+ji8FTgdoCMFw+VwJfjsAeckOM1f7hYesYE1yTDA+zbxEtX dj3R+84r1JLjeP4LbFj7aM8RkbfUO5xql7ss8EGy5TvgULjzREhSxQv0PUf+Y53L +XoQ7UQF/fQhGFeOoE235fEsgwk3MS2wmhbZpLcKarmP7hy0VXBo9W2DWctehjA= =r6Vt -----END PGP SIGNATURE----- --IJRLhHDISa1A83KV7MdQU0pChEXmt38wI--