Re: [libvirt] [PATCHv2 4/6] qemu: add architecture-specific CPU info handling
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