[{"id":1764594,"web_url":"http://patchwork.ozlabs.org/comment/1764594/","msgid":"<87vakuki1h.fsf@dusky.pond.sub.org>","list_archive_url":null,"date":"2017-09-07T08:51:54","subject":"Re: [Qemu-devel] [PATCH v4 08/20] instrument: [hmp] Add library\n\tloader","submitter":{"id":2645,"url":"http://patchwork.ozlabs.org/api/people/2645/","name":"Markus Armbruster","email":"armbru@redhat.com"},"content":"Lluís Vilanova writes:\n\n> Signed-off-by: Lluís Vilanova \n> ---\n> hmp-commands.hx | 28 ++++++++++++++++++++++++++\n> monitor.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 88 insertions(+)\n>\n> diff --git a/hmp-commands.hx b/hmp-commands.hx\n> index 1941e19932..703d7262f5 100644\n> --- a/hmp-commands.hx\n> +++ b/hmp-commands.hx\n> @@ -1858,6 +1858,34 @@ ETEXI\n> .sub_table = info_cmds,\n> },\n> \n> + {\n> + .name = \"instr-load\",\n> + .args_type = \"path:F,args:s?\",\n> + .params = \"path [arg]\",\n> + .help = \"load an instrumentation library\",\n> + .cmd = hmp_instr_load,\n> + },\n> +\n> +STEXI\n> +@item instr-load @var{path} [args=value[,...]]\n> +@findex instr-load\n> +Load an instrumentation library.\n> +ETEXI\n> +\n> + {\n> + .name = \"instr-unload\",\n> + .args_type = \"handle:i\",\n> + .params = \"handle\",\n> + .help = \"unload an instrumentation library\",\n> + .cmd = hmp_instr_unload,\n> + },\n> +\n> +STEXI\n> +@item instr-unload\n> +@findex instr-unload\n> +Unload an instrumentation library.\n> +ETEXI\n> +\n> STEXI\n> @end table\n> ETEXI\n\nWant #ifdef CONFIG_INSTRUMENT, see my review of the previous patch.\n\nSee also my remark there on returning handles vs. passing in IDs.\n\n> diff --git a/monitor.c b/monitor.c\n> index e0f880107f..8a7684f860 100644\n> --- a/monitor.c\n> +++ b/monitor.c\n> @@ -2319,6 +2319,66 @@ int monitor_fd_param(Monitor *mon, const char *fdname, Error **errp)\n> return fd;\n> }\n> \n> +static void hmp_instr_load(Monitor *mon, const QDict *qdict)\n> +{\n> + const char *path = qdict_get_str(qdict, \"path\");\n> + const char *str = qdict_get_try_str(qdict, \"args\");\n> + strList args;\n\nBlank line between last declaration and first statement, please.\n\n> + args.value = (str == NULL) ? NULL : (char *)str;\n\nWhat's wrong with\n\n args.value = (char *)str;\n\n?\n\n> + args.next = NULL;\n> + InstrLoadResult *res = qmp_instr_load(path, args.value != NULL,\n> + args.value != NULL ? &args : NULL,\n> + NULL);\n> + switch (res->code) {\n> + case INSTR_LOAD_CODE_OK:\n> + monitor_printf(mon, \"Handle: %\"PRId64\"\\n\", res->handle);\n> + monitor_printf(mon, \"OK\\n\");\n> + break;\n> + case INSTR_LOAD_CODE_TOO_MANY:\n> + monitor_printf(mon, \"Too many instrumentation libraries already loaded\\n\");\n> + break;\n> + case INSTR_LOAD_CODE_ERROR:\n> + monitor_printf(mon, \"Instrumentation library returned a non-zero value during initialization\");\n> + break;\n> + case INSTR_LOAD_CODE_DLERROR:\n> + monitor_printf(mon, \"Error loading library: %s\\n\", res->msg);\n> + break;\n> + case INSTR_LOAD_CODE_UNAVAILABLE:\n> + monitor_printf(mon, \"Service not available\\n\");\n> + break;\n> + default:\n> + fprintf(stderr, \"Unknown instrumentation load code: %d\", res->code);\n> + exit(1);\n\nImpossible conditions should be assertion failures, but it's a moot point\nbecause...\n\n> + break;\n> + }\n> + qapi_free_InstrLoadResult(res);\n> +}\n\nWith qmp_instr_load() fixed to set an error on failure, this becomes\nsomething like\n\n InstrLoadResult *res = qmp_instr_load(path, args.value != NULL,\n args.value != NULL ? &args : NULL,\n &err);\n if (err) {\n error_report_err(err);\n } else {\n monitor_printf(mon, \"Handle: %\"PRId64\"\\n\", res->handle);\n monitor_printf(mon, \"OK\\n\");\n }\n qapi_free_InstrLoadResult(res);\n\n> +\n> +static void hmp_instr_unload(Monitor *mon, const QDict *qdict)\n> +{\n> + int64_t handle = qdict_get_int(qdict, \"handle\");\n> + InstrUnloadResult *res = qmp_instr_unload(handle, NULL);\n> + switch (res->code) {\n> + case INSTR_UNLOAD_CODE_OK:\n> + monitor_printf(mon, \"OK\\n\");\n> + break;\n> + case INSTR_UNLOAD_CODE_INVALID:\n> + monitor_printf(mon, \"Invalid handle\\n\");\n> + break;\n> + case INSTR_UNLOAD_CODE_DLERROR:\n> + monitor_printf(mon, \"Error unloading library: %s\\n\", res->msg);\n> + break;\n> + case INSTR_UNLOAD_CODE_UNAVAILABLE:\n> + monitor_printf(mon, \"Service not available\\n\");\n> + break;\n> + default:\n> + fprintf(stderr, \"Unknown instrumentation unload code: %d\", res->code);\n> + exit(1);\n> + break;\n> + }\n> + qapi_free_InstrUnloadResult(res);\n> +}\n> +\n> /* Please update hmp-commands.hx when adding or changing commands */\n> static mon_cmd_t info_cmds[] = {\n> #include \"hmp-commands-info.h\"\n\nLikewise.","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-mx09.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx09.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 3xnvNN0Vpjz9s8J\n\tfor ;\n\tThu, 7 Sep 2017 18:52:34 +1000 (AEST)","from localhost ([::1]:39389 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dpsY0-0004xV-G3\n\tfor incoming@patchwork.ozlabs.org; Thu, 07 Sep 2017 04:52:28 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:45678)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dpsXc-0004vq-Mc\n\tfor qemu-devel@nongnu.org; Thu, 07 Sep 2017 04:52:09 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dpsXX-0005Kf-P4\n\tfor qemu-devel@nongnu.org; Thu, 07 Sep 2017 04:52:04 -0400","from mx1.redhat.com ([209.132.183.28]:46206)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from ) id 1dpsXX-0005KF-Fj\n\tfor qemu-devel@nongnu.org; Thu, 07 Sep 2017 04:51:59 -0400","from smtp.corp.redhat.com\n\t(int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13])\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 73F5B4E4C2;\n\tThu, 7 Sep 2017 08:51:58 +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 3933E6060A;\n\tThu, 7 Sep 2017 08:51:56 +0000 (UTC)","by blackfin.pond.sub.org (Postfix, from userid 1000)\n\tid 234C31138645; Thu, 7 Sep 2017 10:51:54 +0200 (CEST)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 73F5B4E4C2","From":"Markus Armbruster ","To":"=?utf-8?b?TGx1w61z?= Vilanova ","References":"<150471856141.24907.274176769201097378.stgit@frigg.lan>\n\t<150472050004.24907.4613952373869620144.stgit@frigg.lan>","Date":"Thu, 07 Sep 2017 10:51:54 +0200","In-Reply-To":"<150472050004.24907.4613952373869620144.stgit@frigg.lan> (\n\t=?utf-8?b?IkxsdcOtcw==?= Vilanova\"'s message of \"Wed,\n\t6 Sep 2017 20:55:00 +0300\")","Message-ID":"<87vakuki1h.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.13","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.38]);\n\tThu, 07 Sep 2017 08:51:58 +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 08/20] instrument: [hmp] 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":"\"Emilio G. Cota\" , qemu-devel@nongnu.org,\n\tStefan Hajnoczi ,\n\t\"Dr. David Alan Gilbert\" ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1766019,"web_url":"http://patchwork.ozlabs.org/comment/1766019/","msgid":"<87poayxl4w.fsf@frigg.lan>","list_archive_url":null,"date":"2017-09-10T22:07:59","subject":"Re: [Qemu-devel] [PATCH v4 08/20] instrument: [hmp] 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> Lluís Vilanova writes:\n>> Signed-off-by: Lluís Vilanova \n>> ---\n>> hmp-commands.hx | 28 ++++++++++++++++++++++++++\n>> monitor.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++++\n>> 2 files changed, 88 insertions(+)\n>> \n>> diff --git a/hmp-commands.hx b/hmp-commands.hx\n>> index 1941e19932..703d7262f5 100644\n>> --- a/hmp-commands.hx\n>> +++ b/hmp-commands.hx\n>> @@ -1858,6 +1858,34 @@ ETEXI\n>> .sub_table = info_cmds,\n>> },\n>> \n>> + {\n>> + .name = \"instr-load\",\n>> + .args_type = \"path:F,args:s?\",\n>> + .params = \"path [arg]\",\n>> + .help = \"load an instrumentation library\",\n>> + .cmd = hmp_instr_load,\n>> + },\n>> +\n>> +STEXI\n>> +@item instr-load @var{path} [args=value[,...]]\n>> +@findex instr-load\n>> +Load an instrumentation library.\n>> +ETEXI\n>> +\n>> + {\n>> + .name = \"instr-unload\",\n>> + .args_type = \"handle:i\",\n>> + .params = \"handle\",\n>> + .help = \"unload an instrumentation library\",\n>> + .cmd = hmp_instr_unload,\n>> + },\n>> +\n>> +STEXI\n>> +@item instr-unload\n>> +@findex instr-unload\n>> +Unload an instrumentation library.\n>> +ETEXI\n>> +\n>> STEXI\n>> @end table\n>> ETEXI\n\n> Want #ifdef CONFIG_INSTRUMENT, see my review of the previous patch.\n\n> See also my remark there on returning handles vs. passing in IDs.\n\n>> diff --git a/monitor.c b/monitor.c\n>> index e0f880107f..8a7684f860 100644\n>> --- a/monitor.c\n>> +++ b/monitor.c\n>> @@ -2319,6 +2319,66 @@ int monitor_fd_param(Monitor *mon, const char *fdname, Error **errp)\n>> return fd;\n>> }\n>> \n>> +static void hmp_instr_load(Monitor *mon, const QDict *qdict)\n>> +{\n>> + const char *path = qdict_get_str(qdict, \"path\");\n>> + const char *str = qdict_get_try_str(qdict, \"args\");\n>> + strList args;\n\n> Blank line between last declaration and first statement, please.\n\n>> + args.value = (str == NULL) ? NULL : (char *)str;\n\n> What's wrong with\n\n> args.value = (char *)str;\n\n> ?\n\n>> + args.next = NULL;\n>> + InstrLoadResult *res = qmp_instr_load(path, args.value != NULL,\n>> + args.value != NULL ? &args : NULL,\n>> + NULL);\n>> + switch (res->code) {\n>> + case INSTR_LOAD_CODE_OK:\n>> + monitor_printf(mon, \"Handle: %\"PRId64\"\\n\", res->handle);\n>> + monitor_printf(mon, \"OK\\n\");\n>> + break;\n>> + case INSTR_LOAD_CODE_TOO_MANY:\n>> + monitor_printf(mon, \"Too many instrumentation libraries already loaded\\n\");\n>> + break;\n>> + case INSTR_LOAD_CODE_ERROR:\n>> + monitor_printf(mon, \"Instrumentation library returned a non-zero value during initialization\");\n>> + break;\n>> + case INSTR_LOAD_CODE_DLERROR:\n>> + monitor_printf(mon, \"Error loading library: %s\\n\", res->msg);\n>> + break;\n>> + case INSTR_LOAD_CODE_UNAVAILABLE:\n>> + monitor_printf(mon, \"Service not available\\n\");\n>> + break;\n>> + default:\n>> + fprintf(stderr, \"Unknown instrumentation load code: %d\", res->code);\n>> + exit(1);\n\n> Impossible conditions should be assertion failures, but it's a moot point\n> because...\n\n>> + break;\n>> + }\n>> + qapi_free_InstrLoadResult(res);\n>> +}\n\n> With qmp_instr_load() fixed to set an error on failure, this becomes\n> something like\n\n> InstrLoadResult *res = qmp_instr_load(path, args.value != NULL,\n> args.value != NULL ? &args : NULL,\n> &err);\n> if (err) {\n> error_report_err(err);\n> } else {\n> monitor_printf(mon, \"Handle: %\"PRId64\"\\n\", res->handle);\n> monitor_printf(mon, \"OK\\n\");\n> }\n> qapi_free_InstrLoadResult(res);\n\n>> +\n>> +static void hmp_instr_unload(Monitor *mon, const QDict *qdict)\n>> +{\n>> + int64_t handle = qdict_get_int(qdict, \"handle\");\n>> + InstrUnloadResult *res = qmp_instr_unload(handle, NULL);\n>> + switch (res->code) {\n>> + case INSTR_UNLOAD_CODE_OK:\n>> + monitor_printf(mon, \"OK\\n\");\n>> + break;\n>> + case INSTR_UNLOAD_CODE_INVALID:\n>> + monitor_printf(mon, \"Invalid handle\\n\");\n>> + break;\n>> + case INSTR_UNLOAD_CODE_DLERROR:\n>> + monitor_printf(mon, \"Error unloading library: %s\\n\", res->msg);\n>> + break;\n>> + case INSTR_UNLOAD_CODE_UNAVAILABLE:\n>> + monitor_printf(mon, \"Service not available\\n\");\n>> + break;\n>> + default:\n>> + fprintf(stderr, \"Unknown instrumentation unload code: %d\", res->code);\n>> + exit(1);\n>> + break;\n>> + }\n>> + qapi_free_InstrUnloadResult(res);\n>> +}\n>> +\n>> /* Please update hmp-commands.hx when adding or changing commands */\n>> static mon_cmd_t info_cmds[] = {\n>> #include \"hmp-commands-info.h\"\n\n> Likewise.\n\nDone, thanks!\n\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 3xr4vc1x16z9sBd\n\tfor ;\n\tMon, 11 Sep 2017 08:08:41 +1000 (AEST)","from localhost ([::1]:54428 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1drAP9-0003hc-BS\n\tfor incoming@patchwork.ozlabs.org; Sun, 10 Sep 2017 18:08:39 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:35527)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1drAOm-0003hP-4V\n\tfor qemu-devel@nongnu.org; Sun, 10 Sep 2017 18:08:17 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1drAOi-0001Ll-3y\n\tfor qemu-devel@nongnu.org; Sun, 10 Sep 2017 18:08:16 -0400","from roura.ac.upc.edu ([147.83.33.10]:48948 helo=roura.ac.upc.es)\n\tby eggs.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1drAOh-0001If-LL\n\tfor qemu-devel@nongnu.org; Sun, 10 Sep 2017 18:08:12 -0400","from correu-1.ac.upc.es (correu-1.ac.upc.es [147.83.30.91])\n\tby roura.ac.upc.es (8.13.8/8.13.8) with ESMTP id v8AM85X9025837;\n\tMon, 11 Sep 2017 00:08:05 +0200","from localhost (unknown [31.210.187.58])\n\tby correu-1.ac.upc.es (Postfix) with ESMTPSA id 635C196A;\n\tMon, 11 Sep 2017 00:08:00 +0200 (CEST)"],"From":"=?utf-8?q?Llu=C3=ADs_Vilanova?= ","To":"Markus Armbruster ","References":"<150471856141.24907.274176769201097378.stgit@frigg.lan>\n\t<150472050004.24907.4613952373869620144.stgit@frigg.lan>\n\t<87vakuki1h.fsf@dusky.pond.sub.org>","Mail-Followup-To":"Markus Armbruster ,\n\tqemu-devel@nongnu.org, \"Emilio G. Cota\" , \"Dr. David\n\tAlan Gilbert\" , Stefan Hajnoczi\n\t, Eric Blake ","Date":"Mon, 11 Sep 2017 01:07:59 +0300","In-Reply-To":"<87vakuki1h.fsf@dusky.pond.sub.org> (Markus Armbruster's message\n\tof \"Thu, 07 Sep 2017 10:51:54 +0200\")","Message-ID":"<87poayxl4w.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 08/20] instrument: [hmp] 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":"\"Emilio G. Cota\" , qemu-devel@nongnu.org,\n\tStefan Hajnoczi ,\n\t\"Dr. David Alan Gilbert\" ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}}]