Re: [Qemu-devel] [PATCH v4 07/20] instrument: [qapi] Add library loader

[Lluís Vilanova]

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