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