Patchwork [v2,15/23] instrument: [qmp, qapi] Add control interface

login
register
mail settings
Submitter Lluís Vilanova
Date April 16, 2013, 1:51 p.m.
Message ID <20130416135115.21588.60152.stgit@fimbulvetr.bsc.es>
Download mbox | patch
Permalink /patch/236986/
State New
Headers show

Comments

Lluís Vilanova - April 16, 2013, 1:51 p.m.
Add QMP commands to control (un)loading of dynamic instrumentation library.

Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
 include/qapi/qmp/qerror.h   |    9 +++++
 instrument/Makefile.objs    |    1 +
 instrument/qapi-schema.json |   33 ++++++++++++++++++++
 instrument/qmp.c            |   70 ++++++++++++++++++++++++++++++++++++++++++
 qapi-schema.json            |    2 +
 qmp-commands.hx             |   71 +++++++++++++++++++++++++++++++++++++++++++
 qmp.c                       |    4 ++
 7 files changed, 190 insertions(+)
 create mode 100644 instrument/qapi-schema.json
 create mode 100644 instrument/qmp.c
Eric Blake - April 19, 2013, 3:07 a.m.
On 04/16/2013 07:51 AM, Lluís Vilanova wrote:
> Add QMP commands to control (un)loading of dynamic instrumentation library.
> 
> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
> ---
>  include/qapi/qmp/qerror.h   |    9 +++++
>  instrument/Makefile.objs    |    1 +
>  instrument/qapi-schema.json |   33 ++++++++++++++++++++
>  instrument/qmp.c            |   70 ++++++++++++++++++++++++++++++++++++++++++
>  qapi-schema.json            |    2 +
>  qmp-commands.hx             |   71 +++++++++++++++++++++++++++++++++++++++++++
>  qmp.c                       |    4 ++
>  7 files changed, 190 insertions(+)
>  create mode 100644 instrument/qapi-schema.json
>  create mode 100644 instrument/qmp.c
> 
> diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
> index 6c0a18d..67b4528 100644
> --- a/include/qapi/qmp/qerror.h
> +++ b/include/qapi/qmp/qerror.h
> @@ -129,6 +129,15 @@ void assert_no_error(Error *err);
>  #define QERR_FEATURE_DISABLED \
>      ERROR_CLASS_GENERIC_ERROR, "The feature '%s' is not enabled"
>  
> +#define QERR_INSTR_LOAD_DL \
> +    ERROR_CLASS_GENERIC_ERROR, "Error loading dynamic library: %s"

These days, it is generally easier to just use error_setg() instead of
defining new QERR_ constants, unless you reuse an error so frequently
that having a QERR_ constant ensures that all use sites have consistent
wording of the message.

> +++ b/instrument/qapi-schema.json
> @@ -0,0 +1,33 @@
> +# *-*- Mode: Python -*-*
> +

New files should generally come with a copyright statement and clear
licensing terms.  Not entirely your fault since qapi-schema.json lacks a
copyright statement, but worth thinking about.

> +##
> +# @instr-dynamic:
> +#
> +# Whether dynamic trace instrumentation is available.
> +#
> +# Since: 1.5
> +##
> +{ 'command': 'instr-dynamic',
> +  'returns': 'bool' }

Perhaps naming this 'query-instr' is nicer, to match existing qemu
commands that can query state.  Then again, keeping all instrumentation
commands under the intsr- prefix makes them group together, while using
query-instr would not.

Is the abbreviation 'intsr-' appropriate, or should we be using the full
word 'instrument-'?

Is returning a bool going to make it harder to expand this command in
the future to return additional (optional) information, such as the name
of the current loaded library?  Is it worth returning a full-blown
dictionary type, so that we can add other name-value pairs into the
return as needed?  Is it reasonable to give instrumentation libraries a
callback hook that they can use to optionally generate additional
information to be returned as part of this query?

> +
> +##
> +# @instr-load:
> +#
> +# Load a dynamic instrumentation library.
> +#
> +# @path: path to the dynamic instrumentation library

Generally a blank line between arguments.

> +# @iargs: arguments to the dynamic instrumentation library

Any reason you named this 'iargs' instead of the simpler 'args'?

