Re: [Qemu-devel] [PATCH 11/13] instrument: [qapi] Add library loader

[Lluís Vilanova]

Eric Blake writes:

> On 07/24/2017 12:46 PM, Lluís Vilanova wrote:
>> Signed-off-by: Lluís Vilanova <address@hidden>
>> ---
>> 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 <address@hidden>
>> +#
>> +# 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