Re: [Qemu-devel] [PATCH v7 12/13] qmp: Add query-ppc-cpu-cores command

[Bharata B Rao <bharata@linux.vnet.ibm.com>]

On Thu, Jan 28, 2016 at 01:52:26PM -0700, Eric Blake wrote:
> On 01/27/2016 10:49 PM, Bharata B Rao wrote:
> > Show the details of PPC CPU cores via a new QMP command.
> > 
> > TODO: update qmp-commands.hx with example
> 
> Is this a stale comment? [1]

Yes, I missed removing it after I put the example in qmp-commands.hx.

> 
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > ---
> 
> >  #include <sysemu/cpus.h>
> > +#include <sysemu/kvm.h>
> >  #include "qemu/error-report.h"
> > +#include "qmp-commands.h"
> > +
> > +/*
> > + * QMP: info ppc-cpu-cores
> > + */
> > +static int qmp_ppc_cpu_list(Object *obj, void *opaque)
> 
> Comment is a bit off - 'info ...' is an HMP command; this callback is
> helping implement the QMP function query-ppc-cpu-cores.

Ok, will make it "QMP: query-ppc-cpu-cores"

> 
> > +++ b/qapi-schema.json
> > @@ -4083,3 +4083,34 @@
> >  ##
> >  { 'enum': 'ReplayMode',
> >    'data': [ 'none', 'record', 'play' ] }
> > +
> > +##
> > +# @PPCCPUCore:
> > +#
> > +# Information about PPC CPU core devices
> > +#
> > +# @hotplugged: true if device was hotplugged
> > +#
> > +# @hotpluggable: true if device if could be added/removed while machine is running
> > +#
> > +# Since: 2.6
> 
> Missing docs on 'id' and 'threads'.

Will add.

> 
> > +##
> > +
> > +{ 'struct': 'PPCCPUCore',
> > +  'data': { '*id': 'str',
> > +            'hotplugged': 'bool',
> > +            'hotpluggable': 'bool',
> > +            'threads' : ['CpuInfo']
> > +          }
> > +}
> > +
> > +##
> > +# @query-ppc-cpu-core:
> > +#
> > +# Returns information for all PPC CPU core devices
> > +#
> > +# Returns: a list of @PPCCPUCore.
> > +#
> > +# Since: 2.6
> > +##
> > +{ 'command': 'query-ppc-cpu-cores', 'returns': ['PPCCPUCore'] }
> 
> Interface seems okay.
> 
> > +++ b/qmp-commands.hx
> > @@ -4795,3 +4795,54 @@ Example:
> >                   {"type": 0, "out-pport": 0, "pport": 0, "vlan-id": 3840,
> >                    "pop-vlan": 1, "id": 251658240}
> >     ]}
> > +
> > +EQMP
> > +
> > +#if defined TARGET_PPC64
> > +    {
> > +        .name       = "query-ppc-cpu-cores",
> > +        .args_type  = "",
> > +        .mhandler.cmd_new = qmp_marshal_query_ppc_cpu_cores,
> > +    },
> > +#endif
> 
> Hmm. Conditional compilation. Does the command show up in
> 'query-commands' and introspection output, even when the target is not
> ppc64?  We may need to fix qapi introspection to support conditionals
> better; maybe some of Marc-Andre's patches towards eliminating
> qmp-commands.hx will come into play here.

For targets other than ppc64, query-commands doesn't list
query-ppc-cpu-cores.

> 
> > +
> > +SQMP
> > +@query-ppc-cpu-cores
> > +--------------------
> > +
> > +Show PowerPC CPU core devices information.
> > +
> > +Example:
> > +-> { "execute": "query-ppc-cpu-cores" }
> > +<- {"return": [{"threads": [
> > +                 {"arch": "ppc",
> 
> [1] looks like you provided an example after all.  Is it worth
> documenting that this command is only conditionally available?

Will add

# Note: This command is available only for PowerPC targets

> 
> 
> > +++ b/stubs/qmp_query_ppc_cpu_cores.c
> > @@ -0,0 +1,10 @@
> > +#include "qom/object.h"
> > +#include "qapi/qmp/qerror.h"
> > +#include "qemu/typedefs.h"
> > +#include "qmp-commands.h"
> > +
> > +PPCCPUCoreList *qmp_query_ppc_cpu_cores(Error **errp)
> > +{
> > +    error_setg(errp, QERR_UNSUPPORTED);
> > +    return 0;
> > +}
> 
> Hmm - will the stub even be used, since you used an #ifdef in the .hx file?

Though I have put

.mhandler.cmd_new = qmp_marshal_query_ppc_cpu_cores,

under ifdef in .hx, qmp_marshal_query_ppc_cpu_cores() is getting defined in
qmp-marshal.c and hence we need this stub file so that qmp_query_ppc_cpu_cores()
gets resolved from qmp-marshal.c:qmp_marshal_query_ppc_cpu_cores().

Regards,
Bharata.