[{"id":1764590,"web_url":"http://patchwork.ozlabs.org/comment/1764590/","msgid":"<877exalwzf.fsf@dusky.pond.sub.org>","list_archive_url":null,"date":"2017-09-07T08:43:48","subject":"Re: [Qemu-devel] [PATCH v4 07/20] instrument: [qapi] Add library\n\tloader","submitter":{"id":2645,"url":"http://patchwork.ozlabs.org/api/people/2645/","name":"Markus Armbruster","email":"armbru@redhat.com"},"content":"I missed prior versions of this series. Please cc: qapi-schema\nmaintainers on all non-trivial schema patches.\nscripts/get_maintainer.pl points to them for this patch.\n\nMarc-André, semantic conflict with your QAPI conditionals series. Just\na heads-up, there's nothing for you to do about it right now.\n\nLluís Vilanova writes:\n\n> Signed-off-by: Lluís Vilanova \n> ---\n> instrument/Makefile.objs | 1 \n> instrument/load.h | 4 ++\n> instrument/qmp.c | 88 ++++++++++++++++++++++++++++++++++++++++++\n> qapi-schema.json | 3 +\n> qapi/instrument.json | 96 ++++++++++++++++++++++++++++++++++++++++++++++\n> 5 files changed, 192 insertions(+)\n> create mode 100644 instrument/qmp.c\n> create mode 100644 qapi/instrument.json\n\nMissing: update of MAINTAINERS for qapi/instrument.json.\n\n> diff --git a/instrument/Makefile.objs b/instrument/Makefile.objs\n> index 5ea5c77245..13a8f60431 100644\n> --- a/instrument/Makefile.objs\n> +++ b/instrument/Makefile.objs\n> @@ -2,3 +2,4 @@\n> \n> target-obj-y += cmdline.o\n> target-obj-$(CONFIG_INSTRUMENT) += load.o\n> +target-obj-y += qmp.o\n> diff --git a/instrument/load.h b/instrument/load.h\n> index 2ddb2c6c19..f8a02e6849 100644\n> --- a/instrument/load.h\n> +++ b/instrument/load.h\n> @@ -25,6 +25,8 @@\n> * @INSTR_LOAD_DLERROR: Error with libdl (see dlerror).\n> *\n> * Error codes for instr_load().\n> + *\n> + * NOTE: Keep in sync with QAPI's #InstrLoadCode.\n> */\n> typedef enum {\n> INSTR_LOAD_OK,\n> @@ -40,6 +42,8 @@ typedef enum {\n> * @INSTR_UNLOAD_DLERROR: Error with libdl (see dlerror).\n> *\n> * Error codes for instr_unload().\n> + *\n> + * NOTE: Keep in sync with QAPI's #InstrUnloadCode.\n> */\n> typedef enum {\n> INSTR_UNLOAD_OK,\n\nAny particular reason why you can't simply use the QAPI-generated enum\ntypes?\n\n> diff --git a/instrument/qmp.c b/instrument/qmp.c\n> new file mode 100644\n> index 0000000000..c36960c12f\n> --- /dev/null\n> +++ b/instrument/qmp.c\n> @@ -0,0 +1,88 @@\n> +/*\n> + * QMP interface for instrumentation control commands.\n> + *\n> + * Copyright (C) 2012-2017 Lluís Vilanova \n> + *\n> + * This work is licensed under the terms of the GNU GPL, version 2 or later.\n> + * See the COPYING file in the top-level directory.\n> + */\n> +\n> +#include \"qemu/osdep.h\"\n> +#include \"qemu-common.h\"\n> +#include \"qapi/qmp/qerror.h\"\n> +#include \"qmp-commands.h\"\n> +\n> +#include \n\nSystem headers go right after \"qemu/osdep.h\".\n\n> +\n> +#include \"instrument/load.h\"\n> +\n> +\n> +\n\nFewer blank lines would do.\n\n> +InstrLoadResult *qmp_instr_load(const char * path,\n> + bool have_args, strList * args,\n\ncheckpatch ERROR: \"foo * bar\" should be \"foo *bar\"\n\nPlease feed it all your patches, and carefully consider which of its\ncomplaints you should address.\n\n> + Error **errp)\n> +{\n> + InstrLoadResult *res = g_malloc0(sizeof(*res));\n> +\n> +#if defined(CONFIG_INSTRUMENT)\n> + int argc = 0;\n> + const char **argv = NULL;\n> +\n> + strList *entry = have_args ? args : NULL;\n> + while (entry != NULL) {\n> + argv = realloc(argv, sizeof(*argv) * (argc + 1));\n> + argv[argc] = entry->value;\n> + argc++;\n> + entry = entry->next;\n> + }\n> +\n> + InstrLoadError code = instr_load(path, argc, argv, &res->handle);\n> + switch (code) {\n> + case INSTR_LOAD_OK:\n> + res->code = INSTR_LOAD_CODE_OK;\n> + res->has_handle = true;\n> + break;\n> + case INSTR_LOAD_TOO_MANY:\n> + res->code = INSTR_LOAD_CODE_TOO_MANY;\n> + break;\n> + case INSTR_LOAD_ERROR:\n> + res->code = INSTR_LOAD_CODE_ERROR;\n> + break;\n> + case INSTR_LOAD_DLERROR:\n> + res->has_msg = true;\n> + res->msg = dlerror();\n> + res->code = INSTR_LOAD_CODE_DLERROR;\n> + break;\n> + }\n> +#else\n> + res->code = INSTR_LOAD_CODE_UNAVAILABLE;\n> +#endif\n> +\n> + return res;\n> +}\n> +\n> +InstrUnloadResult *qmp_instr_unload(int64_t handle, Error **errp)\n> +{\n> + InstrUnloadResult *res = g_malloc0(sizeof(*res));\n> +\n> +#if defined(CONFIG_INSTRUMENT)\n> + InstrUnloadError code = instr_unload(handle);\n> + switch (code) {\n> + case INSTR_UNLOAD_OK:\n> + res->code = INSTR_UNLOAD_CODE_OK;\n> + break;\n> + case INSTR_UNLOAD_INVALID:\n> + res->code = INSTR_UNLOAD_CODE_INVALID;\n> + break;\n> + case INSTR_UNLOAD_DLERROR:\n> + res->has_msg = true;\n> + res->msg = dlerror();\n> + break;\n> + res->code = INSTR_UNLOAD_CODE_DLERROR;\n> + }\n> +#else\n> + res->code = INSTR_UNLOAD_CODE_UNAVAILABLE;\n> +#endif\n> +\n> + return res;\n> +}\n\nYou're inventing your own \"this feature isn't compiled in\" protocol. We\nalready got too many of them. Let's stick to one of the existing ones,\nnamely the one that's got some visibility in introspection. Bonus:\nturns the semantic conflict with Marc-André's work into a textual\nconflict. Here's how:\n\n* Add your commands to qmp_unregister_commands_hack() in monitor.c,\n roughly like this:\n\n #ifndef CONFIG_INSTRUMENT\n qmp_unregister_command(&qmp_commands, \"instr-load\");\n qmp_unregister_command(&qmp_commands, \"instr-unload\");\n #endif\n\n* Compile qmp.c only when CONFIG_INSTRUMENT, just like the other .c\n files there, except for cmdline.c. Drop the ifdeffery there.\n\n* Add stubbed out command handlers to stubs/instrument.c, roughly like\n this:\n\n InstrLoadResult *qmp_instr_load(const char *path,\n bool have_args, strList *args,\n Error **errp)\n {\n error_setg(errp, QERR_UNSUPPORTED);\n return NULL;\n }\n\n Same for qmp_instr_unload().\n\n These stubs must exist for the link to succeed, but they won't be\n called. They'll go away when Marc-André's work lands.\n\n* In the next patch, make the HMP command conditional on\n CONFIG_INSTRUMENT, just like trace-file is conditional on\n CONFIG_TRACE_SIMPLE.\n\nQuestions?\n\nYou're also inventing your own \"command failed\" protocol. I'll explain\nin my review of instrument.json.\n\n> diff --git a/qapi-schema.json b/qapi-schema.json\n> index 802ea53d00..5e343be9ff 100644\n> --- a/qapi-schema.json\n> +++ b/qapi-schema.json\n> @@ -90,6 +90,9 @@\n> # QAPI introspection\n> { 'include': 'qapi/introspect.json' }\n> \n> +# Instrumentation commands\n> +{ 'include': 'qapi/instrument.json' }\n> +\n> ##\n> # = QMP commands\n> ##\n> diff --git a/qapi/instrument.json b/qapi/instrument.json\n> new file mode 100644\n> index 0000000000..ea63fae309\n> --- /dev/null\n> +++ b/qapi/instrument.json\n> @@ -0,0 +1,96 @@\n> +# *-*- Mode: Python -*-*\n> +#\n> +# QAPI instrumentation control commands.\n> +#\n> +# Copyright (C) 2012-2017 Lluís Vilanova \n> +#\n> +# This work is licensed under the terms of the GNU GPL, version 2 or later.\n> +# See the COPYING file in the top-level directory.\n\nMake the title a doc comment, like this:\n\n # *-*- Mode: Python -*-*\n #\n # Copyright (C) 2012-2017 Lluís Vilanova \n #\n # This work is licensed under the terms of the GNU GPL, version 2 or later.\n # See the COPYING file in the top-level directory.\n\n ##\n # QAPI instrumentation control commands.\n ##\n\nThe ## around the title make it go into generated\ndocs/interop/qemu-qmp-ref.* documentation.\n\n> +\n> +##\n> +# @InstrLoadCode:\n> +#\n> +# Result code of an 'instr-load' command.\n> +#\n> +# @ok: Correctly loaded.\n> +# @too-many: Tried to load too many instrumentation libraries.\n> +# @error: The library's main() function returned a non-zero value.\n> +# @dlerror: Error with libdl (see 'msg').\n> +# @unavailable: Service not available.\n> +#\n> +# Since: 2.11\n> +##\n> +{ 'enum': 'InstrLoadCode',\n> + 'data': [ 'ok', 'too-many', 'error', 'dlerror', 'unavailable' ] }\n> +\n> +##\n> +# @InstrLoadResult:\n> +#\n> +# Result of an 'instr-load' command.\n> +#\n> +# @code: Result code.\n> +# @msg: Additional error message (for human consumption only; present only in\n> +# case of error).\n> +# @handle: Instrumentation library identifier (present only if successful).\n> +#\n> +# Since: 2.11\n> +##\n> +{ 'struct': 'InstrLoadResult',\n> + 'data': { 'code': 'InstrLoadCode', '*msg': 'str', '*handle': 'int' } }\n> +\n> +##\n> +# @instr-load:\n> +#\n> +# Load an instrumentation library.\n> +#\n> +# @path: path to the dynamic instrumentation library\n> +# @args: arguments to the dynamic instrumentation library\n> +#\n> +# Since: 2.11\n> +##\n> +{ 'command': 'instr-load',\n> + 'data': { 'path': 'str', '*args': ['str'] },\n> + 'returns': 'InstrLoadResult' }\n\nNo :)\n\nIf the command fails, it must send an error response instead success\nresponse. Yours always sends a success response.\n\nAs far as I can tell, instr-load returns a handle on success, always,\nand nothing else. Therefore:\n\n { 'struct': 'InstrLoadResult',\n 'data': { 'handle': 'int' } }\n\nOn error, qmp_instr_load() should set a suitable error with error_setg()\nand return NULL.\n\nQAPI enum type InstrLoadCode goes away.\n\nOn returning a handle: we commonly let the user specify an ID string\ninstead. For instance, device_add doesn't return a handle you can pass\nto device_del. Instead, it takes a string-valued 'id' parameter. Other\ncommands, such as device_del, can refer to the device by that ID. Is\nthere any particular reason why you can't stick to this convention for\ninstrumentation?\n\n> +\n> +\n> +##\n> +# @InstrUnloadCode:\n> +#\n> +# Result code of an 'instr-unload' command.\n> +#\n> +# @ok: Correctly unloaded.\n> +# @invalid: Invalid handle.\n> +# @dlerror: Error with libdl (see 'msg').\n> +# @unavailable: Service not available.\n> +#\n> +# Since: 2.11\n> +##\n> +{ 'enum': 'InstrUnloadCode',\n> + 'data': [ 'ok', 'invalid', 'dlerror', 'unavailable' ] }\n> +\n> +##\n> +# @InstrUnloadResult:\n> +#\n> +# Result of an 'instr-unload' command.\n> +#\n> +# @code: Result code.\n> +# @msg: Additional error message (for human consumption only; present only in\n> +# case of error).\n> +#\n> +# Since: 2.11\n> +##\n> +{ 'struct': 'InstrUnloadResult',\n> + 'data': { 'code': 'InstrUnloadCode', '*msg': 'str' } }\n> +\n> +##\n> +# @instr-unload:\n> +#\n> +# Unload an instrumentation library.\n> +#\n> +# @handle: Instrumentation library identifier (see #InstrLoadResult).\n> +#\n> +# Since: 2.11\n> +##\n> +{ 'command': 'instr-unload',\n> + 'data': { 'handle': 'int' },\n> + 'returns': 'InstrUnloadResult' }\n\nLikewise. instr-unload returns nothing on success. Both QAPI types go\naway.","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ext-mx02.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx02.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=armbru@redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xnvCH4LJFz9sRV\n\tfor ;\n\tThu, 7 Sep 2017 18:44:43 +1000 (AEST)","from localhost ([::1]:39349 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dpsQT-0001QG-PY\n\tfor incoming@patchwork.ozlabs.org; Thu, 07 Sep 2017 04:44:41 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:42331)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dpsPw-0001KD-QU\n\tfor qemu-devel@nongnu.org; Thu, 07 Sep 2017 04:44:14 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dpsPu-0001O5-VE\n\tfor qemu-devel@nongnu.org; Thu, 07 Sep 2017 04:44:08 -0400","from mx1.redhat.com ([209.132.183.28]:37634)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from ) id 1dpsPu-0001No-MP\n\tfor qemu-devel@nongnu.org; Thu, 07 Sep 2017 04:44:06 -0400","from smtp.corp.redhat.com\n\t(int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id AAE61859F7;\n\tThu, 7 Sep 2017 08:44:05 +0000 (UTC)","from blackfin.pond.sub.org (ovpn-116-75.ams2.redhat.com\n\t[10.36.116.75])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id B4652544F5;\n\tThu, 7 Sep 2017 08:43:51 +0000 (UTC)","by blackfin.pond.sub.org (Postfix, from userid 1000)\n\tid A76A91138645; Thu, 7 Sep 2017 10:43:48 +0200 (CEST)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com AAE61859F7","From":"Markus Armbruster ","To":"=?utf-8?b?TGx1w61z?= Vilanova ","References":"<150471856141.24907.274176769201097378.stgit@frigg.lan>\n\t<150472025751.24907.6133235993130047929.stgit@frigg.lan>","Date":"Thu, 07 Sep 2017 10:43:48 +0200","In-Reply-To":"<150472025751.24907.6133235993130047929.stgit@frigg.lan> (\n\t=?utf-8?b?IkxsdcOtcw==?= Vilanova\"'s message of \"Wed,\n\t6 Sep 2017 20:50:57 +0300\")","Message-ID":"<877exalwzf.fsf@dusky.pond.sub.org>","User-Agent":"Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux)","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.15","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.26]);\n\tThu, 07 Sep 2017 08:44:05 +0000 (UTC)","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","Subject":"Re: [Qemu-devel] [PATCH v4 07/20] instrument: [qapi] Add library\n\tloader","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"=?utf-8?q?Marc-Andr=C3=A9_Lureau?= ,\n\t\"Emilio G. Cota\" , qemu-devel@nongnu.org, \n\tStefan Hajnoczi ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1766011,"web_url":"http://patchwork.ozlabs.org/comment/1766011/","msgid":"<87tw0axmh2.fsf@frigg.lan>","list_archive_url":null,"date":"2017-09-10T21:39:05","subject":"Re: [Qemu-devel] [PATCH v4 07/20] instrument: [qapi] Add library\n\tloader","submitter":{"id":9099,"url":"http://patchwork.ozlabs.org/api/people/9099/","name":"Lluís Vilanova","email":"vilanova@ac.upc.edu"},"content":"Markus Armbruster writes:\n\n> I missed prior versions of this series. Please cc: qapi-schema\n> maintainers on all non-trivial schema patches.\n> scripts/get_maintainer.pl points to them for this patch.\n\n> Marc-André, semantic conflict with your QAPI conditionals series. Just\n> a heads-up, there's nothing for you to do about it right now.\n\n> Lluís Vilanova writes:\n\n[...]\n>> diff --git a/instrument/load.h b/instrument/load.h\n>> index 2ddb2c6c19..f8a02e6849 100644\n>> --- a/instrument/load.h\n>> +++ b/instrument/load.h\n>> @@ -25,6 +25,8 @@\n>> * @INSTR_LOAD_DLERROR: Error with libdl (see dlerror).\n>> *\n>> * Error codes for instr_load().\n>> + *\n>> + * NOTE: Keep in sync with QAPI's #InstrLoadCode.\n>> */\n>> typedef enum {\n>> INSTR_LOAD_OK,\n>> @@ -40,6 +42,8 @@ typedef enum {\n>> * @INSTR_UNLOAD_DLERROR: Error with libdl (see dlerror).\n>> *\n>> * Error codes for instr_unload().\n>> + *\n>> + * NOTE: Keep in sync with QAPI's #InstrUnloadCode.\n>> */\n>> typedef enum {\n>> INSTR_UNLOAD_OK,\n\n> Any particular reason why you can't simply use the QAPI-generated enum\n> types?\n\nSilly me, just missed that \"optimization\" :) But after reading your later\ncomments, I'll keep these and remove the QAPI-generated enums.\n\n\n[...]\n>> diff --git a/instrument/qmp.c b/instrument/qmp.c\n>> new file mode 100644\n>> index 0000000000..c36960c12f\n>> --- /dev/null\n>> +++ b/instrument/qmp.c\n>> @@ -0,0 +1,88 @@\n>> +/*\n>> + * QMP interface for instrumentation control commands.\n>> + *\n>> + * Copyright (C) 2012-2017 Lluís Vilanova \n>> + *\n>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.\n>> + * See the COPYING file in the top-level directory.\n>> + */\n>> +\n>> +#include \"qemu/osdep.h\"\n>> +#include \"qemu-common.h\"\n>> +#include \"qapi/qmp/qerror.h\"\n>> +#include \"qmp-commands.h\"\n>> +\n>> +#include \n\n> System headers go right after \"qemu/osdep.h\".\n\n>> +\n>> +#include \"instrument/load.h\"\n>> +\n>> +\n>> +\n\n> Fewer blank lines would do.\n\n>> +InstrLoadResult *qmp_instr_load(const char * path,\n>> + bool have_args, strList * args,\n\n> checkpatch ERROR: \"foo * bar\" should be \"foo *bar\"\n\n> Please feed it all your patches, and carefully consider which of its\n> complaints you should address.\n\n>> + Error **errp)\n>> +{\n>> + InstrLoadResult *res = g_malloc0(sizeof(*res));\n>> +\n>> +#if defined(CONFIG_INSTRUMENT)\n>> + int argc = 0;\n>> + const char **argv = NULL;\n>> +\n>> + strList *entry = have_args ? args : NULL;\n>> + while (entry != NULL) {\n>> + argv = realloc(argv, sizeof(*argv) * (argc + 1));\n>> + argv[argc] = entry->value;\n>> + argc++;\n>> + entry = entry->next;\n>> + }\n>> +\n>> + InstrLoadError code = instr_load(path, argc, argv, &res->handle);\n>> + switch (code) {\n>> + case INSTR_LOAD_OK:\n>> + res->code = INSTR_LOAD_CODE_OK;\n>> + res->has_handle = true;\n>> + break;\n>> + case INSTR_LOAD_TOO_MANY:\n>> + res->code = INSTR_LOAD_CODE_TOO_MANY;\n>> + break;\n>> + case INSTR_LOAD_ERROR:\n>> + res->code = INSTR_LOAD_CODE_ERROR;\n>> + break;\n>> + case INSTR_LOAD_DLERROR:\n>> + res->has_msg = true;\n>> + res->msg = dlerror();\n>> + res->code = INSTR_LOAD_CODE_DLERROR;\n>> + break;\n>> + }\n>> +#else\n>> + res->code = INSTR_LOAD_CODE_UNAVAILABLE;\n>> +#endif\n>> +\n>> + return res;\n>> +}\n>> +\n>> +InstrUnloadResult *qmp_instr_unload(int64_t handle, Error **errp)\n>> +{\n>> + InstrUnloadResult *res = g_malloc0(sizeof(*res));\n>> +\n>> +#if defined(CONFIG_INSTRUMENT)\n>> + InstrUnloadError code = instr_unload(handle);\n>> + switch (code) {\n>> + case INSTR_UNLOAD_OK:\n>> + res->code = INSTR_UNLOAD_CODE_OK;\n>> + break;\n>> + case INSTR_UNLOAD_INVALID:\n>> + res->code = INSTR_UNLOAD_CODE_INVALID;\n>> + break;\n>> + case INSTR_UNLOAD_DLERROR:\n>> + res->has_msg = true;\n>> + res->msg = dlerror();\n>> + break;\n>> + res->code = INSTR_UNLOAD_CODE_DLERROR;\n>> + }\n>> +#else\n>> + res->code = INSTR_UNLOAD_CODE_UNAVAILABLE;\n>> +#endif\n>> +\n>> + return res;\n>> +}\n\n> You're inventing your own \"this feature isn't compiled in\" protocol. We\n> already got too many of them. Let's stick to one of the existing ones,\n> namely the one that's got some visibility in introspection. Bonus:\n> turns the semantic conflict with Marc-André's work into a textual\n> conflict. Here's how:\n\n> * Add your commands to qmp_unregister_commands_hack() in monitor.c,\n> roughly like this:\n\n> #ifndef CONFIG_INSTRUMENT\n> qmp_unregister_command(&qmp_commands, \"instr-load\");\n> qmp_unregister_command(&qmp_commands, \"instr-unload\");\n> #endif\n\n> * Compile qmp.c only when CONFIG_INSTRUMENT, just like the other .c\n> files there, except for cmdline.c. Drop the ifdeffery there.\n\n> * Add stubbed out command handlers to stubs/instrument.c, roughly like\n> this:\n\n> InstrLoadResult *qmp_instr_load(const char *path,\n> bool have_args, strList *args,\n> Error **errp)\n> {\n> error_setg(errp, QERR_UNSUPPORTED);\n> return NULL;\n> }\n\n> Same for qmp_instr_unload().\n\n> These stubs must exist for the link to succeed, but they won't be\n> called. They'll go away when Marc-André's work lands.\n\n> * In the next patch, make the HMP command conditional on\n> CONFIG_INSTRUMENT, just like trace-file is conditional on\n> CONFIG_TRACE_SIMPLE.\n\n> Questions?\n\nCrystal clear, applied.\n\n\n> You're also inventing your own \"command failed\" protocol. I'll explain\n> in my review of instrument.json.\n\n>> diff --git a/qapi-schema.json b/qapi-schema.json\n>> index 802ea53d00..5e343be9ff 100644\n>> --- a/qapi-schema.json\n>> +++ b/qapi-schema.json\n>> @@ -90,6 +90,9 @@\n>> # QAPI introspection\n>> { 'include': 'qapi/introspect.json' }\n>> \n>> +# Instrumentation commands\n>> +{ 'include': 'qapi/instrument.json' }\n>> +\n>> ##\n>> # = QMP commands\n>> ##\n>> diff --git a/qapi/instrument.json b/qapi/instrument.json\n>> new file mode 100644\n>> index 0000000000..ea63fae309\n>> --- /dev/null\n>> +++ b/qapi/instrument.json\n>> @@ -0,0 +1,96 @@\n>> +# *-*- Mode: Python -*-*\n>> +#\n>> +# QAPI instrumentation control commands.\n>> +#\n>> +# Copyright (C) 2012-2017 Lluís Vilanova \n>> +#\n>> +# This work is licensed under the terms of the GNU GPL, version 2 or later.\n>> +# See the COPYING file in the top-level directory.\n\n> Make the title a doc comment, like this:\n\n> # *-*- Mode: Python -*-*\n> #\n> # Copyright (C) 2012-2017 Lluís Vilanova \n> #\n> # This work is licensed under the terms of the GNU GPL, version 2 or later.\n> # See the COPYING file in the top-level directory.\n\n> ##\n> # QAPI instrumentation control commands.\n> ##\n\n> The ## around the title make it go into generated\n> docs/interop/qemu-qmp-ref.* documentation.\n\n>> +\n>> +##\n>> +# @InstrLoadCode:\n>> +#\n>> +# Result code of an 'instr-load' command.\n>> +#\n>> +# @ok: Correctly loaded.\n>> +# @too-many: Tried to load too many instrumentation libraries.\n>> +# @error: The library's main() function returned a non-zero value.\n>> +# @dlerror: Error with libdl (see 'msg').\n>> +# @unavailable: Service not available.\n>> +#\n>> +# Since: 2.11\n>> +##\n>> +{ 'enum': 'InstrLoadCode',\n>> + 'data': [ 'ok', 'too-many', 'error', 'dlerror', 'unavailable' ] }\n>> +\n>> +##\n>> +# @InstrLoadResult:\n>> +#\n>> +# Result of an 'instr-load' command.\n>> +#\n>> +# @code: Result code.\n>> +# @msg: Additional error message (for human consumption only; present only in\n>> +# case of error).\n>> +# @handle: Instrumentation library identifier (present only if successful).\n>> +#\n>> +# Since: 2.11\n>> +##\n>> +{ 'struct': 'InstrLoadResult',\n>> + 'data': { 'code': 'InstrLoadCode', '*msg': 'str', '*handle': 'int' } }\n>> +\n>> +##\n>> +# @instr-load:\n>> +#\n>> +# Load an instrumentation library.\n>> +#\n>> +# @path: path to the dynamic instrumentation library\n>> +# @args: arguments to the dynamic instrumentation library\n>> +#\n>> +# Since: 2.11\n>> +##\n>> +{ 'command': 'instr-load',\n>> + 'data': { 'path': 'str', '*args': ['str'] },\n>> + 'returns': 'InstrLoadResult' }\n\n> No :)\n\n> If the command fails, it must send an error response instead success\n> response. Yours always sends a success response.\n\n> As far as I can tell, instr-load returns a handle on success, always,\n> and nothing else. Therefore:\n\n> { 'struct': 'InstrLoadResult',\n> 'data': { 'handle': 'int' } }\n\n> On error, qmp_instr_load() should set a suitable error with error_setg()\n> and return NULL.\n\n> QAPI enum type InstrLoadCode goes away.\n\n> On returning a handle: we commonly let the user specify an ID string\n> instead. For instance, device_add doesn't return a handle you can pass\n> to device_del. Instead, it takes a string-valued 'id' parameter. Other\n> commands, such as device_del, can refer to the device by that ID. Is\n> there any particular reason why you can't stick to this convention for\n> instrumentation?\n\nDone too :)\n\n>> +\n>> +\n>> +##\n>> +# @InstrUnloadCode:\n>> +#\n>> +# Result code of an 'instr-unload' command.\n>> +#\n>> +# @ok: Correctly unloaded.\n>> +# @invalid: Invalid handle.\n>> +# @dlerror: Error with libdl (see 'msg').\n>> +# @unavailable: Service not available.\n>> +#\n>> +# Since: 2.11\n>> +##\n>> +{ 'enum': 'InstrUnloadCode',\n>> + 'data': [ 'ok', 'invalid', 'dlerror', 'unavailable' ] }\n>> +\n>> +##\n>> +# @InstrUnloadResult:\n>> +#\n>> +# Result of an 'instr-unload' command.\n>> +#\n>> +# @code: Result code.\n>> +# @msg: Additional error message (for human consumption only; present only in\n>> +# case of error).\n>> +#\n>> +# Since: 2.11\n>> +##\n>> +{ 'struct': 'InstrUnloadResult',\n>> + 'data': { 'code': 'InstrUnloadCode', '*msg': 'str' } }\n>> +\n>> +##\n>> +# @instr-unload:\n>> +#\n>> +# Unload an instrumentation library.\n>> +#\n>> +# @handle: Instrumentation library identifier (see #InstrLoadResult).\n>> +#\n>> +# Since: 2.11\n>> +##\n>> +{ 'command': 'instr-unload',\n>> + 'data': { 'handle': 'int' },\n>> + 'returns': 'InstrUnloadResult' }\n\n> Likewise. instr-unload returns nothing on success. Both QAPI types go\n> away.\n\n\nThanks a lot!\nLluis","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xr4GH69nkz9s4s\n\tfor ;\n\tMon, 11 Sep 2017 07:39:49 +1000 (AEST)","from localhost ([::1]:54352 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dr9xB-0005uJ-6G\n\tfor incoming@patchwork.ozlabs.org; Sun, 10 Sep 2017 17:39:45 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:58083)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dr9wp-0005u7-P4\n\tfor qemu-devel@nongnu.org; Sun, 10 Sep 2017 17:39:25 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dr9wm-0007T1-Hf\n\tfor qemu-devel@nongnu.org; Sun, 10 Sep 2017 17:39:23 -0400","from roura.ac.upc.edu ([147.83.33.10]:42354 helo=roura.ac.upc.es)\n\tby eggs.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dr9wm-0007SB-2e\n\tfor qemu-devel@nongnu.org; Sun, 10 Sep 2017 17:39:20 -0400","from correu-2.ac.upc.es (correu-2.ac.upc.es [147.83.30.92])\n\tby roura.ac.upc.es (8.13.8/8.13.8) with ESMTP id v8ALdC9F025323;\n\tSun, 10 Sep 2017 23:39:12 +0200","from localhost (unknown [31.210.187.58])\n\tby correu-2.ac.upc.es (Postfix) with ESMTPSA id 1E93C17AB;\n\tSun, 10 Sep 2017 23:39:07 +0200 (CEST)"],"From":"=?utf-8?q?Llu=C3=ADs_Vilanova?= ","To":"Markus Armbruster ","References":"<150471856141.24907.274176769201097378.stgit@frigg.lan>\n\t<150472025751.24907.6133235993130047929.stgit@frigg.lan>\n\t<877exalwzf.fsf@dusky.pond.sub.org>","Mail-Followup-To":"Markus Armbruster ,\n\tqemu-devel@nongnu.org, \"Emilio G. Cota\" ,\n\tStefan Hajnoczi ,\n\tEric Blake , =?utf-8?q?Marc-Andr?=\n\t=?utf-8?b?w6k=?= Lureau ","Date":"Mon, 11 Sep 2017 00:39:05 +0300","In-Reply-To":"<877exalwzf.fsf@dusky.pond.sub.org> (Markus Armbruster's message\n\tof \"Thu, 07 Sep 2017 10:43:48 +0200\")","Message-ID":"<87tw0axmh2.fsf@frigg.lan>","User-Agent":"Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux)","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.6.x [fuzzy]","X-Received-From":"147.83.33.10","Subject":"Re: [Qemu-devel] [PATCH v4 07/20] instrument: [qapi] Add library\n\tloader","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"=?utf-8?q?Marc-Andr=C3=A9?= Lureau ,\n\t\"Emilio G. Cota\" , qemu-devel@nongnu.org, \n\tStefan Hajnoczi ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}}]