> +#
> +# Since: 1.5
> +##
> +{ 'command': 'instr-load',
> +  'data':    { 'path': 'str', 'iargs': ['String'] } }

There's another thread going on right now about fixing the generator to
let us use the much simpler ['str'] instead of having to go through the
extra layer of the wrapper type 'String'.

@iargs should be optional (#optional in the comment, listed as '*iargs'
in the JSON form); where omitting it is shorthand for [ ].

Should you return a handle representing the library that got loaded?  By
returning nothing, you have permanently limited this command to only
being able to load one library at a time.  But if we return a handle,
and make instr-unload take a handle argument, then even if we choose to
support only one library at a time now, later on, we will have the
option of extending things to support parallel libraries at once without
having to invent new commands.

> +
> +##
> +# @instr-unload:
> +#
> +# Unload the current dynamic instrumentation library.
> +#
> +# Since: 1.5
> +##
> +{ 'command': 'instr-unload' }

See above for comments about whether this should take an argument of the
handle returned from the load command.

> diff --git a/instrument/qmp.c b/instrument/qmp.c
> +++ b/qapi-schema.json
> @@ -3513,3 +3513,5 @@
>      '*asl_compiler_rev':  'uint32',
>      '*file':              'str',
>      '*data':              'str' }}
> +
> +input("instrument/qapi-schema.json")

See my comments on 13/23 about possibly naming this include() instead of
input().

> +Example:
> +
> +-> { "execute": "instr-load", "arguments": { "path": "/tmp/libtrace-instrument.so" } }

This example does not match the JSON listed above, because it omits the
mandatory 'iargs':[ ].

> +++ b/qmp.c
> @@ -24,6 +24,10 @@
>  #include "hw/qdev.h"
>  #include "sysemu/blockdev.h"
>  #include "qom/qom-qobject.h"
> +#if defined(TRACE_INSTRUMENT_DYNAMIC)

I don't know whether qemu coding style prefers to use #ifdef foo instead
of #if defined(foo) when there is only a single item being tested for
definedness.
Lluís Vilanova - April 19, 2013, 11:46 p.m.
Anything not commented inline has been changed according to your feedback.


Eric Blake writes:
[...]
>> +##
>> +# @instr-dynamic:
>> +#
>> +# Whether dynamic trace instrumentation is available.
>> +#
>> +# Since: 1.5
>> +##
>> +{ 'command': 'instr-dynamic',
>> +  'returns': 'bool' }

> Perhaps naming this 'query-instr' is nicer, to match existing qemu
> commands that can query state.  Then again, keeping all instrumentation
> commands under the intsr- prefix makes them group together, while using
> query-instr would not.

I leaned towards having a common prefix for all commands, so that they're easier
to remember/find.


> Is the abbreviation 'intsr-' appropriate, or should we be using the full
> word 'instrument-'?

It uses the same prefix as the C routines, which are shorter to avoid too much
typing.


> Is returning a bool going to make it harder to expand this command in
> the future to return additional (optional) information, such as the name
> of the current loaded library?  Is it worth returning a full-blown
> dictionary type, so that we can add other name-value pairs into the
> return as needed?

The command is now 'instr-query', and returns both the available type (none,
static, dynamic), and whether it's currently "active".


> Is it reasonable to give instrumentation libraries a callback hook that they
> can use to optionally generate additional information to be returned as part
> of this query?

Hmmm, letting the instrumentation library to interact both ways with QEMU
through QAPI would be an interesting feature. But I'd like to keep this first
implementation as simple as possible regarding this, and instead concentrate on
the instrumentation functionalities.

Such features can be added later, when the main ideas have been seen fit for
landing upstream.


[...]
>> +# @iargs: arguments to the dynamic instrumentation library

> Any reason you named this 'iargs' instead of the simpler 'args'?

Yes; the auto-generated code already uses "args" internally. I've fixed it to
allow "args".


>> +#
>> +# Since: 1.5
>> +##
>> +{ 'command': 'instr-load',
>> +  'data':    { 'path': 'str', 'iargs': ['String'] } }

> There's another thread going on right now about fixing the generator to
> let us use the much simpler ['str'] instead of having to go through the
> extra layer of the wrapper type 'String'.

