Re: [Qemu-devel] [RFC] qapi: New command query-mtree

[Eric Blake <eblake@redhat.com>]

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

On 08/20/2014 11:46 AM, Marc Marí wrote:
> Add command query-mtree to get the memory tree of the guest.
> 
> As we were looking for a flexible solution on accessing the guest memory from
> qtests, Stefan came with the idea to implement this new qmp command.
> 
> 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).
> 
> 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.

> 
> Signed-off-by: Marc Marí <marc.mari.barcelo@gmail.com>
> ---
>  memory.c         |  148 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qapi-schema.json |   68 +++++++++++++++++++++++++
>  qmp-commands.hx  |   19 +++++++
>  3 files changed, 235 insertions(+)
> 

>  
> +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 = NULL, *subregion;
> +    MemRegion *region = NULL;
> +    const MemoryRegion *submr;
> +
> +    if (!mr || !mr->enabled) {
> +        return region;
> +    }
> +
> +    region = g_malloc0(sizeof(*region));

g_new0 is safer than g_malloc0.


> +
> +        region->base = base+mr->addr;

Spaces around binary operators.

> +
> +MemTree *qmp_query_mtree(Error **errp)
> +{
> +    MemoryRegionListHead ml_head;
> +    MemoryRegionList *ml, *ml2;
> +    AddressSpace *as;
> +    AddrSpaceList *head = NULL, *cur_item = NULL, *space;
> +    MemTree *mt = g_malloc0(sizeof(*mt));
> +
> +    QTAILQ_INIT(&ml_head);
> +
> +    QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
> +        space = g_malloc0(sizeof(*space));
> +        space->value = 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": {} }
>  
>  EQMP
> +    {
> +        .name       = "query-mtree",
> +        .args_type  = "",
> +        .mhandler.cmd_new = qmp_marshal_input_query_mtree,
> +    },
> +SQMP
> +query-mtree
> +---------
> +
> +Memory tree.
> +
> +The returned value is a json-array of the memory distribution of the system.

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 :)

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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