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

From: Eric Blake <eblake@redhat.com>
To: "Lluís Vilanova" <vilanova@ac.upc.edu>, qemu-devel@nongnu.org
Cc: "Emilio G. Cota" <cota@braap.org>,
	Stefan Hajnoczi <stefanha@redhat.com>,
	Markus Armbruster <armbru@redhat.com>
Subject: Re: [Qemu-devel] [PATCH 11/13] instrument: [qapi] Add library loader
Date: Mon, 24 Jul 2017 13:03:46 -0500	[thread overview]
Message-ID: <109e1849-1bf6-5875-b304-7cacbf45f068@redhat.com> (raw)
In-Reply-To: <150091841019.30739.3661641061220051037.stgit@frigg.lan>

[-- Attachment #1: Type: text/plain, Size: 3786 bytes --]

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, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]