Re: [PATCH 2/2] qmp: Expose MachineClass::default_ram_id

[Eduardo Habkost <ehabkost@redhat.com>]

On Mon, May 25, 2020 at 07:03:28PM +0200, Michal Privoznik wrote:
> If a management application (like Libvirt) want's to preserve
> migration ability and switch to '-machine memory-backend' it
> needs to set exactly the same RAM id as QEMU would. Since the id
> is machine type dependant, expose it under 'query-machines'
> result.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>

The code looks good, but documentation was a bit confusing:

> ---
[...]
> +# @default-ram-id: the default name of initial RAM memory region (since 5.1)
> +#

Everywhere else in the commit message you call it "id", but here
you say "name".  Also, I don't think we have any references to a
"memory region" abstraction in the docs for the QAPI schema,
-machine options, or memory backend objects.

I had to look it up in the code, to finally understand you were
talking about the memory backend object ID.

To make it consistent with terminology used for -machine and
QAPI, I suggest:

  @default-ram-id: the default ID of initial RAM memory backend (since 5.1)

I can change it before committing, if you agree.



>  # @default-cpu-type: default CPU model typename if none is requested via
>  #                    the -cpu argument. (since 4.2)
>  #
> @@ -361,7 +363,8 @@
>    'data': { 'name': 'str', '*alias': 'str',
>              '*is-default': 'bool', 'cpu-max': 'int',
>              'hotpluggable-cpus': 'bool',  'numa-mem-supported': 'bool',
> -            'deprecated': 'bool', '*default-cpu-type': 'str' } }
> +            'deprecated': 'bool', 'default-ram-id': 'str',
> +            '*default-cpu-type': 'str' } }
>  
>  ##
>  # @query-machines:
> -- 
> 2.26.2
> 

-- 
Eduardo