diff mbox

[v4,08/20] instrument: [hmp] Add library loader

Message ID 150472050004.24907.4613952373869620144.stgit@frigg.lan (mailing list archive)
State New, archived
Headers show

Commit Message

Lluís Vilanova Sept. 6, 2017, 5:55 p.m. UTC 
Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
 hmp-commands.hx |   28 ++++++++++++++++++++++++++
 monitor.c       |   60 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 88 insertions(+)

Comments

Markus Armbruster Sept. 7, 2017, 8:51 a.m. UTC | #1 
Lluís Vilanova <vilanova@ac.upc.edu> writes:

> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
> ---
>  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  = info_cmds,
>      },
>  
> +    {
> +        .name       = "instr-load",
> +        .args_type  = "path:F,args:s?",
> +        .params     = "path [arg]",
> +        .help       = "load an instrumentation library",
> +        .cmd        = hmp_instr_load,
> +    },
> +
> +STEXI
> +@item instr-load @var{path} [args=value[,...]]
> +@findex instr-load
> +Load an instrumentation library.
> +ETEXI
> +
> +    {
> +        .name       = "instr-unload",
> +        .args_type  = "handle:i",
> +        .params     = "handle",
> +        .help       = "unload an instrumentation library",
> +        .cmd        = 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 *fdname, Error **errp)
>      return fd;
>  }
>  
> +static void hmp_instr_load(Monitor *mon, const QDict *qdict)
> +{
> +    const char *path = qdict_get_str(qdict, "path");
> +    const char *str = qdict_get_try_str(qdict, "args");
> +    strList args;

Blank line between last declaration and first statement, please.

> +    args.value = (str == NULL) ? NULL : (char *)str;

What's wrong with

       args.value = (char *)str;

?

> +    args.next = NULL;
> +    InstrLoadResult *res = qmp_instr_load(path, args.value != NULL,
> +                                          args.value != 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->code);
> +        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 = qmp_instr_load(path, args.value != NULL,
                                             args.value != 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 = qdict_get_int(qdict, "handle");
> +    InstrUnloadResult *res = 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[] = {
>  #include "hmp-commands-info.h"

Likewise.
Lluís Vilanova Sept. 10, 2017, 10:07 p.m. UTC | #2 
Markus Armbruster writes:

> Lluís Vilanova <vilanova@ac.upc.edu> writes:
>> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
>> ---
>> 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  = info_cmds,
>> },
>> 
>> +    {
>> +        .name       = "instr-load",
>> +        .args_type  = "path:F,args:s?",
>> +        .params     = "path [arg]",
>> +        .help       = "load an instrumentation library",
>> +        .cmd        = hmp_instr_load,
>> +    },
>> +
>> +STEXI
>> +@item instr-load @var{path} [args=value[,...]]
>> +@findex instr-load
>> +Load an instrumentation library.
>> +ETEXI
>> +
>> +    {
>> +        .name       = "instr-unload",
>> +        .args_type  = "handle:i",
>> +        .params     = "handle",
>> +        .help       = "unload an instrumentation library",
>> +        .cmd        = 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 *fdname, Error **errp)
>> return fd;
>> }
>> 
>> +static void hmp_instr_load(Monitor *mon, const QDict *qdict)
>> +{
>> +    const char *path = qdict_get_str(qdict, "path");
>> +    const char *str = qdict_get_try_str(qdict, "args");
>> +    strList args;

> Blank line between last declaration and first statement, please.

>> +    args.value = (str == NULL) ? NULL : (char *)str;

> What's wrong with

>        args.value = (char *)str;

> ?

>> +    args.next = NULL;
>> +    InstrLoadResult *res = qmp_instr_load(path, args.value != NULL,
>> +                                          args.value != 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->code);
>> +        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 = qmp_instr_load(path, args.value != NULL,
>                                              args.value != 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 = qdict_get_int(qdict, "handle");
>> +    InstrUnloadResult *res = 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[] = {
>> #include "hmp-commands-info.h"

> Likewise.

Done, thanks!

Lluis
diff mbox

Patch

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  = info_cmds,
     },
 
+    {
+        .name       = "instr-load",
+        .args_type  = "path:F,args:s?",
+        .params     = "path [arg]",
+        .help       = "load an instrumentation library",
+        .cmd        = hmp_instr_load,
+    },
+
+STEXI
+@item instr-load @var{path} [args=value[,...]]
+@findex instr-load
+Load an instrumentation library.
+ETEXI
+
+    {
+        .name       = "instr-unload",
+        .args_type  = "handle:i",
+        .params     = "handle",
+        .help       = "unload an instrumentation library",
+        .cmd        = hmp_instr_unload,
+    },
+
+STEXI
+@item instr-unload
+@findex instr-unload
+Unload an instrumentation library.
+ETEXI
+
 STEXI
 @end table
 ETEXI
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 *fdname, Error **errp)
     return fd;
 }
 
+static void hmp_instr_load(Monitor *mon, const QDict *qdict)
+{
+    const char *path = qdict_get_str(qdict, "path");
+    const char *str = qdict_get_try_str(qdict, "args");
+    strList args;
+    args.value = (str == NULL) ? NULL : (char *)str;
+    args.next = NULL;
+    InstrLoadResult *res = qmp_instr_load(path, args.value != NULL,
+                                          args.value != 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->code);
+        exit(1);
+        break;
+    }
+    qapi_free_InstrLoadResult(res);
+}
+
+static void hmp_instr_unload(Monitor *mon, const QDict *qdict)
+{
+    int64_t handle = qdict_get_int(qdict, "handle");
+    InstrUnloadResult *res = 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[] = {
 #include "hmp-commands-info.h"