Re: [libvirt] [PATCH 07/10] qemu: domain: Improve vCPU data checking in qemuDomainDetectVcpuPids
$SUBJ
s/qemuDomainDetectVcpuPids/qemuDomainRefreshVcpuInfo/
On 08/03/2016 04:10 AM, Peter Krempa wrote:
Validate the presence of the thread id according to state of the
vCPU
rather than just checking the vCPU count. Additionally put the new
validation code into a separate function so that the information
retrieval can be split from the validation.
---
src/qemu/qemu_domain.c | 49 +++++++++++++++++++++++++++++++++++++++++++------
src/qemu/qemu_domain.h | 1 +
2 files changed, 44 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 5cde841..f27f2f7 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5624,6 +5624,48 @@ qemuDomainGetVcpuPid(virDomainObjPtr vm,
/**
+ * qemuDomainValidateVcpuInfo:
+ *
+ * Validates vcpu thread information. If vcpu thread IDs are reported by qemu,
+ * this function validates that online vcpus have thread info present and
+ * offline vcpus don't.
+ *
+ * Returns 0 on success -1 on error.
+ */
+int
+qemuDomainValidateVcpuInfo(virDomainObjPtr vm)
+{
+ size_t maxvcpus = virDomainDefGetVcpusMax(vm->def);
+ virDomainVcpuDefPtr vcpu;
+ qemuDomainVcpuPrivatePtr vcpupriv;
+ size_t i;
+
+ if (!qemuDomainHasVcpuPids(vm))
+ return 0;
This is checking priv->nvcpupids > 0, which we know as a truth since a
few lines before calling this function there's a check:
/* failure to get the VCPU <-> PID mapping or to execute the query
* command will not be treated fatal as some versions of qemu don't
* support this command */
if (ncpupids <= 0) {
Of course future patches "adjust" where this function is called to split
the Refresh and Validate into two parts and by patch 9 those lines
disappear. I guess it's just duplicitous for a few patches.
Still, by the time we reach patch 9 other than the VIR_DOMAIN_VIRT_QEMU
check - I wonder if we may want to make the subsequent checks since
there'd be no other way to get here since if the Refresh function
returns failure, this function wouldn't be called.
ACK to what's here with the adjustment for the commit message... maybe
I'm thinking too hard for this early in the morning ;-)
John
+
+ for (i = 0; i < maxvcpus; i++) {
+ vcpu = virDomainDefGetVcpu(vm->def, i);
+ vcpupriv = QEMU_DOMAIN_VCPU_PRIVATE(vcpu);
+
+ if (vcpu->online && vcpupriv->tid == 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("qemu didn't report thread id for vcpu
'%zu'"), i);
+ return -1;
+ }
+
+ if (!vcpu->online && vcpupriv->tid != 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("qemu reported thread if for inactive vcpu
'%zu'"),
+ i);
+ return -1;
+ }
+ }
+
+ return 0;
+}
+
+
+/**
* qemuDomainRefreshVcpuInfo:
* @driver: qemu driver data
* @vm: domain object
@@ -5703,13 +5745,8 @@ qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
QEMU_DOMAIN_VCPU_PRIVATE(vcpu)->tid = 0;
}
- if (ncpupids != virDomainDefGetVcpus(vm->def)) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("got wrong number of vCPU pids from QEMU monitor. "
- "got %d, wanted %d"),
- ncpupids, virDomainDefGetVcpus(vm->def));
+ if (qemuDomainValidateVcpuInfo(vm) < 0)
goto cleanup;
- }
ret = ncpupids;
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 3193427..0613093 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -647,6 +647,7 @@ int qemuDomainDefValidateMemoryHotplug(const virDomainDef *def,
bool qemuDomainHasVcpuPids(virDomainObjPtr vm);
pid_t qemuDomainGetVcpuPid(virDomainObjPtr vm, unsigned int vcpuid);
+int qemuDomainValidateVcpuInfo(virDomainObjPtr vm);
int qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
virDomainObjPtr vm,
int asyncJob);