[libvirt] [PATCHv2 4/6] qemu: add architecture-specific CPU info handling

[Viktor Mihajlovski]

On 23.03.2018 17:08, John Ferlan wrote:
[...]
>> +static void
>> +qemuMonitorJSONExtractCPUS390Info(virJSONValuePtr jsoncpu,
>> +                                  struct qemuMonitorQueryCpusEntry *cpu)
>> +{
>> +    const char *cpu_state = virJSONValueObjectGetString(jsoncpu, "cpu-state");
>> +
>> +    if (STREQ_NULLABLE(cpu_state, "operating") ||
>> +        STREQ_NULLABLE(cpu_state, "load"))
>> +        cpu->halted = false;
>> +    else if (STREQ_NULLABLE(cpu_state, "stopped") ||
>> +             STREQ_NULLABLE(cpu_state, "check-stop"))
>> +        cpu->halted = true;
> 
> Realistically, since false is the default, then only "stopped" and
> "check-stop" need to be checked... Even 'uninitialized' would show up as
> false since it's not checked.
> 
As you say, it's not necessary to handle the false case explicitly.
Still, I would like to be explicit here.
> Perhaps you should create and carry up the calling stack a copy of the
> cpu-state that way eventually it could be printed in a 'stats' output as
> well...
> 
> I suppose another concern is that some future halted state is created
> and this code does account for that leading to incorrect reporting and
> well some other issues since @halted is used for various decisions.
> 
At this point in time I'm mainly concerned about providing the same
client behaviour with both query-cpus and query-cpus-fast.
In the long run it might make sense to provide architecture specific CPU
information, but this will require more thought and discussion.
>> +}
>> +
>> +/**
>> + * qemuMonitorJSONExtractCPUArchInfo:
>> + * @arch: virtual CPU's architecture
>> + * @jsoncpu: pointer to a single JSON cpu entry
>> + * @cpu: pointer to a single cpu entry
>>   *
>> + * Extracts architecure specific virtual CPU data for a single
>> + * CPU from the QAPI response using an architecture specific
>> + * function.
>> + *
>> + */
>> +static void
>> +qemuMonitorJSONExtractCPUArchInfo(const char *arch,
>> +                                  virJSONValuePtr jsoncpu,
>> +                                  struct qemuMonitorQueryCpusEntry *cpu)
>> +{
>> +    if (STREQ_NULLABLE(arch, "s390"))
>> +        qemuMonitorJSONExtractCPUS390Info(jsoncpu, cpu);
>> +}
>> +
>> +/**
>> + * qemuMonitorJSONExtractCPUArchInfo:
> 
> ^^ This is still qemuMonitorJSONExtractCPUInfo
> 
oops
>> + * @data: JSON response data
>> + * @entries: filled with detected cpu entries on success
>> + * @nentries: number of entries returned
>> + * @fast: true if this is a response from query-cpus-fast
>> + *
>> + * The JSON response @data will have the following format
>> + * in case @fast == false
>>   * [{ "arch": "x86",
>>   *    "current": true,
>>   *    "CPU": 0,
>>   *    "qom_path": "/machine/unattached/device[0]",
>>   *    "pc": -2130415978,
>>   *    "halted": true,
>> - *    "thread_id": 2631237},
>> + *    "thread_id": 2631237,
>> + *    ...},
>> + *    {...}
>> + *  ]
>> + * and for @fast == true
>> + * [{ "arch": "x86",
>> + *    "cpu-index": 0,
>> + *    "qom-path": "/machine/unattached/device[0]",
>> + *    "thread_id": 2631237,
>> + *    ...},
> 
> May as well show the whole example and even provide a s390 example so
> that it's more obvious...
> can do
>>   *    {...}
>>   *  ]
>>   */
>> @@ -1486,6 +1556,7 @@ qemuMonitorJSONExtractCPUInfo(virJSONValuePtr data,
>>          int thread = 0;
>>          bool halted = false;
>>          const char *qom_path;
>> +        const char *arch;
>>          if (!entry) {
>>              ret = -2;
>>              goto cleanup;
>> @@ -1511,6 +1582,10 @@ qemuMonitorJSONExtractCPUInfo(virJSONValuePtr data,
>>          cpus[i].halted = halted;
>>          if (VIR_STRDUP(cpus[i].qom_path, qom_path) < 0)
>>              goto cleanup;
>> +
>> +        /* process optional architecture-specific data */
>> +        arch = virJSONValueObjectGetString(entry, "arch");
>> +        qemuMonitorJSONExtractCPUArchInfo(arch, entry, cpus + i);
> 
> Since you're passing @entry anyway, you could fetch @arch in
> qemuMonitorJSONExtractCPUArchInfo and since it's only valid for "fast ==
> true", calling should be gated on that; otherwise, this would be called
> for both fast options.
I can push the architecture extraction down the stack, but I wouldn't
make the call optional. While not used, there's still
architecture-specific information returned in query-cpus.
> 
> BTW: Rather than "cpus[i]" and "cpus + i", could we just create a
> local/stack "cpu" and use it?
> 
I'll have a look.
> 
> John
> 
>>      }
>>  
>>      VIR_STEAL_PTR(*entries, cpus);
>>
> 


-- 
Regards,
 Viktor Mihajlovski