Re: [Qemu-devel] [PATCH 1/5] qmp: add query-memory-devices command

[Eric Blake <eblake@redhat.com>]

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

On 06/05/2014 08:36 AM, Igor Mammedov wrote:
> ... allowing to get state of present memory devices.
> Currently implemented only for PCDIMMDevice.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v2:
>  * fix typos an json syntax in QMP example
>  * make 'id' optional to allow command work with
>    anonymous memory devices
> ---
>  hw/mem/pc-dimm.c                |   39 ++++++++++++++++++++++++++++
>  include/hw/mem/pc-dimm.h        |    2 +
>  qapi-schema.json                |   53 +++++++++++++++++++++++++++++++++++++++
>  qmp-commands.hx                 |   27 ++++++++++++++++++++
>  qmp.c                           |   11 ++++++++
>  stubs/Makefile.objs             |    1 +
>  stubs/qmp_pc_dimm_device_list.c |    7 +++++
>  7 files changed, 140 insertions(+), 0 deletions(-)
>  create mode 100644 stubs/qmp_pc_dimm_device_list.c
> 

> +++ b/qapi-schema.json
> @@ -4722,3 +4722,56 @@
>                'btn'     : 'InputBtnEvent',
>                'rel'     : 'InputMoveEvent',
>                'abs'     : 'InputMoveEvent' } }
> +
> +##
> +# @PCDIMMDeviceInfo:
> +#
> +# PCDIMMDevice state information
> +#
> +# @id: the device's ID

Might be worth annotating this as #optional

> +#
> +# @addr: physical address, where device is mapped
> +#
> +# @size: size of memory device provides

grammar reads awkwardly; maybe 's/device/that the device/


> +{ 'type': 'PCDIMMDeviceInfo',
> +  'data': { '*id': 'str',
> +            'addr': 'int',
> +            'size': 'int',
> +            'slot': 'int',
> +            'node': 'int',
> +            'memdev': 'str',
> +            'hotplugged': 'bool',
> +            'hotpluggable': 'bool'
> +          }
> +}

Looks okay.

> +
> +##
> +# @MemoryDeviceInfo:
> +#
> +# Union containing information about a memory device
> +#
> +# Since: 2.1
> +##
> +{ 'union': 'MemoryDeviceInfo', 'data': {'dimm': 'PCDIMMDeviceInfo'} }
> +

This command works, but it is a bit verbose.  A flat union would be a
bit more compact over the wire, but we don't yet have support for a flat
union with an empty base class.  So I'll live with this.

The two tweaks to the .json file only affect comments, so they are minor
enough that I'm okay with:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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: 604 bytes --]