> @iargs should be optional (#optional in the comment, listed as '*iargs'
> in the JSON form); where omitting it is shorthand for [ ].

Ok, I thought a list was empty by default when no contents were given to it. It
is now optional.


> Should you return a handle representing the library that got loaded?  By
> returning nothing, you have permanently limited this command to only
> being able to load one library at a time.  But if we return a handle,
> and make instr-unload take a handle argument, then even if we choose to
> support only one library at a time now, later on, we will have the
> option of extending things to support parallel libraries at once without
> having to invent new commands.

The return structure contains an integer handle, now, together with some other
information.


[...]
>> +++ b/qmp.c
>> @@ -24,6 +24,10 @@
>> #include "hw/qdev.h"
>> #include "sysemu/blockdev.h"
>> #include "qom/qom-qobject.h"
>> +#if defined(TRACE_INSTRUMENT_DYNAMIC)

> I don't know whether qemu coding style prefers to use #ifdef foo instead
> of #if defined(foo) when there is only a single item being tested for
> definedness.

I have no strong opinion on either side, so for now I've left it as it is.


Thanks a lot for the review.


Lluis

Patch

diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
index 6c0a18d..67b4528 100644
--- a/include/qapi/qmp/qerror.h
+++ b/include/qapi/qmp/qerror.h
@@ -129,6 +129,15 @@  void assert_no_error(Error *err);
 #define QERR_FEATURE_DISABLED \
     ERROR_CLASS_GENERIC_ERROR, "The feature '%s' is not enabled"
 
+#define QERR_INSTR_LOAD_DL \
+    ERROR_CLASS_GENERIC_ERROR, "Error loading dynamic library: %s"
+
+#define QERR_INSTR_LOAD_LOADED \
+    ERROR_CLASS_GENERIC_ERROR, "Already loaded"
+
+#define QERR_INSTR_LOAD_UNLOADED \
+    ERROR_CLASS_GENERIC_ERROR, "Already unloaded"
+
 #define QERR_INVALID_BLOCK_FORMAT \
     ERROR_CLASS_GENERIC_ERROR, "Invalid block format '%s'"
 
diff --git a/instrument/Makefile.objs b/instrument/Makefile.objs
index 02cc5b7..e3d4e9b 100644
--- a/instrument/Makefile.objs
+++ b/instrument/Makefile.objs
@@ -68,3 +68,4 @@  endif
 target-obj-y += control.o
 
 common-obj-$(CONFIG_SOFTMMU) += hmp.o
