[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 07/20] instrument: [qapi] Add library loader
From: |
Lluís Vilanova |
Subject: |
Re: [Qemu-devel] [PATCH v4 07/20] instrument: [qapi] Add library loader |
Date: |
Mon, 11 Sep 2017 00:39:05 +0300 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux) |
Markus Armbruster writes:
> I missed prior versions of this series. Please cc: qapi-schema
> maintainers on all non-trivial schema patches.
> scripts/get_maintainer.pl points to them for this patch.
> Marc-André, semantic conflict with your QAPI conditionals series. Just
> a heads-up, there's nothing for you to do about it right now.
> Lluís Vilanova <address@hidden> writes:
[...]
>> diff --git a/instrument/load.h b/instrument/load.h
>> index 2ddb2c6c19..f8a02e6849 100644
>> --- a/instrument/load.h
>> +++ b/instrument/load.h
>> @@ -25,6 +25,8 @@
>> * @INSTR_LOAD_DLERROR: Error with libdl (see dlerror).
>> *
>> * Error codes for instr_load().
>> + *
>> + * NOTE: Keep in sync with QAPI's #InstrLoadCode.
>> */
>> typedef enum {
>> INSTR_LOAD_OK,
>> @@ -40,6 +42,8 @@ typedef enum {
>> * @INSTR_UNLOAD_DLERROR: Error with libdl (see dlerror).
>> *
>> * Error codes for instr_unload().
>> + *
>> + * NOTE: Keep in sync with QAPI's #InstrUnloadCode.
>> */
>> typedef enum {
>> INSTR_UNLOAD_OK,
> Any particular reason why you can't simply use the QAPI-generated enum
> types?
Silly me, just missed that "optimization" :) But after reading your later
comments, I'll keep these and remove the QAPI-generated enums.
[...]
>> diff --git a/instrument/qmp.c b/instrument/qmp.c
>> new file mode 100644
>> index 0000000000..c36960c12f
>> --- /dev/null
>> +++ b/instrument/qmp.c
>> @@ -0,0 +1,88 @@
>> +/*
>> + * QMP interface for instrumentation control commands.
>> + *
>> + * Copyright (C) 2012-2017 Lluís Vilanova <address@hidden>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qemu-common.h"
>> +#include "qapi/qmp/qerror.h"
>> +#include "qmp-commands.h"
>> +
>> +#include <dlfcn.h>
> System headers go right after "qemu/osdep.h".
>> +
>> +#include "instrument/load.h"
>> +
>> +
>> +
> Fewer blank lines would do.
>> +InstrLoadResult *qmp_instr_load(const char * path,
>> + bool have_args, strList * args,
> checkpatch ERROR: "foo * bar" should be "foo *bar"
> Please feed it all your patches, and carefully consider which of its
> complaints you should address.
>> + Error **errp)
>> +{
>> + InstrLoadResult *res = g_malloc0(sizeof(*res));
>> +
>> +#if defined(CONFIG_INSTRUMENT)
>> + int argc = 0;
>> + const char **argv = NULL;
>> +
>> + strList *entry = have_args ? args : NULL;
>> + while (entry != NULL) {
>> + argv = realloc(argv, sizeof(*argv) * (argc + 1));
>> + argv[argc] = entry->value;
>> + argc++;
>> + entry = entry->next;
>> + }
>> +
>> + InstrLoadError code = instr_load(path, argc, argv, &res->handle);
>> + switch (code) {
>> + case INSTR_LOAD_OK:
>> + res->code = INSTR_LOAD_CODE_OK;
>> + res->has_handle = true;
>> + break;
>> + case INSTR_LOAD_TOO_MANY:
>> + res->code = INSTR_LOAD_CODE_TOO_MANY;
>> + break;
>> + case INSTR_LOAD_ERROR:
>> + res->code = INSTR_LOAD_CODE_ERROR;
>> + break;
>> + case INSTR_LOAD_DLERROR:
>> + res->has_msg = true;
>> + res->msg = dlerror();
>> + res->code = INSTR_LOAD_CODE_DLERROR;
>> + break;
>> + }
>> +#else
>> + res->code = INSTR_LOAD_CODE_UNAVAILABLE;
>> +#endif
>> +
>> + return res;
>> +}
>> +
>> +InstrUnloadResult *qmp_instr_unload(int64_t handle, Error **errp)
>> +{
>> + InstrUnloadResult *res = g_malloc0(sizeof(*res));
>> +
>> +#if defined(CONFIG_INSTRUMENT)
>> + InstrUnloadError code = instr_unload(handle);
>> + switch (code) {
>> + case INSTR_UNLOAD_OK:
>> + res->code = INSTR_UNLOAD_CODE_OK;
>> + break;
>> + case INSTR_UNLOAD_INVALID:
>> + res->code = INSTR_UNLOAD_CODE_INVALID;
>> + break;
>> + case INSTR_UNLOAD_DLERROR:
>> + res->has_msg = true;
>> + res->msg = dlerror();
>> + break;
>> + res->code = INSTR_UNLOAD_CODE_DLERROR;
>> + }
>> +#else
>> + res->code = INSTR_UNLOAD_CODE_UNAVAILABLE;
>> +#endif
>> +
>> + return res;
>> +}
> You're inventing your own "this feature isn't compiled in" protocol. We
> already got too many of them. Let's stick to one of the existing ones,
> namely the one that's got some visibility in introspection. Bonus:
> turns the semantic conflict with Marc-André's work into a textual
> conflict. Here's how:
> * Add your commands to qmp_unregister_commands_hack() in monitor.c,
> roughly like this:
> #ifndef CONFIG_INSTRUMENT
> qmp_unregister_command(&qmp_commands, "instr-load");
> qmp_unregister_command(&qmp_commands, "instr-unload");
> #endif
> * Compile qmp.c only when CONFIG_INSTRUMENT, just like the other .c
> files there, except for cmdline.c. Drop the ifdeffery there.
> * Add stubbed out command handlers to stubs/instrument.c, roughly like
> this:
> InstrLoadResult *qmp_instr_load(const char *path,
> bool have_args, strList *args,
> Error **errp)
> {
> error_setg(errp, QERR_UNSUPPORTED);
> return NULL;
> }
> Same for qmp_instr_unload().
> These stubs must exist for the link to succeed, but they won't be
> called. They'll go away when Marc-André's work lands.
> * In the next patch, make the HMP command conditional on
> CONFIG_INSTRUMENT, just like trace-file is conditional on
> CONFIG_TRACE_SIMPLE.
> Questions?
Crystal clear, applied.
> You're also inventing your own "command failed" protocol. I'll explain
> in my review of instrument.json.
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index 802ea53d00..5e343be9ff 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -90,6 +90,9 @@
>> # QAPI introspection
>> { 'include': 'qapi/introspect.json' }
>>
>> +# Instrumentation commands
>> +{ 'include': 'qapi/instrument.json' }
>> +
>> ##
>> # = QMP commands
>> ##
>> diff --git a/qapi/instrument.json b/qapi/instrument.json
>> new file mode 100644
>> index 0000000000..ea63fae309
>> --- /dev/null
>> +++ b/qapi/instrument.json
>> @@ -0,0 +1,96 @@
>> +# *-*- Mode: Python -*-*
>> +#
>> +# QAPI instrumentation control commands.
>> +#
>> +# Copyright (C) 2012-2017 Lluís Vilanova <address@hidden>
>> +#
>> +# This work is licensed under the terms of the GNU GPL, version 2 or later.
>> +# See the COPYING file in the top-level directory.
> Make the title a doc comment, like this:
> # *-*- Mode: Python -*-*
> #
> # Copyright (C) 2012-2017 Lluís Vilanova <address@hidden>
> #
> # This work is licensed under the terms of the GNU GPL, version 2 or later.
> # See the COPYING file in the top-level directory.
> ##
> # QAPI instrumentation control commands.
> ##
> The ## around the title make it go into generated
> docs/interop/qemu-qmp-ref.* documentation.
>> +
>> +##
>> +# @InstrLoadCode:
>> +#
>> +# Result code of an 'instr-load' command.
>> +#
>> +# @ok: Correctly loaded.
>> +# @too-many: Tried to load too many instrumentation libraries.
>> +# @error: The library's main() function returned a non-zero value.
>> +# @dlerror: Error with libdl (see 'msg').
>> +# @unavailable: Service not available.
>> +#
>> +# Since: 2.11
>> +##
>> +{ 'enum': 'InstrLoadCode',
>> + 'data': [ 'ok', 'too-many', 'error', 'dlerror', 'unavailable' ] }
>> +
>> +##
>> +# @InstrLoadResult:
>> +#
>> +# Result of an 'instr-load' command.
>> +#
>> +# @code: Result code.
>> +# @msg: Additional error message (for human consumption only; present only
>> in
>> +# case of error).
>> +# @handle: Instrumentation library identifier (present only if successful).
>> +#
>> +# Since: 2.11
>> +##
>> +{ 'struct': 'InstrLoadResult',
>> + 'data': { 'code': 'InstrLoadCode', '*msg': 'str', '*handle': 'int' } }
>> +
>> +##
>> +# @instr-load:
>> +#
>> +# Load an instrumentation library.
>> +#
>> +# @path: path to the dynamic instrumentation library
>> +# @args: arguments to the dynamic instrumentation library
>> +#
>> +# Since: 2.11
>> +##
>> +{ 'command': 'instr-load',
>> + 'data': { 'path': 'str', '*args': ['str'] },
>> + 'returns': 'InstrLoadResult' }
> No :)
> If the command fails, it must send an error response instead success
> response. Yours always sends a success response.
> As far as I can tell, instr-load returns a handle on success, always,
> and nothing else. Therefore:
> { 'struct': 'InstrLoadResult',
> 'data': { 'handle': 'int' } }
> On error, qmp_instr_load() should set a suitable error with error_setg()
> and return NULL.
> QAPI enum type InstrLoadCode goes away.
> On returning a handle: we commonly let the user specify an ID string
> instead. For instance, device_add doesn't return a handle you can pass
> to device_del. Instead, it takes a string-valued 'id' parameter. Other
> commands, such as device_del, can refer to the device by that ID. Is
> there any particular reason why you can't stick to this convention for
> instrumentation?
Done too :)
>> +
>> +
>> +##
>> +# @InstrUnloadCode:
>> +#
>> +# Result code of an 'instr-unload' command.
>> +#
>> +# @ok: Correctly unloaded.
>> +# @invalid: Invalid handle.
>> +# @dlerror: Error with libdl (see 'msg').
>> +# @unavailable: Service not available.
>> +#
>> +# Since: 2.11
>> +##
>> +{ 'enum': 'InstrUnloadCode',
>> + 'data': [ 'ok', 'invalid', 'dlerror', 'unavailable' ] }
>> +
>> +##
>> +# @InstrUnloadResult:
>> +#
>> +# Result of an 'instr-unload' command.
>> +#
>> +# @code: Result code.
>> +# @msg: Additional error message (for human consumption only; present only
>> in
>> +# case of error).
>> +#
>> +# Since: 2.11
>> +##
>> +{ 'struct': 'InstrUnloadResult',
>> + 'data': { 'code': 'InstrUnloadCode', '*msg': 'str' } }
>> +
>> +##
>> +# @instr-unload:
>> +#
>> +# Unload an instrumentation library.
>> +#
>> +# @handle: Instrumentation library identifier (see #InstrLoadResult).
>> +#
>> +# Since: 2.11
>> +##
>> +{ 'command': 'instr-unload',
>> + 'data': { 'handle': 'int' },
>> + 'returns': 'InstrUnloadResult' }
> Likewise. instr-unload returns nothing on success. Both QAPI types go
> away.
Thanks a lot!
Lluis
- [Qemu-devel] [PATCH v4 01/20] instrument: Add documentation, (continued)
- [Qemu-devel] [PATCH v4 01/20] instrument: Add documentation, Lluís Vilanova, 2017/09/06
- [Qemu-devel] [PATCH v4 02/20] instrument: Add configure-time flag, Lluís Vilanova, 2017/09/06
- [Qemu-devel] [PATCH v4 03/20] instrument: Add generic library loader, Lluís Vilanova, 2017/09/06
- [Qemu-devel] [PATCH v4 04/20] instrument: [linux-user] Add command line library loader, Lluís Vilanova, 2017/09/06
- [Qemu-devel] [PATCH v4 05/20] instrument: [bsd-user] Add command line library loader, Lluís Vilanova, 2017/09/06
- [Qemu-devel] [PATCH v4 06/20] instrument: [softmmu] Add command line library loader, Lluís Vilanova, 2017/09/06
- [Qemu-devel] [PATCH v4 07/20] instrument: [qapi] Add library loader, Lluís Vilanova, 2017/09/06
- [Qemu-devel] [PATCH v4 08/20] instrument: [hmp] Add library loader, Lluís Vilanova, 2017/09/06
- [Qemu-devel] [PATCH v4 09/20] instrument: Add basic control interface, Lluís Vilanova, 2017/09/06
- [Qemu-devel] [PATCH v4 10/20] instrument: Add support for tracing events, Lluís Vilanova, 2017/09/06
- [Qemu-devel] [PATCH v4 11/20] instrument: Track vCPUs, Lluís Vilanova, 2017/09/06