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@linux.vnet.ibm.com>
---
 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;
+}
+
+/**
+ * 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:
+ * @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,
+ *    ...},
  *    {...}
  *  ]
  */
@@ -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);
     }
 
     VIR_STEAL_PTR(*entries, cpus);
-- 
1.9.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

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