+common-obj-$(CONFIG_SOFTMMU) += qmp.o
diff --git a/instrument/qapi-schema.json b/instrument/qapi-schema.json
new file mode 100644
index 0000000..0ecf31f
--- /dev/null
+++ b/instrument/qapi-schema.json
@@ -0,0 +1,33 @@ 
+# *-*- Mode: Python -*-*
+
+##
+# @instr-dynamic:
+#
+# Whether dynamic trace instrumentation is available.
+#
+# Since: 1.5
+##
+{ 'command': 'instr-dynamic',
+  'returns': 'bool' }
+
+##
+# @instr-load:
+#
+# Load a dynamic instrumentation library.
+#
+# @path: path to the dynamic instrumentation library
+# @iargs: arguments to the dynamic instrumentation library
+#
+# Since: 1.5
+##
+{ 'command': 'instr-load',
+  'data':    { 'path': 'str', 'iargs': ['String'] } }
+
+##
+# @instr-unload:
+#
+# Unload the current dynamic instrumentation library.
+#
+# Since: 1.5
+##
+{ 'command': 'instr-unload' }
diff --git a/instrument/qmp.c b/instrument/qmp.c
new file mode 100644
index 0000000..f744250
--- /dev/null
+++ b/instrument/qmp.c
@@ -0,0 +1,70 @@ 
+/*
+ * QMP interface for dynamic trace instrumentation control commands.
+ *
+ * Copyright (C) 2012-2013 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-common.h"
+#include "qapi/qmp/qerror.h"
+#include "qmp-commands.h"
+
+#include <dlfcn.h>
+
+#include "instrument/control.h"
+
+
+
+bool qmp_instr_dynamic(Error **errp)
+{
+    return instr_dynamic();
+}
+
+void qmp_instr_load(const char * path, StringList * iargs, Error **errp)
+{
+    int argc = 0;
+    const char **argv = NULL;
+
+    StringList *entry;
+    while (entry != NULL) {
+        argv = realloc(argv, sizeof(*argv) * (argc + 1));
+        argv[argc] = entry->value->str;
+        argc++;
+        entry = entry->next;
+    }
+
+    InstrLoadError err = instr_load(path, argc, argv);
+    switch (err) {
+    case INSTR_LOAD_OK:
+        break;
+    case INSTR_LOAD_UNAVAILABLE:
+        error_set(errp, QERR_UNSUPPORTED);
+        break;
+    case INSTR_LOAD_LOADED:
+        error_set(errp, QERR_INSTR_LOAD_LOADED);
+        break;
+    case INSTR_LOAD_DL:
+        error_set(errp, QERR_INSTR_LOAD_DL, dlerror());
+        break;
+    }
+}
+
+void qmp_instr_unload(Error **errp)
+{
+    InstrLoadError err = instr_unload();
+    switch (err) {
+    case INSTR_UNLOAD_OK:
+        break;
+    case INSTR_UNLOAD_UNAVAILABLE:
+        error_set(errp, QERR_UNSUPPORTED);
+        break;
+    case INSTR_UNLOAD_UNLOADED:
+        error_set(errp, QERR_INSTR_LOAD_UNLOADED);
+        break;
+    case INSTR_UNLOAD_DL:
+        error_set(errp, QERR_INSTR_LOAD_DL, dlerror());
+        break;
+    }
+}
diff --git a/qapi-schema.json b/qapi-schema.json
index db542f6..bc5fe06 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3513,3 +3513,5 @@ 
     '*asl_compiler_rev':  'uint32',
     '*file':              'str',
     '*data':              'str' }}
+
+input("instrument/qapi-schema.json")
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 1e0e11e..1ea6c38 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1510,6 +1510,77 @@  Notes:
     o Commands that prompt the user for data (eg. 'cont' when the block
       device is encrypted) don't currently work
 
+instr-dynamic
+-------------
+
+Whether dynamic trace instrumentation is available.
+
+Arguments: None.
+
+Example:
+
+-> { "execute": "instr-dyanmic" }
+<- { "return": true }
+
+EQMP
+
+    {
+        .name       = "instr-dynamic",
+        .args_type  = "",
+        .mhandler.cmd_new = qmp_marshal_input_instr_dynamic,
+    },
+
+
+SQMP
+
+instr-load
+----------
+
+Load a dynamic instrumentation library.
+
+Arguments:
+
+- path: path to the dynamic instrumentation library
+- args: arguments to the dynamic instrumentation library
+
+Example:
+
+-> { "execute": "instr-load", "arguments": { "path": "/tmp/libtrace-instrument.so" } }
+<- { "return": {} }
+
+EQMP
+
+    {
+        .name       = "instr-load",
+        .args_type  = "",
+        .mhandler.cmd_new = qmp_marshal_input_instr_load,
+    },
+
+
+SQMP
+
+instr-unload
+------------
+
+Unload the current dynamic instrumentation library.
+
+Arguments: None.
+
+Example:
+
+-> { "execute": "instr-unload" }
+<- { "return": {} }
+
+EQMP
+
+    {
+        .name       = "instr-unload",
+        .args_type  = "",
+        .mhandler.cmd_new = qmp_marshal_input_instr_unload,
+    },
+
+
+SQMP
 3. Query Commands
 =================
 
diff --git a/qmp.c b/qmp.c
index 55b056b..72abe03 100644
--- a/qmp.c
+++ b/qmp.c
@@ -24,6 +24,10 @@ 
 #include "hw/qdev.h"
 #include "sysemu/blockdev.h"
 #include "qom/qom-qobject.h"
+#if defined(TRACE_INSTRUMENT_DYNAMIC)
+#include "instrument/qi-qmp-commands.h"
+#endif
+
 
 NameInfo *qmp_query_name(Error **errp)
 {