From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:39727) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UT1g2-00084N-Ux for qemu-devel@nongnu.org; Thu, 18 Apr 2013 23:07:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UT1g1-0004qE-FG for qemu-devel@nongnu.org; Thu, 18 Apr 2013 23:07:54 -0400 Received: from mx1.redhat.com ([209.132.183.28]:30656) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UT1g1-0004py-6u for qemu-devel@nongnu.org; Thu, 18 Apr 2013 23:07:53 -0400 Message-ID: <5170B505.2040908@redhat.com> Date: Thu, 18 Apr 2013 21:07:49 -0600 From: Eric Blake MIME-Version: 1.0 References: <20130416134949.21588.80064.stgit@fimbulvetr.bsc.es> <20130416135115.21588.60152.stgit@fimbulvetr.bsc.es> In-Reply-To: <20130416135115.21588.60152.stgit@fimbulvetr.bsc.es> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="----enig2HPCPHAIFWPHQGKDQNHUH" Subject: Re: [Qemu-devel] [PATCH v2 15/23] instrument: [qmp, qapi] Add control interface List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?B?TGx1w61zIFZpbGFub3Zh?= Cc: qemu-devel@nongnu.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) ------enig2HPCPHAIFWPHQGKDQNHUH Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 04/16/2013 07:51 AM, Llu=C3=ADs Vilanova wrote: > Add QMP commands to control (un)loading of dynamic instrumentation libr= ary. >=20 > Signed-off-by: Llu=C3=ADs Vilanova > --- > include/qapi/qmp/qerror.h | 9 +++++ > instrument/Makefile.objs | 1 + > instrument/qapi-schema.json | 33 ++++++++++++++++++++ > instrument/qmp.c | 70 +++++++++++++++++++++++++++++++++++= +++++++ > qapi-schema.json | 2 + > qmp-commands.hx | 71 +++++++++++++++++++++++++++++++++++= ++++++++ > qmp.c | 4 ++ > 7 files changed, 190 insertions(+) > create mode 100644 instrument/qapi-schema.json > create mode 100644 instrument/qmp.c >=20 > diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h > index 6c0a18d..67b4528 100644 > --- a/include/qapi/qmp/qerror.h > +++ b/include/qapi/qmp/qerror.h > @@ -129,6 +129,15 @@ void assert_no_error(Error *err); > #define QERR_FEATURE_DISABLED \ > ERROR_CLASS_GENERIC_ERROR, "The feature '%s' is not enabled" > =20 > +#define QERR_INSTR_LOAD_DL \ > + ERROR_CLASS_GENERIC_ERROR, "Error loading dynamic library: %s" These days, it is generally easier to just use error_setg() instead of defining new QERR_ constants, unless you reuse an error so frequently that having a QERR_ constant ensures that all use sites have consistent wording of the message. > +++ b/instrument/qapi-schema.json > @@ -0,0 +1,33 @@ > +# *-*- Mode: Python -*-* > + New files should generally come with a copyright statement and clear licensing terms. Not entirely your fault since qapi-schema.json lacks a copyright statement, but worth thinking about. > +## > +# @instr-dynamic: > +# > +# Whether dynamic trace instrumentation is available. > +# > +# Since: 1.5 > +## > +{ 'command': 'instr-dynamic', > + 'returns': 'bool' } Perhaps naming this 'query-instr' is nicer, to match existing qemu commands that can query state. Then again, keeping all instrumentation commands under the intsr- prefix makes them group together, while using query-instr would not. Is the abbreviation 'intsr-' appropriate, or should we be using the full word 'instrument-'? Is returning a bool going to make it harder to expand this command in the future to return additional (optional) information, such as the name of the current loaded library? Is it worth returning a full-blown dictionary type, so that we can add other name-value pairs into the return as needed? Is it reasonable to give instrumentation libraries a callback hook that they can use to optionally generate additional information to be returned as part of this query? > + > +## > +# @instr-load: > +# > +# Load a dynamic instrumentation library. > +# > +# @path: path to the dynamic instrumentation library Generally a blank line between arguments. > +# @iargs: arguments to the dynamic instrumentation library Any reason you named this 'iargs' instead of the simpler 'args'? > +# > +# Since: 1.5 > +## > +{ 'command': 'instr-load', > + 'data': { 'path': 'str', 'iargs': ['String'] } } There's another thread going on right now about fixing the generator to let us use the much simpler ['str'] instead of having to go through the extra layer of the wrapper type 'String'. @iargs should be optional (#optional in the comment, listed as '*iargs' in the JSON form); where omitting it is shorthand for [ ]. Should you return a handle representing the library that got loaded? By returning nothing, you have permanently limited this command to only being able to load one library at a time. But if we return a handle, and make instr-unload take a handle argument, then even if we choose to support only one library at a time now, later on, we will have the option of extending things to support parallel libraries at once without having to invent new commands. > + > +## > +# @instr-unload: > +# > +# Unload the current dynamic instrumentation library. > +# > +# Since: 1.5 > +## > +{ 'command': 'instr-unload' } See above for comments about whether this should take an argument of the handle returned from the load command. > diff --git a/instrument/qmp.c b/instrument/qmp.c > +++ b/qapi-schema.json > @@ -3513,3 +3513,5 @@ > '*asl_compiler_rev': 'uint32', > '*file': 'str', > '*data': 'str' }} > + > +input("instrument/qapi-schema.json") See my comments on 13/23 about possibly naming this include() instead of input(). > +Example: > + > +-> { "execute": "instr-load", "arguments": { "path": "/tmp/libtrace-in= strument.so" } } This example does not match the JSON listed above, because it omits the mandatory 'iargs':[ ]. > +++ b/qmp.c > @@ -24,6 +24,10 @@ > #include "hw/qdev.h" > #include "sysemu/blockdev.h" > #include "qom/qom-qobject.h" > +#if defined(TRACE_INSTRUMENT_DYNAMIC) I don't know whether qemu coding style prefers to use #ifdef foo instead of #if defined(foo) when there is only a single item being tested for definedness. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org ------enig2HPCPHAIFWPHQGKDQNHUH Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.13 (GNU/Linux) Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJRcLUGAAoJEKeha0olJ0Nq5QAH/jFffeFyDXhizI/0WBaW2uMC oXZvxvFXWGQYO3nOK1ebuWMphCHWqEuwLxAmiRUBoADJcAd8kTmgyzCjQQGehOSt 1AZREXZC9SO9cJXM53AiH1knWITN33FgZFlOtynANm8ABnJW7xRTU8Y0Lnh5D5q7 7H0W4MF9ChEJDP2p+BxN37el7YbxnXvQroihW0uElTnbzrlLZCYSETB9fj1CnhuF M9D+KtVTBugSOaLyQOOej8MS2pUkpk60VCR5vtIJ/9WXo/ER/cjMdSzyrVBRvlpv MworRelAqgt+yczTmNzNKhittVW1OJwmHYkkXuG/BfYlcq0IqCUr1K3vkn78bC0= =JEG6 -----END PGP SIGNATURE----- ------enig2HPCPHAIFWPHQGKDQNHUH--