Eric Blake writes: > On 07/24/2017 12:46 PM, Lluís Vilanova wrote: >> Signed-off-by: Lluís Vilanova <vilan...@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 <vilan...@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