Re: [libvirt] [PATCHv3 3/6] tests: add qemumonitorjson tests for query-cpus-fast

[Viktor VM Mihajlovski]

On 12.04.2018 17:52, John Ferlan wrote:
> 
> 
> On 04/04/2018 10:45 AM, Viktor Mihajlovski wrote:
>> Extended the json monitor test program with support for query-cpus-fast
>> and added a sample file set for x86 data obtained using the it.
>> Also extend the test program to recognize the halted property.
> 
> This last sentence involves code that probably should be separated into
> its own patch since it's unrelated to the new data. If it's moved this
> patch into one previously, then this patch/adjustment "proves" nothing
> changes (which is good).
> 
> I can do that for you so you don't have to post another series.  I would
> make you the author and add your S-o-b... Of course I need you to say OK
> for that too!
> 
> The rest of this shows what I'd extract...
> Good point. I will gladly accept your offer to split out the 'halted' part.
[...]

-- 
Regards,
  Viktor Mihajlovski

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCHv3 2/6] qemu: use query-cpus-fast in JSON monitor

[Viktor VM Mihajlovski]

On 12.04.2018 17:52, John Ferlan wrote:
> 
> 
> On 04/04/2018 10:45 AM, Viktor Mihajlovski wrote:
>> Use query-cpus-fast instead of query-cpus if supported by QEMU.
>> Based on the QEMU_CAPS_QUERY_CPUS_FAST capability.
>>
>> Signed-off-by: Viktor Mihajlovski 
>> ---
>>  src/qemu/qemu_domain.c   | 14 +++---
>>  src/qemu/qemu_monitor.c  | 30 ++
>>  src/qemu/qemu_monitor.h  |  7 +--
>>  src/qemu/qemu_monitor_json.c | 37 +++--
>>  src/qemu/qemu_monitor_json.h |  3 ++-
>>  tests/qemumonitorjsontest.c  |  4 ++--
>>  6 files changed, 65 insertions(+), 30 deletions(-)
>>
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index 9d1c33b..662937b 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -9006,7 +9006,12 @@ qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
>>  if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
>>  return -1;
>>  
> 
> Count me as one of those that would prefer to see:
> 
> bool fast;
> ...
> fast = virQEMUCapsGet(QEMU_DOMAIN_PRIVATE(vm)->qemuCaps,
>   QEMU_CAPS_QUERY_CPUS_FAST);
> ...
> 
>> -rc = qemuMonitorGetCPUInfo(qemuDomainGetMonitor(vm), , maxvcpus, 
>> hotplug);
>> +rc = qemuMonitorGetCPUInfo(qemuDomainGetMonitor(vm),
>> +   ,
>> +   maxvcpus,
>> +   hotplug,
>> +   
>> virQEMUCapsGet(QEMU_DOMAIN_PRIVATE(vm)->qemuCaps,
>> +  QEMU_CAPS_QUERY_CPUS_FAST));
> 
>rc = qemuMonitorGetCPUInfo(qemuDomainGetMonitor(vm), , maxvcpus,
>   hotplug, fast);
> >>
>>  if (qemuDomainObjExitMonitor(driver, vm) < 0)
>>  goto cleanup;
>> @@ -9025,7 +9030,7 @@ qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
>>   * thread, but it runs every vCPU in that same thread. So it
>>   * is impossible to setup different affinity per thread.
>>   *
>> - * What's more the 'query-cpus' command returns bizarre
>> + * What's more the 'query-cpus[-fast]' command returns bizarre
>>   * data for the threads. It gives the TCG thread for the
>>   * vCPU 0, but for vCPUs 1-> N, it actually replies with
>>   * the main process thread ID.
>> @@ -9126,7 +9131,10 @@ qemuDomainRefreshVcpuHalted(virQEMUDriverPtr driver,
>>  if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
>>  return -1;
>>  
>> -haltedmap = qemuMonitorGetCpuHalted(qemuDomainGetMonitor(vm), maxvcpus);
> 
> 
>> +haltedmap = qemuMonitorGetCpuHalted(qemuDomainGetMonitor(vm),
>> +maxvcpus,
>> +
>> virQEMUCapsGet(QEMU_DOMAIN_PRIVATE(vm)->qemuCaps,
>> +   
>> QEMU_CAPS_QUERY_CPUS_FAST));
> 
> Likewise,
> 
>  bool fast;
> ...
> 
>  fast = virQEMUCapsGet(QEMU_DOMAIN_PRIVATE(vm)->qemuCaps,
>QEMU_CAPS_QUERY_CPUS_FAST);
>  haltedmap = qemuMonitorGetCpuHalted(qemuDomainGetMonitor(vm), maxvcpus,
>  fast);
> 
>>  
>>  if (qemuDomainObjExitMonitor(driver, vm) < 0 || !haltedmap)
>>  goto cleanup;
> 
> [...]
> 
> John
> 
> If you're OK with this I can make the alterations...  What you have
> works - it's just one of those preferential things related to seeing
> and/or relying on function calls within function calls.
> 

Sure. On another day I might have used a local variable as well.
Appreciating and accepting your offer to squash in this changes.

-- 
Regards,
  Viktor Mihajlovski

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list