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

[Markus Armbruster <armbru@redhat.com>]

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 <vilanova@ac.upc.edu> writes:

> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
> ---
>  instrument/Makefile.objs |    1 
>  instrument/load.h        |    4 ++
>  instrument/qmp.c         |   88 ++++++++++++++++++++++++++++++++++++++++++
>  qapi-schema.json         |    3 +
>  qapi/instrument.json     |   96 ++++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 192 insertions(+)
>  create mode 100644 instrument/qmp.c
>  create mode 100644 qapi/instrument.json

Missing: update of MAINTAINERS for qapi/instrument.json.

> diff --git a/instrument/Makefile.objs b/instrument/Makefile.objs
> index 5ea5c77245..13a8f60431 100644
> --- a/instrument/Makefile.objs
> +++ b/instrument/Makefile.objs
> @@ -2,3 +2,4 @@
>  
>  target-obj-y += cmdline.o
>  target-obj-$(CONFIG_INSTRUMENT) += load.o
> +target-obj-y += qmp.o
> 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?

> 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 <vilanova@ac.upc.edu>
> + *
> + * 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?

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 <vilanova@ac.upc.edu>
> +#
> +# 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 <vilanova@ac.upc.edu>
   #
   # 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?

> +
> +
> +##
> +# @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.