From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45678) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dpsXc-0004vq-Mc for qemu-devel@nongnu.org; Thu, 07 Sep 2017 04:52:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dpsXX-0005Kf-P4 for qemu-devel@nongnu.org; Thu, 07 Sep 2017 04:52:04 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46206) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1dpsXX-0005KF-Fj for qemu-devel@nongnu.org; Thu, 07 Sep 2017 04:51:59 -0400 From: Markus Armbruster References: <150471856141.24907.274176769201097378.stgit@frigg.lan> <150472050004.24907.4613952373869620144.stgit@frigg.lan> Date: Thu, 07 Sep 2017 10:51:54 +0200 In-Reply-To: <150472050004.24907.4613952373869620144.stgit@frigg.lan> (=?utf-8?Q?=22Llu=C3=ADs?= Vilanova"'s message of "Wed, 6 Sep 2017 20:55:00 +0300") Message-ID: <87vakuki1h.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v4 08/20] instrument: [hmp] Add library loader List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?utf-8?Q?Llu=C3=ADs?= Vilanova Cc: qemu-devel@nongnu.org, "Emilio G. Cota" , "Dr. David Alan Gilbert" , Stefan Hajnoczi , Eric Blake Llu=C3=ADs Vilanova writes: > Signed-off-by: Llu=C3=ADs Vilanova > --- > hmp-commands.hx | 28 ++++++++++++++++++++++++++ > monitor.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++= ++++++ > 2 files changed, 88 insertions(+) > > diff --git a/hmp-commands.hx b/hmp-commands.hx > index 1941e19932..703d7262f5 100644 > --- a/hmp-commands.hx > +++ b/hmp-commands.hx > @@ -1858,6 +1858,34 @@ ETEXI > .sub_table =3D info_cmds, > }, >=20=20 > + { > + .name =3D "instr-load", > + .args_type =3D "path:F,args:s?", > + .params =3D "path [arg]", > + .help =3D "load an instrumentation library", > + .cmd =3D hmp_instr_load, > + }, > + > +STEXI > +@item instr-load @var{path} [args=3Dvalue[,...]] > +@findex instr-load > +Load an instrumentation library. > +ETEXI > + > + { > + .name =3D "instr-unload", > + .args_type =3D "handle:i", > + .params =3D "handle", > + .help =3D "unload an instrumentation library", > + .cmd =3D hmp_instr_unload, > + }, > + > +STEXI > +@item instr-unload > +@findex instr-unload > +Unload an instrumentation library. > +ETEXI > + > STEXI > @end table > ETEXI Want #ifdef CONFIG_INSTRUMENT, see my review of the previous patch. See also my remark there on returning handles vs. passing in IDs. > diff --git a/monitor.c b/monitor.c > index e0f880107f..8a7684f860 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -2319,6 +2319,66 @@ int monitor_fd_param(Monitor *mon, const char *fdn= ame, Error **errp) > return fd; > } >=20=20 > +static void hmp_instr_load(Monitor *mon, const QDict *qdict) > +{ > + const char *path =3D qdict_get_str(qdict, "path"); > + const char *str =3D qdict_get_try_str(qdict, "args"); > + strList args; Blank line between last declaration and first statement, please. > + args.value =3D (str =3D=3D NULL) ? NULL : (char *)str; What's wrong with args.value =3D (char *)str; ? > + args.next =3D NULL; > + InstrLoadResult *res =3D qmp_instr_load(path, args.value !=3D NULL, > + args.value !=3D NULL ? &args := NULL, > + NULL); > + switch (res->code) { > + case INSTR_LOAD_CODE_OK: > + monitor_printf(mon, "Handle: %"PRId64"\n", res->handle); > + monitor_printf(mon, "OK\n"); > + break; > + case INSTR_LOAD_CODE_TOO_MANY: > + monitor_printf(mon, "Too many instrumentation libraries already = loaded\n"); > + break; > + case INSTR_LOAD_CODE_ERROR: > + monitor_printf(mon, "Instrumentation library returned a non-zero= value during initialization"); > + break; > + case INSTR_LOAD_CODE_DLERROR: > + monitor_printf(mon, "Error loading library: %s\n", res->msg); > + break; > + case INSTR_LOAD_CODE_UNAVAILABLE: > + monitor_printf(mon, "Service not available\n"); > + break; > + default: > + fprintf(stderr, "Unknown instrumentation load code: %d", res->co= de); > + exit(1); Impossible conditions should be assertion failures, but it's a moot point because... > + break; > + } > + qapi_free_InstrLoadResult(res); > +} With qmp_instr_load() fixed to set an error on failure, this becomes something like InstrLoadResult *res =3D qmp_instr_load(path, args.value !=3D NULL, args.value !=3D NULL ? &args := NULL, &err); if (err) { error_report_err(err); } else { monitor_printf(mon, "Handle: %"PRId64"\n", res->handle); monitor_printf(mon, "OK\n"); } qapi_free_InstrLoadResult(res); > + > +static void hmp_instr_unload(Monitor *mon, const QDict *qdict) > +{ > + int64_t handle =3D qdict_get_int(qdict, "handle"); > + InstrUnloadResult *res =3D qmp_instr_unload(handle, NULL); > + switch (res->code) { > + case INSTR_UNLOAD_CODE_OK: > + monitor_printf(mon, "OK\n"); > + break; > + case INSTR_UNLOAD_CODE_INVALID: > + monitor_printf(mon, "Invalid handle\n"); > + break; > + case INSTR_UNLOAD_CODE_DLERROR: > + monitor_printf(mon, "Error unloading library: %s\n", res->msg); > + break; > + case INSTR_UNLOAD_CODE_UNAVAILABLE: > + monitor_printf(mon, "Service not available\n"); > + break; > + default: > + fprintf(stderr, "Unknown instrumentation unload code: %d", res->= code); > + exit(1); > + break; > + } > + qapi_free_InstrUnloadResult(res); > +} > + > /* Please update hmp-commands.hx when adding or changing commands */ > static mon_cmd_t info_cmds[] =3D { > #include "hmp-commands-info.h" Likewise.