Re: [Qemu-devel] [PATCH v7 13/13] hmp: Add "info ppc-cpu-cores" command

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

On Thu, Jan 28, 2016 at 02:56:41PM -0700, Eric Blake wrote:
> On 01/27/2016 10:49 PM, Bharata B Rao wrote:
> > This is the hmp equivalent of "query ppc-cpu-cores"
> 
> The QMP command is spelled "query-ppc-cpu-cores".
> 
> Most HMP commands prefer '_' over '-'; so this should be 'info
> ppc_cpu_cores'.

I see that a few commands have '-' but as you note, majority of them use
'_'. Though I personally prefer '-', if HMP convention is to go with '_',
will change in the next iteration.

> 
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > ---
> >  hmp-commands-info.hx | 16 ++++++++++++++++
> >  hmp.c                | 31 +++++++++++++++++++++++++++++++
> >  hmp.h                |  1 +
> >  3 files changed, 48 insertions(+)
> > 
> 
> > +++ b/hmp.c
> > @@ -2375,3 +2375,34 @@ void hmp_rocker_of_dpa_groups(Monitor *mon, const QDict *qdict)
> >  
> >      qapi_free_RockerOfDpaGroupList(list);
> >  }
> > +
> > +void hmp_info_ppc_cpu_cores(Monitor *mon, const QDict *qdict)
> > +{
> > +    Error *err = NULL;
> > +    PPCCPUCoreList *ppc_cpu_core_list = qmp_query_ppc_cpu_cores(&err);
> > +    PPCCPUCoreList *s = ppc_cpu_core_list;
> > +    CpuInfoList *thread;
> > +
> > +    while (s) {
> > +        monitor_printf(mon, "PowerPC CPU device: \"%s\"\n",
> > +                       s->value->id ? s->value->id : "");
> 
> This should probably be checking s->value->has_id rather than assuming
> that s->value->id will be NULL when not present (well, I'd like to clean
> up qapi to avoid the need for has_FOO when FOO  is a pointer, but we're
> not there yet).

Ok, will switch to s->value->has_id ? s->value->id : "")

> 
> > +        monitor_printf(mon, "  hotplugged: %s\n",
> > +                           s->value->hotplugged ? "true" : "false");
> > +        monitor_printf(mon, "  hotpluggable: %s\n",
> > +                           s->value->hotpluggable ? "true" : "false");
> > +        monitor_printf(mon, "  Threads:\n");
> > +        for (thread = s->value->threads; thread; thread = thread->next) {
> > +            monitor_printf(mon, "    CPU #%" PRId64 ":", thread->value->CPU);
> > +            monitor_printf(mon, " nip=0x%016" PRIx64,
> > +                           thread->value->u.ppc->nip);
> 
> This uses value->u.ppc without first checking that the discriminator
> value->arch is set to CPU_INFO_ARCH_PPC; could that be a problem down
> the road?

Can't think of any potential problems as this command is PowerPC
specific.

BTW can you please let me know what else is needed from QEMU end to
drive this PowerPC CPU core device hotplug from libvirt ?

Regards,
Bharata.