Re: [Qemu-devel] [RFC PATCH v2 11/15] cpu-model/s390: Add QMP command query-cpu-model

[Michael Mueller]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Tue, 17 Feb 2015 11:03:11 -0700
Eric Blake <eblake@xxxxxxxxxx> wrote:

> On 02/17/2015 07:24 AM, Michael Mueller wrote:
> > This patch implements a new QMP request named 'query-cpu-model'.
> > It returns the cpu model of cpu 0 and its backing accelerator.
> > 
> > request:
> >   {"execute" : "query-cpu-model" }
> > 
> > answer:
> >   {"return" : {"name": "2827-ga2", "accelerator": "kvm" }}
> > 
> > Alias names are resolved to their respective machine type and GA names
> > already during cpu instantiation. Thus, also a cpu model like 'host'
> > which is implemented as alias will return its normalized cpu model name.
> > 
> > Furthermore the patch implements the following functions:
> > 
> > - s390_cpu_typename(), returns the currently selected cpu type name or NULL
> > - s390_cpu_models_used(), returns true if S390 cpu models are in use
> > 
> > Signed-off-by: Michael Mueller <mimu@xxxxxxxxxxxxxxxxxx>
> > ---
> >  
> > +##
> > +# @CpuModelInfo:
> > +#
> > +# Virtual CPU model definition.
> > +#
> > +# @name: the name of the CPU model definition
> > +#
> > +# Since: 2.3.0
> > +##
> > +{ 'type': 'CpuModelInfo',
> > +  'data': { 'name': 'str', '*accelerator': 'AccelId' } }
> 
> You didn't document '*accelerator', including mention that it is
> optional (why would it not be output always?).

Right, as it is a new command and all cpus once implementing it have an associated accelerator
there is no point in making it optional. Will add a comment as well.
> 
> > +
> > +##
> > +# @query-cpu-model:
> > +#
> > +# Return to current virtual CPU model
> 
> s/to/the/

yep

> 
> > +#
> > +# Returns: CpuModelInfo
> > +#
> > +# Since: 2.3.0
> 
> We aren't very consistent on '2.3' vs. '2.3.0', so I won't complain
> about that.

But you seem to prefer major and minor only, so I will skip the trailing 0.

> 
> > +##
> > +{ 'command': 'query-cpu-model', 'returns': 'CpuModelInfo' }
> 
> Seems reasonable from the interface point of view; I have not closely
> reviewed the implementation.
> 

Thanks
Michael
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBAgAGBQJU5E/fAAoJELPcPLQSJsKQYksP/inWhcSnQF6WgNTaF2qsEqyG
6kRyAwfVID9k0Rh1Eh3fDKbDDrtXe1pB3dbHlX6JWWrDCQCzfFxbRDElK3lX4k1g
ryAi9ZRPZ7TB88eGIS66vQ4J/O9WWe/I7tzoX2yeFcMRNa9+8dpyluPHWNYbeN6f
KkzQy15PabCiggwQuDfm+X4jiV7HtIdxRVNuOJV4j2U8p6Wp5YaZMEp8MJQ2tDMe
Uion91VZ2izLQ/rn+QIb1F28om03LWfh47sKzam9l9oomFglGhmHXkEktL+KCQTK
MSUQOiIK3rSzZiFq5dsjIAOo1bkrjYpPxUfxq0xDjreJoGkd1sbtQ663QHG0RUTT
1G0BNKHxNggkxoxyChaEYWopNyZ2wMEn3XBaVHvnfDqDyymsWo3nAQVWIMwFpHth
vSfBLY3CJnAW4HnDiaV3+sNDlFZlttzPE5jVXibR98sCdQORNEpCt9frdu1AToGB
MCiAwJMc1qescJMghsogCUzG9ElA7wbvGIv+e/uEGj7LB/1H0ZMIpN/QU9NDd3Fi
XpldGvPLIWRJq7/l8c6/KNg8SbTn1prtGZ1pKNOlkmn+BeEQjcihEmGOStoWny48
1hT08enhBJ9skXvWlGTvYBlohQ6vERwZ5B9E4tmSeLRQG+4YkE3JTyb0FJJScBoV
l1X+PmwiT2AfeXLlDHlm
=6Qmi
-----END PGP SIGNATURE-----