On 03/05/2018 06:44 AM, Viktor Mihajlovski wrote:
> Extract architecture specific data from query-cpus[-fast] if
> available. A new function qemuMonitorJSONExtractCPUArchInfo()
> uses a call-back table to find and call architecture-specific
> extraction handlers.
>
> Initially, there's a handler for s390 cpu info to
> set the halted property depending on the s390 cpu state
> returned by QEMU. With this it's still possible to report
> the halted condition even when using query-cpus-fast.
>
> Signed-off-by: Viktor Mihajlovski <mihajlov@xxxxxxxxxxxxxxxxxx>
> ---
> src/qemu/qemu_monitor.c | 4 +--
> src/qemu/qemu_monitor_json.c | 79 ++++++++++++++++++++++++++++++++++++++++++--
> 2 files changed, 79 insertions(+), 4 deletions(-)
>
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 22b2091..5840e25 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -2076,7 +2076,7 @@ qemuMonitorGetCPUInfo(qemuMonitorPtr mon,
> virBitmapPtr
> qemuMonitorGetCpuHalted(qemuMonitorPtr mon,
> size_t maxvcpus,
> - bool fast ATTRIBUTE_UNUSED)
> + bool fast)
> {
> struct qemuMonitorQueryCpusEntry *cpuentries = NULL;
> size_t ncpuentries = 0;
> @@ -2088,7 +2088,7 @@ qemuMonitorGetCpuHalted(qemuMonitorPtr mon,
>
> if (mon->json)
> rc = qemuMonitorJSONQueryCPUs(mon, &cpuentries, &ncpuentries, false,
> - false);
> + fast);
> else
> rc = qemuMonitorTextQueryCPUs(mon, &cpuentries, &ncpuentries);
>
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 6a5fb12..1924cfe 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -1451,15 +1451,85 @@ int qemuMonitorJSONSystemReset(qemuMonitorPtr mon)
> }
>
>
> -/*
> +/**
> + * qemuMonitorJSONExtractCPUS390Info:
> + * @jsoncpu: pointer to a single JSON cpu entry
> + * @cpu: pointer to a single cpu entry
> + *
> + * Derive the legacy cpu info 'halted' information
> + * from the more accurate s390 cpu state. @cpu is
> + * modified only on success.
> + *
> + * Note: the 'uninitialized' s390 cpu state can't be
> + * mapped to halted yes/no.
> + *
> + * A s390 cpu entry could look like this
> + * { "arch": "s390",
> + * "cpu-index": 0,
> + * "qom-path": "/machine/unattached/device[0]",
> + * "thread_id": 3081,
> + * "cpu-state": "operating" }
> + *
> + */
> +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.
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.
> +}
> +
> +/**
> + * 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
> + * @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...
> * {...}
> * ]
> */
> @@ -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.
BTW: Rather than "cpus[i]" and "cpus + i", could we just create a
local/stack "cpu" and use it?
John
> }
>
> VIR_STEAL_PTR(*entries, cpus);
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
