[libvirt] [PATCH 0/2] [for-5.7.0] qemu: Honor qemu's request to use all vCPU props
qemu documents that we should pass in all fields of 'CpuInstanceProperties' back when hotplugging cpus, but we didn't do that. It turns out that qemu wanted to add new fields which were mandatory and thus broke the interop with libvirt. Let's honor their reques. Peter Krempa (2): qemu: Extract and store vCPU properties as qemu returned them qemu: command: Use all vCPU properties when creating args for vCPU hotplug src/qemu/qemu_command.c | 19 +++---------------- src/qemu/qemu_domain.c | 3 +++ src/qemu/qemu_domain.h | 3 +++ src/qemu/qemu_monitor.c | 2 ++ src/qemu/qemu_monitor.h | 6 ++++++ src/qemu/qemu_monitor_json.c | 4 ++++ .../x86-modern-bulk-monitor.json | 8 ++++---- .../x86-modern-individual-add-monitor.json | 4 ++-- 8 files changed, 27 insertions(+), 22 deletions(-) -- 2.21.0
In addition to the data that libvirt needs and extracts internally, copy and store the whole 'props' JSON sub-object of the data returned by query-hotpluggable-cpus for future use. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 3 +++ src/qemu/qemu_domain.h | 3 +++ src/qemu/qemu_monitor.c | 2 ++ src/qemu/qemu_monitor.h | 6 ++++++ src/qemu/qemu_monitor_json.c | 4 ++++ 5 files changed, 18 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 4998474dc9..657f3ecfe4 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1144,6 +1144,7 @@ qemuDomainVcpuPrivateDispose(void *obj) VIR_FREE(priv->type); VIR_FREE(priv->alias); + virJSONValueFree(priv->props); return; } @@ -11920,6 +11921,8 @@ qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver, VIR_STEAL_PTR(vcpupriv->type, info[i].type); VIR_FREE(vcpupriv->alias); VIR_STEAL_PTR(vcpupriv->alias, info[i].alias); + virJSONValueFree(vcpupriv->props); + VIR_STEAL_PTR(vcpupriv->props, info[i].props); vcpupriv->enable_id = info[i].id; vcpupriv->qemu_id = info[i].qemu_id; diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 37a00323a7..d097f23342 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -449,6 +449,9 @@ struct _qemuDomainVcpuPrivate { char *alias; virTristateBool halted; + /* copy of the data that qemu returned */ + virJSONValuePtr props; + /* information for hotpluggable cpus */ char *type; int socket_id; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index a880da3ab6..58ad109680 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1780,6 +1780,7 @@ qemuMonitorCPUInfoClear(qemuMonitorCPUInfoPtr cpus, VIR_FREE(cpus[i].qom_path); VIR_FREE(cpus[i].alias); VIR_FREE(cpus[i].type); + virJSONValueFree(cpus[i].props); } } @@ -1931,6 +1932,7 @@ qemuMonitorGetCPUInfoHotplug(struct qemuMonitorQueryHotpluggableCpusEntry *hotpl VIR_STEAL_PTR(vcpus[mastervcpu].qom_path, hotplugvcpus[i].qom_path); VIR_STEAL_PTR(vcpus[mastervcpu].alias, hotplugvcpus[i].alias); VIR_STEAL_PTR(vcpus[mastervcpu].type, hotplugvcpus[i].type); + VIR_STEAL_PTR(vcpus[mastervcpu].props, hotplugvcpus[i].props); vcpus[mastervcpu].id = hotplugvcpus[i].enable_id; /* copy state information to slave vcpus */ diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 88c9702530..de85a3ba0d 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -570,6 +570,9 @@ struct qemuMonitorQueryHotpluggableCpusEntry { char *qom_path; /* full device qom path only present for online cpus */ char *alias; /* device alias, may be NULL for non-hotpluggable entities */ + /* verbatim copy of the JSON data representing the CPU which must be used for hotplug */ + virJSONValuePtr props; + /* topology information -1 if qemu didn't report given parameter */ int node_id; int socket_id; @@ -603,6 +606,9 @@ struct _qemuMonitorCPUInfo { /* name of the qemu type to add in case of hotplug */ char *type; + /* verbatim copy of the returned data from qemu which should be used when plugging */ + virJSONValuePtr props; + /* alias of an hotpluggable entry. Entries with alias can be hot-unplugged */ char *alias; diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index d38b2f2cbe..da1e89dded 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -8378,6 +8378,7 @@ qemuMonitorQueryHotpluggableCpusFree(struct qemuMonitorQueryHotpluggableCpusEntr VIR_FREE(entry->type); VIR_FREE(entry->qom_path); VIR_FREE(entry->alias); + virJSONValueFree(entry->props); } VIR_FREE(entries); @@ -8426,6 +8427,9 @@ qemuMonitorJSONProcessHotpluggableCpusReply(virJSONValuePtr vcpu, return -1; } + if (!(entry->props = virJSONValueCopy(props))) + return -1; + entry->node_id = -1; entry->socket_id = -1; entry->core_id = -1; -- 2.21.0
As qemu documents we should use everything in the 'props' sub-object of the data returned by query-hotpluggable-cpus. Until now we only used everything we recognized, but that may break in cases when qemu introduces new fields. This change requires a fix to the test data as some fields were reordered. https://bugzilla.redhat.com/show_bug.cgi?id=1741658 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 19 +++---------------- .../x86-modern-bulk-monitor.json | 8 ++++---- .../x86-modern-individual-add-monitor.json | 4 ++-- 3 files changed, 9 insertions(+), 22 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 373ebd6d1a..1ca1ecd2f0 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -10609,24 +10609,11 @@ qemuBuildHotpluggableCPUProps(const virDomainVcpuDef *vcpu) qemuDomainVcpuPrivatePtr vcpupriv = QEMU_DOMAIN_VCPU_PRIVATE(vcpu); VIR_AUTOPTR(virJSONValue) ret = NULL; - if (virJSONValueObjectCreate(&ret, "s:driver", vcpupriv->type, - "s:id", vcpupriv->alias, NULL) < 0) + if (!(ret = virJSONValueCopy(vcpupriv->props))) return NULL; - if (vcpupriv->socket_id != -1 && - virJSONValueObjectAdd(ret, "i:socket-id", vcpupriv->socket_id, NULL) < 0) - return NULL; - - if (vcpupriv->core_id != -1 && - virJSONValueObjectAdd(ret, "i:core-id", vcpupriv->core_id, NULL) < 0) - return NULL; - - if (vcpupriv->thread_id != -1 && - virJSONValueObjectAdd(ret, "i:thread-id", vcpupriv->thread_id, NULL) < 0) - return NULL; - - if (vcpupriv->node_id != -1 && - virJSONValueObjectAdd(ret, "i:node-id", vcpupriv->node_id, NULL) < 0) + if (virJSONValueObjectPrependString(ret, "id", vcpupriv->alias) < 0 || + virJSONValueObjectPrependString(ret, "driver", vcpupriv->type) < 0) return NULL; VIR_RETURN_PTR(ret); diff --git a/tests/qemuhotplugtestcpus/x86-modern-bulk-monitor.json b/tests/qemuhotplugtestcpus/x86-modern-bulk-monitor.json index 4dcb6a2cf5..b1d02b8d43 100644 --- a/tests/qemuhotplugtestcpus/x86-modern-bulk-monitor.json +++ b/tests/qemuhotplugtestcpus/x86-modern-bulk-monitor.json @@ -141,9 +141,9 @@ "arguments": { "driver": "qemu64-x86_64-cpu", "id": "vcpu5", - "socket-id": 1, "core-id": 0, - "thread-id": 1 + "thread-id": 1, + "socket-id": 1 }, "id": "libvirt-3" } @@ -303,9 +303,9 @@ "arguments": { "driver": "qemu64-x86_64-cpu", "id": "vcpu6", - "socket-id": 1, "core-id": 1, - "thread-id": 0 + "thread-id": 0, + "socket-id": 1 }, "id": "libvirt-6" } diff --git a/tests/qemuhotplugtestcpus/x86-modern-individual-add-monitor.json b/tests/qemuhotplugtestcpus/x86-modern-individual-add-monitor.json index 294198b270..df05e5e3a9 100644 --- a/tests/qemuhotplugtestcpus/x86-modern-individual-add-monitor.json +++ b/tests/qemuhotplugtestcpus/x86-modern-individual-add-monitor.json @@ -141,9 +141,9 @@ "arguments": { "driver": "qemu64-x86_64-cpu", "id": "vcpu7", - "socket-id": 1, "core-id": 1, - "thread-id": 1 + "thread-id": 1, + "socket-id": 1 }, "id": "libvirt-3" } -- 2.21.0
On Thu, Aug 29, 2019 at 03:45:15PM +0200, Peter Krempa wrote:
qemu documents that we should pass in all fields of 'CpuInstanceProperties' back when hotplugging cpus, but we didn't do that. It turns out that qemu wanted to add new fields which were mandatory and thus broke the interop with libvirt.
Let's honor their reques.
s/reques/request/
Peter Krempa (2): qemu: Extract and store vCPU properties as qemu returned them qemu: command: Use all vCPU properties when creating args for vCPU hotplug
src/qemu/qemu_command.c | 19 +++---------------- src/qemu/qemu_domain.c | 3 +++ src/qemu/qemu_domain.h | 3 +++ src/qemu/qemu_monitor.c | 2 ++ src/qemu/qemu_monitor.h | 6 ++++++ src/qemu/qemu_monitor_json.c | 4 ++++ .../x86-modern-bulk-monitor.json | 8 ++++---- .../x86-modern-individual-add-monitor.json | 4 ++-- 8 files changed, 27 insertions(+), 22 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Peter Krempa