From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50543) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XKBGI-0005RU-GZ for qemu-devel@nongnu.org; Wed, 20 Aug 2014 15:09:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XKBGD-0008DJ-Fl for qemu-devel@nongnu.org; Wed, 20 Aug 2014 15:09:34 -0400 Received: from mx1.redhat.com ([209.132.183.28]:8333) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XKBGD-0008D9-8q for qemu-devel@nongnu.org; Wed, 20 Aug 2014 15:09:29 -0400 Message-ID: <53F4F260.8030600@redhat.com> Date: Wed, 20 Aug 2014 13:09:20 -0600 From: Eric Blake MIME-Version: 1.0 References: <1408556804-5266-1-git-send-email-marc.mari.barcelo@gmail.com> In-Reply-To: <1408556804-5266-1-git-send-email-marc.mari.barcelo@gmail.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="JVQfMEt1E3m0jMDe52pRpuxAuPMQrlgRk" Subject: Re: [Qemu-devel] [RFC] qapi: New command query-mtree List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?B?TWFyYyBNYXLDrQ==?= , qemu-devel@nongnu.org Cc: Paolo Bonzini , =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= , Stefan Hajnoczi , Peter Maydell This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --JVQfMEt1E3m0jMDe52pRpuxAuPMQrlgRk Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 08/20/2014 11:46 AM, Marc Mar=C3=AD wrote: > Add command query-mtree to get the memory tree of the guest. >=20 > As we were looking for a flexible solution on accessing the guest memor= y from > qtests, Stefan came with the idea to implement this new qmp command. >=20 > This way, the result can be parsed, and the RAM direction extracted, so= only > a generic qtest malloc is necessary and not one per machine, as it is > implemented at the moment (malloc-pc uses fw_cfg). >=20 > The actual output is this: http://pastebin.com/nHAH9Jie > Which corresponds to this info mtree: http://pastebin.com/B5vw8DDf Alas, these pastebins will expire, even when we still read qemu.git many years from now. If it weren't for the fact that 116 lines of mtree is exploding into 1033 lines of prettified JSON, I'd suggest putting it here in the commit message. Or if you can come up with a simpler testcase, or summarize a sample section here, even that would be nicer than pointing to what will eventually be a stale URL. >=20 > Signed-off-by: Marc Mar=C3=AD > --- > memory.c | 148 ++++++++++++++++++++++++++++++++++++++++++++++= ++++++++ > qapi-schema.json | 68 +++++++++++++++++++++++++ > qmp-commands.hx | 19 +++++++ > 3 files changed, 235 insertions(+) >=20 > =20 > +static MemRegion *qmp_mtree_mr(const MemoryRegion *mr, hwaddr base, > + MemoryRegionListHead *alias_queue) Indentation is off. > +{ > + MemoryRegionList *new_ml, *ml, *next_ml; > + MemoryRegionListHead submr_print_queue; > + MemRegionList *cur_item =3D NULL, *subregion; > + MemRegion *region =3D NULL; > + const MemoryRegion *submr; > + > + if (!mr || !mr->enabled) { > + return region; > + } > + > + region =3D g_malloc0(sizeof(*region)); g_new0 is safer than g_malloc0. > + > + region->base =3D base+mr->addr; Spaces around binary operators. > + > +MemTree *qmp_query_mtree(Error **errp) > +{ > + MemoryRegionListHead ml_head; > + MemoryRegionList *ml, *ml2; > + AddressSpace *as; > + AddrSpaceList *head =3D NULL, *cur_item =3D NULL, *space; > + MemTree *mt =3D g_malloc0(sizeof(*mt)); > + > + QTAILQ_INIT(&ml_head); > + > + QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) { > + space =3D g_malloc0(sizeof(*space)); > + space->value =3D g_malloc0(sizeof(*space->value)); Several more places where g_new0 is preferred > +++ b/qapi-schema.json > @@ -3481,3 +3481,71 @@ > # Since: 2.1 > ## > { 'command': 'rtc-reset-reinjection' } > + > +## > +# @MemRegion: > +# > +# Information about a memory region > +# > +# @submr: #optional submemory regions of this region Bike-shedding - no need to abbreviate. I'd be fine calling it 'subregions'. Likewise, s/prio/priority/ > +# > +# Since: 2.x s/2.x/2.2/ > +## > +{ 'type': 'MemRegion', > + 'data': {'base': 'uint64', 'size': 'uint64', 'prio': 'uint32', 'read= ': 'bool', > + 'write': 'bool', 'ram': 'bool', 'name': 'str', > + '*alias': 'str', '*submr': ['MemRegion']} } Looks reasonable (modulo any name changes). > + > +## > +# @AddrSpace: > +# > +# An address space of the system > +# > +# @name: name of the address space > +# > +# @mem: a list of memory regions in the address space > +# > +# Since: 2.x s/2.x/2.2/ > +## > +{ 'type': 'AddrSpace', 'data': {'name': 'str', 'mem': 'MemRegion'} } > + > +## > +# @MemTree: > +# > +# Memory tree of the system > +# > +# @spaces: Address spaces of the system > +# > +# @aliases: Aliased memory regions > +# > +# Since: 2.x s/2.x/2.2/ > +## > +{ 'type': 'MemTree', 'data': {'spaces': ['AddrSpace'], > + 'aliases': ['AddrSpace']} } Whitespace doesn't matter, but consistent alignment would look better. > + > +## > +# @query-mtree: > +# > +# Return the memory distribution of the guest > +# > +# Returns: a list of @AddrSpace > +# > +# Since: 2.x s/2.x/2.2/ > +## > +{ 'command': 'query-mtree', 'returns': 'MemTree' } I was expecting ['MemTree'] (an array of structs), but you gave 'MemTreee' (a struct of parallel arrays). Am I guaranteed that returns.spaces and returns.aliases are arrays of the same length? If not, what's the difference between 'spaces' and 'aliases' (that is, why do I need two arrays)? Should the designation of 'space' vs. 'alias' be part of the 'AddrSpace' struct, rather than having to be learned indirectly by which of the two arrays it appeared in? > diff --git a/qmp-commands.hx b/qmp-commands.hx > index 4be4765..22f91b0 100644 > --- a/qmp-commands.hx > +++ b/qmp-commands.hx > @@ -3755,3 +3755,22 @@ Example: > <- { "return": {} } > =20 > EQMP > + { > + .name =3D "query-mtree", > + .args_type =3D "", > + .mhandler.cmd_new =3D qmp_marshal_input_query_mtree, > + }, > +SQMP > +query-mtree > +--------- > + > +Memory tree. > + > +The returned value is a json-array of the memory distribution of the s= ystem. Docs don't match your qapi schema. Let's make sure we get the schema correct, and then make the docs match. > +Each address space is represented by a json-object, which has a name, = and a > +json-array of all memory regions that contain. that contain what? Did you mean "that it contains"? > Each memory region is > +represented by a json-object. > + > +(Missing schema and example) Indeed, hence this being an RFC :) --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --JVQfMEt1E3m0jMDe52pRpuxAuPMQrlgRk 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 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg iQEcBAEBCAAGBQJT9PJgAAoJEKeha0olJ0NqqKIH/RfBpAodWNcMrwcw9FVD+Tt7 mVOsOIOf43f2XF0KKwUZ2a9eCh0RAhuxI9B2Vub3t4gs4EIPqQwIhn3y0gOYiYDZ /N8qeOOZYVaoLiSfYZ2PNN02EBTKSbOWfMr0lxX2aJWAfS7qQoiWQpH3II+p4jg4 zW2btCMMLSQ60gDpsD63Gi0T0BiOehaL6xfeozDFg23y8iQG+aeF9+8KCpANTp2M qnhLA4/mRjEwz8FqzR409tXrus6OsTDLTQ92uVa2wKT1mtrz+bxhcrKK2+zagXw6 sfOOo4eb7gMBO5dFn/8wtOEkntit4i1WPlqFXGT6KBGEFGaaRFFu4CNkWyBPtxw= =/S42 -----END PGP SIGNATURE----- --JVQfMEt1E3m0jMDe52pRpuxAuPMQrlgRk--