| Message ID | 150091841019.30739.3661641061220051037.stgit@frigg.lan (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/24/2017 12:46 PM, Lluís Vilanova wrote: > Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu> > --- > instrument/Makefile.objs | 1 + > instrument/qmp.c | 71 ++++++++++++++++++++++++++++++++++++ > qapi-schema.json | 3 ++ > qapi/instrument.json | 92 ++++++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 167 insertions(+) > create mode 100644 instrument/qmp.c > create mode 100644 qapi/instrument.json Adding new files; but I don't see a patch to MAINTAINERS to cover instrument/*. > +++ b/qapi/instrument.json > @@ -0,0 +1,92 @@ > +# *-*- Mode: Python -*-* > +# > +# QAPI trace 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. > + > +## > +# @InstrLoadCode: > +# > +# Result code of an 'instr-load' command. > +# > +# @ok: Correctly loaded. > +# @unavailable: Service not available. > +# @error: Error with libdl (see 'msg'). > +# > +# Since: 2.10 This is a new feature, and you've missed soft freeze. You'll want to use 2.11 throughout the file. > +## > +{ 'enum': 'InstrLoadCode', > + 'data': [ 'ok', 'unavailable', 'error' ] } > + > +## > +# @InstrLoadResult: > +# > +# Result of an 'instr-load' command. > +# > +# @code: Result code. > +# @msg: Additional error message. Worth a comment that the message is for human consumption, and should not be further parsed by machine? Should msg be optional, present only when there is an error? > +# @handle: Instrumentation library identifier (undefined in case of error). Should it be an optional member, omitted when there is an error? > +# > +# Since: 2.10 > +## > +{ '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.10 > +## > +{ 'command': 'instr-load', > + 'data': { 'path': 'str', '*args': ['String'] }, Why are you double-nesting things? It's a lot nicer to use ['str'] (that is, your way requires "arguments":{"path":"/some/path", "args": [ { "str": "string1" }, { "str": "string2" } ] } whereas mine only needs: "arguments":{"path":"/some/path", "args":[ "string1", "string2" ]} > + 'returns': 'InstrLoadResult' } > + > + > +## > +# @InstrUnloadCode: > +# > +# Result code of an 'instr-unload' command. > +# > +# @ok: Correctly unloaded. > +# @unavailable: Service not available. > +# @invalid: Invalid handle. > +# @error: Error with libdl (see 'msg'). > +# > +# Since: 2.10 > +## > +{ 'enum': 'InstrUnloadCode', > + 'data': [ 'ok', 'unavailable', 'invalid', 'error' ] } > + > +## > +# @InstrUnloadResult: > +# > +# Result of an 'instr-unload' command. > +# > +# @code: Result code. > +# @msg: Additional error message. Again, should msg be optional? Document that it is only for human consumption. > +# > +# Since: 2.10 > +## > +{ 'struct': 'InstrUnloadResult', > + 'data': { 'code': 'InstrUnloadCode', 'msg': 'str' } } > + > +## > +# @instr-unload: > +# > +# Unload an instrumentation library. > +# > +# @handle: Instrumentation library identifier (see #InstrLoadResult). > +# > +# Since: 2.10 > +## > +{ 'command': 'instr-unload', > + 'data': { 'handle': 'int' }, > + 'returns': 'InstrUnloadResult' } > >
Eric Blake writes: > On 07/24/2017 12:46 PM, Lluís Vilanova wrote: >> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu> >> --- >> instrument/Makefile.objs | 1 + >> instrument/qmp.c | 71 ++++++++++++++++++++++++++++++++++++ >> qapi-schema.json | 3 ++ >> qapi/instrument.json | 92 ++++++++++++++++++++++++++++++++++++++++++++++ >> 4 files changed, 167 insertions(+) >> create mode 100644 instrument/qmp.c >> create mode 100644 qapi/instrument.json > Adding new files; but I don't see a patch to MAINTAINERS to cover > instrument/*. Who should I put as a maintainer? Or does this go to the general maintainer(s)? >> +++ b/qapi/instrument.json >> @@ -0,0 +1,92 @@ >> +# *-*- Mode: Python -*-* >> +# >> +# QAPI trace 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. >> + >> +## >> +# @InstrLoadCode: >> +# >> +# Result code of an 'instr-load' command. >> +# >> +# @ok: Correctly loaded. >> +# @unavailable: Service not available. >> +# @error: Error with libdl (see 'msg'). >> +# >> +# Since: 2.10 > This is a new feature, and you've missed soft freeze. You'll want to > use 2.11 throughout the file. Thx! >> +## >> +{ 'enum': 'InstrLoadCode', >> + 'data': [ 'ok', 'unavailable', 'error' ] } >> + >> +## >> +# @InstrLoadResult: >> +# >> +# Result of an 'instr-load' command. >> +# >> +# @code: Result code. >> +# @msg: Additional error message. > Worth a comment that the message is for human consumption, and should > not be further parsed by machine? > Should msg be optional, present only when there is an error? True. >> +# @handle: Instrumentation library identifier (undefined in case of error). > Should it be an optional member, omitted when there is an error? Also true. >> +# >> +# Since: 2.10 >> +## >> +{ '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.10 >> +## >> +{ 'command': 'instr-load', >> + 'data': { 'path': 'str', '*args': ['String'] }, > Why are you double-nesting things? It's a lot nicer to use ['str'] > (that is, your way requires > "arguments":{"path":"/some/path", > "args": [ { "str": "string1" }, { "str": "string2" } ] } > whereas mine only needs: > "arguments":{"path":"/some/path", "args":[ "string1", "string2" ]} Aha, you mean the definition should be this instead? { 'command': 'instr-load', 'data': { 'path': 'str', '*args': ['str'] }, 'returns': 'InstrLoadResult' } >> + 'returns': 'InstrLoadResult' } >> + >> + >> +## >> +# @InstrUnloadCode: >> +# >> +# Result code of an 'instr-unload' command. >> +# >> +# @ok: Correctly unloaded. >> +# @unavailable: Service not available. >> +# @invalid: Invalid handle. >> +# @error: Error with libdl (see 'msg'). >> +# >> +# Since: 2.10 >> +## >> +{ 'enum': 'InstrUnloadCode', >> + 'data': [ 'ok', 'unavailable', 'invalid', 'error' ] } >> + >> +## >> +# @InstrUnloadResult: >> +# >> +# Result of an 'instr-unload' command. >> +# >> +# @code: Result code. >> +# @msg: Additional error message. > Again, should msg be optional? Document that it is only for human > consumption. Ok. >> +# >> +# Since: 2.10 >> +## >> +{ 'struct': 'InstrUnloadResult', >> + 'data': { 'code': 'InstrUnloadCode', 'msg': 'str' } } >> + >> +## >> +# @instr-unload: >> +# >> +# Unload an instrumentation library. >> +# >> +# @handle: Instrumentation library identifier (see #InstrLoadResult). >> +# >> +# Since: 2.10 >> +## >> +{ 'command': 'instr-unload', >> + 'data': { 'handle': 'int' }, >> + 'returns': 'InstrUnloadResult' } >> >> Thanks, Lluis
On 07/25/2017 03:24 AM, Lluís Vilanova wrote: > Eric Blake writes: > >> On 07/24/2017 12:46 PM, Lluís Vilanova wrote: >>> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu> >>> --- >>> instrument/Makefile.objs | 1 + >>> instrument/qmp.c | 71 ++++++++++++++++++++++++++++++++++++ >>> qapi-schema.json | 3 ++ >>> qapi/instrument.json | 92 ++++++++++++++++++++++++++++++++++++++++++++++ >>> 4 files changed, 167 insertions(+) >>> create mode 100644 instrument/qmp.c >>> create mode 100644 qapi/instrument.json > >> Adding new files; but I don't see a patch to MAINTAINERS to cover >> instrument/*. > > Who should I put as a maintainer? Or does this go to the general maintainer(s)? You can be the maintainer if you'd like; or see if Stefan is okay including it as part of the trace files, since it trace-related. >>> +## >>> +{ 'command': 'instr-load', >>> + 'data': { 'path': 'str', '*args': ['String'] }, > >> Why are you double-nesting things? It's a lot nicer to use ['str'] > Aha, you mean the definition should be this instead? > > { 'command': 'instr-load', > 'data': { 'path': 'str', '*args': ['str'] }, > 'returns': 'InstrLoadResult' } Yes.
Eric Blake writes: > On 07/25/2017 03:24 AM, Lluís Vilanova wrote: >> Eric Blake writes: >> >>> On 07/24/2017 12:46 PM, Lluís Vilanova wrote: >>>> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu> >>>> --- >>>> instrument/Makefile.objs | 1 + >>>> instrument/qmp.c | 71 ++++++++++++++++++++++++++++++++++++ >>>> qapi-schema.json | 3 ++ >>>> qapi/instrument.json | 92 ++++++++++++++++++++++++++++++++++++++++++++++ >>>> 4 files changed, 167 insertions(+) >>>> create mode 100644 instrument/qmp.c >>>> create mode 100644 qapi/instrument.json >> >>> Adding new files; but I don't see a patch to MAINTAINERS to cover >>> instrument/*. >> >> Who should I put as a maintainer? Or does this go to the general maintainer(s)? > You can be the maintainer if you'd like; or see if Stefan is okay > including it as part of the trace files, since it trace-related. I can certainly do that. I was just wondering if the project prefers to have someone on the "core team" as a maintainer. Stefan? Thanks, Lluis
diff --git a/instrument/Makefile.objs b/instrument/Makefile.objs index aa6db29ff4..757a247321 100644 --- a/instrument/Makefile.objs +++ b/instrument/Makefile.objs @@ -44,3 +44,4 @@ $(obj)/qemu-instr/events.h-timestamp: $(BUILD_DIR)/trace-events-all $(BUILD_DIR) target-obj-y += control.o target-obj-y += cmdline.o +target-obj-y += qmp.o diff --git a/instrument/qmp.c b/instrument/qmp.c new file mode 100644 index 0000000000..3f577e0c78 --- /dev/null +++ b/instrument/qmp.c @@ -0,0 +1,71 @@ +/* + * QMP interface for dynamic trace 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> + +#include "instrument/control.h" + + + +InstrLoadResult *qmp_instr_load(const char * path, + bool have_args, StringList * args, + Error **errp) +{ + int argc = 0; + const char **argv = NULL; + + StringList *entry = have_args ? args : NULL; + while (entry != NULL) { + argv = realloc(argv, sizeof(*argv) * (argc + 1)); + argv[argc] = entry->value->str; + argc++; + entry = entry->next; + } + + InstrLoadResult *res = g_malloc0(sizeof(*res)); + res->code = instr_load(path, argc, argv, &res->handle); + switch (res->code) { + case INSTR_LOAD_CODE_OK: + case INSTR_LOAD_CODE_UNAVAILABLE: + break; + case INSTR_LOAD_CODE_ERROR: + res->msg = dlerror(); + break; + default: + fprintf(stderr, "Unknown instrumentation load code: %d", res->code); + exit(1); + break; + } + return res; +} + +InstrUnloadResult *qmp_instr_unload(int64_t handle, Error **errp) +{ + InstrUnloadResult *res = g_malloc0(sizeof(*res)); + res->code = instr_unload(handle); + switch (res->code) { + case INSTR_UNLOAD_OK: + case INSTR_UNLOAD_UNAVAILABLE: + case INSTR_UNLOAD_INVALID: + break; + case INSTR_UNLOAD_CODE_ERROR: + res->msg = dlerror(); + break; + default: + fprintf(stderr, "Unknown instrumentation unload code: %d", res->code); + exit(1); + break; + } + return res; +} diff --git a/qapi-schema.json b/qapi-schema.json index ab438ead70..6c4f237af8 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..f0d725e9a9 --- /dev/null +++ b/qapi/instrument.json @@ -0,0 +1,92 @@ +# *-*- Mode: Python -*-* +# +# QAPI trace 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. + +## +# @InstrLoadCode: +# +# Result code of an 'instr-load' command. +# +# @ok: Correctly loaded. +# @unavailable: Service not available. +# @error: Error with libdl (see 'msg'). +# +# Since: 2.10 +## +{ 'enum': 'InstrLoadCode', + 'data': [ 'ok', 'unavailable', 'error' ] } + +## +# @InstrLoadResult: +# +# Result of an 'instr-load' command. +# +# @code: Result code. +# @msg: Additional error message. +# @handle: Instrumentation library identifier (undefined in case of error). +# +# Since: 2.10 +## +{ '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.10 +## +{ 'command': 'instr-load', + 'data': { 'path': 'str', '*args': ['String'] }, + 'returns': 'InstrLoadResult' } + + +## +# @InstrUnloadCode: +# +# Result code of an 'instr-unload' command. +# +# @ok: Correctly unloaded. +# @unavailable: Service not available. +# @invalid: Invalid handle. +# @error: Error with libdl (see 'msg'). +# +# Since: 2.10 +## +{ 'enum': 'InstrUnloadCode', + 'data': [ 'ok', 'unavailable', 'invalid', 'error' ] } + +## +# @InstrUnloadResult: +# +# Result of an 'instr-unload' command. +# +# @code: Result code. +# @msg: Additional error message. +# +# Since: 2.10 +## +{ 'struct': 'InstrUnloadResult', + 'data': { 'code': 'InstrUnloadCode', 'msg': 'str' } } + +## +# @instr-unload: +# +# Unload an instrumentation library. +# +# @handle: Instrumentation library identifier (see #InstrLoadResult). +# +# Since: 2.10 +## +{ 'command': 'instr-unload', + 'data': { 'handle': 'int' }, + 'returns': 'InstrUnloadResult' }
Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu> --- instrument/Makefile.objs | 1 + instrument/qmp.c | 71 ++++++++++++++++++++++++++++++++++++ qapi-schema.json | 3 ++ qapi/instrument.json | 92 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 167 insertions(+) create mode 100644 instrument/qmp.c create mode 100644 qapi/instrument.json