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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
