3:10 a.m.
A few patches that are standalone enough which originated from my work in
progress on new way to do vcpu hotplug.
Peter Krempa (10):
qemu: monitor: Add monitor API for device_add supporting JSON objects
internal: Introduce macro for stealing pointers
qemu: monitor: Add do-while block to QEMU_CHECK_MONITOR_FULL
qemu: Improve error message in virDomainGetVcpus
qemu: domain: Rename qemuDomainDetectVcpuPids to
qemuDomainRefreshVcpuInfo
qemu: monitor: Rename qemuMonitor(JSON|Text)GetCPUInfo
qemu: domain: Improve vCPU data checking in qemuDomainDetectVcpuPids
qemu: domain: Simplify return values of qemuDomainRefreshVcpuInfo
qemu: monitor: Return structures from qemuMonitorGetCPUInfo
qemu: monitor: Return struct from qemuMonitor(Text|Json)QueryCPUs
src/internal.h | 12 +++++
src/qemu/qemu_domain.c | 95 ++++++++++++++++++++++++-------------
src/qemu/qemu_domain.h | 6 ++-
src/qemu/qemu_driver.c | 25 +++++-----
src/qemu/qemu_monitor.c | 110 ++++++++++++++++++++++++++++++++++++-------
src/qemu/qemu_monitor.h | 21 ++++++++-
src/qemu/qemu_monitor_json.c | 103 ++++++++++++++++++++++------------------
src/qemu/qemu_monitor_json.h | 6 ++-
src/qemu/qemu_monitor_text.c | 40 ++++++++--------
src/qemu/qemu_monitor_text.h | 4 +-
src/qemu/qemu_process.c | 12 +++--
tests/qemumonitorjsontest.c | 35 ++++++++++----
12 files changed, 316 insertions(+), 153 deletions(-)
--
2.9.2
3:10 a.m.
New subject: [libvirt] [PATCH 01/10] qemu: monitor: Add monitor API for device_add supporting JSON objects
Rather than formating a string and splitting it back to a JSON object
add API that will take a JSON object directly.
---
src/qemu/qemu_monitor.c | 18 ++++++++++++++++++
src/qemu/qemu_monitor.h | 2 ++
src/qemu/qemu_monitor_json.c | 29 +++++++++++++++++++----------
src/qemu/qemu_monitor_json.h | 2 ++
4 files changed, 41 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 58c04d5..b58c412 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -2771,6 +2771,24 @@ qemuMonitorAddDevice(qemuMonitorPtr mon,
/**
+ * qemuMonitorAddDeviceArgs:
+ * @mon: monitor object
+ * @args: arguments for device add, consumed on success or failure
+ *
+ * Adds a device described by @args. Requires JSON monitor.
+ * Returns 0 on success -1 on error.
+ */
+int
+qemuMonitorAddDeviceArgs(qemuMonitorPtr mon,
+ virJSONValuePtr args)
+{
+ QEMU_CHECK_MONITOR_JSON(mon);
+
+ return qemuMonitorJSONAddDeviceArgs(mon, args);
+}
+
+
+/**
* qemuMonitorAddObject:
* @mon: Pointer to monitor object
* @type: Type name of object to add
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index ae0954d..805656b 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -685,6 +685,8 @@ int qemuMonitorAttachPCIDiskController(qemuMonitorPtr mon,
const char *bus,
virPCIDeviceAddress *guestAddr);
+int qemuMonitorAddDeviceArgs(qemuMonitorPtr mon,
+ virJSONValuePtr args);
int qemuMonitorAddDevice(qemuMonitorPtr mon,
const char *devicestr);
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 5283024..cf55a61 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -3583,20 +3583,15 @@ int qemuMonitorJSONDelDevice(qemuMonitorPtr mon,
}
-int qemuMonitorJSONAddDevice(qemuMonitorPtr mon,
- const char *devicestr)
+int
+qemuMonitorJSONAddDeviceArgs(qemuMonitorPtr mon,
+ virJSONValuePtr args)
{
int ret = -1;
- virJSONValuePtr cmd;
+ virJSONValuePtr cmd = NULL;
virJSONValuePtr reply = NULL;
- virJSONValuePtr args;
- cmd = qemuMonitorJSONMakeCommand("device_add", NULL);
- if (!cmd)
- return -1;
-
- args = qemuMonitorJSONKeywordStringToJSON(devicestr, "driver");
- if (!args)
+ if (!(cmd = qemuMonitorJSONMakeCommand("device_add", NULL)))
goto cleanup;
if (virJSONValueObjectAppend(cmd, "arguments", args) < 0)
@@ -3618,6 +3613,20 @@ int qemuMonitorJSONAddDevice(qemuMonitorPtr mon,
}
+int
+qemuMonitorJSONAddDevice(qemuMonitorPtr mon,
+ const char *devicestr)
+{
+ virJSONValuePtr args;
+
+ if (!(args = qemuMonitorJSONKeywordStringToJSON(devicestr, "driver")))
+ return -1;
+
+ /* qemuMonitorJSONAddDeviceArgs always consumes @args */
+ return qemuMonitorJSONAddDeviceArgs(mon, args);
+}
+
+
int qemuMonitorJSONAddObject(qemuMonitorPtr mon,
const char *type,
const char *objalias,
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index 0b3d898..7f3222a 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -215,6 +215,8 @@ int qemuMonitorJSONAttachPCIDiskController(qemuMonitorPtr mon,
const char *bus,
virPCIDeviceAddress *guestAddr);
+int qemuMonitorJSONAddDeviceArgs(qemuMonitorPtr mon,
+ virJSONValuePtr args);
int qemuMonitorJSONAddDevice(qemuMonitorPtr mon,
const char *devicestr);
--
2.9.2
5:43 a.m.
New subject: [libvirt] [PATCH 01/10] qemu: monitor: Add monitor API for device_add supporting JSON objects
On Wed, Aug 03, 2016 at 10:10:53AM +0200, Peter Krempa wrote:
Rather than formating a string and splitting it back to a JSON
object
add API that will take a JSON object directly.
---
src/qemu/qemu_monitor.c | 18 ++++++++++++++++++
src/qemu/qemu_monitor.h | 2 ++
src/qemu/qemu_monitor_json.c | 29 +++++++++++++++++++----------
src/qemu/qemu_monitor_json.h | 2 ++
4 files changed, 41 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 58c04d5..b58c412 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -2771,6 +2771,24 @@ qemuMonitorAddDevice(qemuMonitorPtr mon,
/**
+ * qemuMonitorAddDeviceArgs:
+ * @mon: monitor object
+ * @args: arguments for device add, consumed on success or failure
+ *
+ * Adds a device described by @args. Requires JSON monitor.
+ * Returns 0 on success -1 on error.
+ */
+int
+qemuMonitorAddDeviceArgs(qemuMonitorPtr mon,
+ virJSONValuePtr args)
+{
+ QEMU_CHECK_MONITOR_JSON(mon);
+
+ return qemuMonitorJSONAddDeviceArgs(mon, args);
+}
+
+
+/**
* qemuMonitorAddObject:
* @mon: Pointer to monitor object
* @type: Type name of object to add
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index ae0954d..805656b 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -685,6 +685,8 @@ int qemuMonitorAttachPCIDiskController(qemuMonitorPtr mon,
const char *bus,
virPCIDeviceAddress *guestAddr);
+int qemuMonitorAddDeviceArgs(qemuMonitorPtr mon,
+ virJSONValuePtr args);
int qemuMonitorAddDevice(qemuMonitorPtr mon,
const char *devicestr);
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 5283024..cf55a61 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -3583,20 +3583,15 @@ int qemuMonitorJSONDelDevice(qemuMonitorPtr mon,
}
-int qemuMonitorJSONAddDevice(qemuMonitorPtr mon,
- const char *devicestr)
+int
+qemuMonitorJSONAddDeviceArgs(qemuMonitorPtr mon,
+ virJSONValuePtr args)
{
int ret = -1;
- virJSONValuePtr cmd;
+ virJSONValuePtr cmd = NULL;
virJSONValuePtr reply = NULL;
- virJSONValuePtr args;
- cmd = qemuMonitorJSONMakeCommand("device_add", NULL);
- if (!cmd)
- return -1;
-
- args = qemuMonitorJSONKeywordStringToJSON(devicestr, "driver");
- if (!args)
+ if (!(cmd = qemuMonitorJSONMakeCommand("device_add", NULL)))
goto cleanup;
if (virJSONValueObjectAppend(cmd, "arguments", args) < 0)
@@ -3618,6 +3613,20 @@ int qemuMonitorJSONAddDevice(qemuMonitorPtr mon,
}
+int
+qemuMonitorJSONAddDevice(qemuMonitorPtr mon,
+ const char *devicestr)
+{
+ virJSONValuePtr args;
+
+ if (!(args = qemuMonitorJSONKeywordStringToJSON(devicestr, "driver")))
+ return -1;
+
+ /* qemuMonitorJSONAddDeviceArgs always consumes @args */
This comment is redundant, the same information is in the function doc.
+ return qemuMonitorJSONAddDeviceArgs(mon, args);
+}
+
+
int qemuMonitorJSONAddObject(qemuMonitorPtr mon,
const char *type,
const char *objalias,
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index 0b3d898..7f3222a 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -215,6 +215,8 @@ int qemuMonitorJSONAttachPCIDiskController(qemuMonitorPtr mon,
const char *bus,
virPCIDeviceAddress *guestAddr);
+int qemuMonitorJSONAddDeviceArgs(qemuMonitorPtr mon,
+ virJSONValuePtr args);
int qemuMonitorJSONAddDevice(qemuMonitorPtr mon,
const char *devicestr);
ACK
5:54 a.m.
New subject: [libvirt] [PATCH 01/10] qemu: monitor: Add monitor API for device_add supporting JSON objects
On Wed, Aug 03, 2016 at 12:43:48PM +0200, Pavel Hrdina wrote:
On Wed, Aug 03, 2016 at 10:10:53AM +0200, Peter Krempa wrote:
> Rather than formating a string and splitting it back to a JSON object
> add API that will take a JSON object directly.
> ---
> src/qemu/qemu_monitor.c | 18 ++++++++++++++++++
> src/qemu/qemu_monitor.h | 2 ++
> src/qemu/qemu_monitor_json.c | 29 +++++++++++++++++++----------
> src/qemu/qemu_monitor_json.h | 2 ++
> 4 files changed, 41 insertions(+), 10 deletions(-)
>
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 58c04d5..b58c412 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -2771,6 +2771,24 @@ qemuMonitorAddDevice(qemuMonitorPtr mon,
>
>
> /**
> + * qemuMonitorAddDeviceArgs:
> + * @mon: monitor object
> + * @args: arguments for device add, consumed on success or failure
> + *
> + * Adds a device described by @args. Requires JSON monitor.
> + * Returns 0 on success -1 on error.
> + */
> +int
> +qemuMonitorAddDeviceArgs(qemuMonitorPtr mon,
> + virJSONValuePtr args)
> +{
> + QEMU_CHECK_MONITOR_JSON(mon);
> +
> + return qemuMonitorJSONAddDeviceArgs(mon, args);
> +}
> +
> +
> +/**
> * qemuMonitorAddObject:
> * @mon: Pointer to monitor object
> * @type: Type name of object to add
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index ae0954d..805656b 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -685,6 +685,8 @@ int qemuMonitorAttachPCIDiskController(qemuMonitorPtr mon,
> const char *bus,
> virPCIDeviceAddress *guestAddr);
>
> +int qemuMonitorAddDeviceArgs(qemuMonitorPtr mon,
> + virJSONValuePtr args);
I've missed this one, wrong indentation.
8:09 a.m.
New subject: [libvirt] [PATCH 01/10] qemu: monitor: Add monitor API for device_add supporting JSON objects
On Wed, Aug 03, 2016 at 10:10:53AM +0200, Peter Krempa wrote:
Rather than formating a string and splitting it back to a JSON object
s/formating/formatting/
Jan
add API that will take a JSON object directly.
---
src/qemu/qemu_monitor.c | 18 ++++++++++++++++++
src/qemu/qemu_monitor.h | 2 ++
src/qemu/qemu_monitor_json.c | 29 +++++++++++++++++++----------
src/qemu/qemu_monitor_json.h | 2 ++
4 files changed, 41 insertions(+), 10 deletions(-)
3:10 a.m.
New subject: [libvirt] [PATCH 02/10] internal: Introduce macro for stealing pointers
VIR_STEAL copies the second argument into the first and then sets it to
NULL. This is useful for stealing pointers.
---
src/internal.h | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/src/internal.h b/src/internal.h
index 0dc34c7..c633ee6 100644
--- a/src/internal.h
+++ b/src/internal.h
@@ -307,6 +307,18 @@
} while (0)
/**
+ * VIR_STEAL:
+ *
+ * Steals pointer passed as second argument into the first argument. Second
+ * argument must not have side effects.
+ */
+# define VIR_STEAL(a, b) \
+ do { \
+ (a) = (b); \
+ (b) = NULL;\
+ } while (0)
+
+/**
* virCheckFlags:
* @supported: an OR'ed set of supported flags
* @retval: return value in case unsupported flags were passed
--
2.9.2
5:48 a.m.
New subject: [libvirt] [PATCH 02/10] internal: Introduce macro for stealing pointers
On Wed, Aug 03, 2016 at 10:10:54AM +0200, Peter Krempa wrote:
VIR_STEAL copies the second argument into the first and then sets it
to
NULL. This is useful for stealing pointers.
---
src/internal.h | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/src/internal.h b/src/internal.h
index 0dc34c7..c633ee6 100644
--- a/src/internal.h
+++ b/src/internal.h
@@ -307,6 +307,18 @@
} while (0)
/**
+ * VIR_STEAL:
+ *
+ * Steals pointer passed as second argument into the first argument. Second
+ * argument must not have side effects.
+ */
+# define VIR_STEAL(a, b) \
+ do { \
+ (a) = (b); \
+ (b) = NULL;\
+ } while (0)
I guess that there is no harm having this macro even though it's only two lines
of code.
I would align the backslash to the same column as we tend to do in libvirt code.
ACK
6:14 a.m.
New subject: [libvirt] [PATCH 02/10] internal: Introduce macro for stealing pointers
On 08/03/2016 04:10 AM, Peter Krempa wrote:
VIR_STEAL copies the second argument into the first and then sets it
to
NULL. This is useful for stealing pointers.
---
src/internal.h | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/src/internal.h b/src/internal.h
index 0dc34c7..c633ee6 100644
--- a/src/internal.h
+++ b/src/internal.h
@@ -307,6 +307,18 @@
} while (0)
/**
+ * VIR_STEAL:
+ *
+ * Steals pointer passed as second argument into the first argument. Second
+ * argument must not have side effects.
+ */
+# define VIR_STEAL(a, b) \
+ do { \
+ (a) = (b); \
+ (b) = NULL;\
+ } while (0)
+
+/**
* virCheckFlags:
* @supported: an OR'ed set of supported flags
* @retval: return value in case unsupported flags were passed
While one can look at the code and deduce what it does - the name is not
that descriptive. VIR_ASSIGN_PTR or VIR_ASSIGN_POINTER? Just a thought...
It's also not used until patch 9 - perhaps move it a bit closer to that.
John
3:10 a.m.
New subject: [libvirt] [PATCH 03/10] qemu: monitor: Add do-while block to QEMU_CHECK_MONITOR_FULL
Assure that it's just one statement to avoid problems when used with
conditions.
---
src/qemu/qemu_monitor.c | 25 ++++++++++++++-----------
1 file changed, 14 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index b58c412..1ef18dd 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -112,17 +112,20 @@ struct _qemuMonitor {
* monitor.
*/
#define QEMU_CHECK_MONITOR_FULL(mon, force_json, exit) \
- if (!mon) { \
- virReportError(VIR_ERR_INVALID_ARG, "%s",
\
- _("monitor must not be NULL"));
\
- exit; \
- } \
- VIR_DEBUG("mon:%p vm:%p json:%d fd:%d", mon, mon->vm, mon->json,
mon->fd); \
- if (force_json && !mon->json) {
\
- virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
\
- _("JSON monitor is required"));
\
- exit; \
- }
+ do { \
+ if (!mon) { \
+ virReportError(VIR_ERR_INVALID_ARG, "%s",
\
+ _("monitor must not be NULL"));
\
+ exit; \
+ } \
+ VIR_DEBUG("mon:%p vm:%p json:%d fd:%d",
\
+ mon, mon->vm, mon->json, mon->fd);
\
+ if (force_json && !mon->json) {
\
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
\
+ _("JSON monitor is required"));
\
+ exit; \
+ } \
+ } while (0)
/* Check monitor and return NULL on error */
#define QEMU_CHECK_MONITOR_NULL(mon) \
--
2.9.2
5:49 a.m.
New subject: [libvirt] [PATCH 03/10] qemu: monitor: Add do-while block to QEMU_CHECK_MONITOR_FULL
On Wed, Aug 03, 2016 at 10:10:55AM +0200, Peter Krempa wrote:
Assure that it's just one statement to avoid problems when used
with
conditions.
---
src/qemu/qemu_monitor.c | 25 ++++++++++++++-----------
1 file changed, 14 insertions(+), 11 deletions(-)
ACK
3:10 a.m.
New subject: [libvirt] [PATCH 04/10] qemu: Improve error message in virDomainGetVcpus
If the VM is offline we can't retrieve the runtime statistical
information. Pinning could be retrieved but there are separate APIs for
that.
---
src/qemu/qemu_driver.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 11db9a5..061c16f 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5284,9 +5284,8 @@ qemuDomainGetVcpus(virDomainPtr dom,
goto cleanup;
if (!virDomainObjIsActive(vm)) {
- virReportError(VIR_ERR_OPERATION_INVALID,
- "%s",
- _("cannot list vcpu pinning for an inactive domain"));
+ virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("cannot retrieve vcpu information for inactive
domain"));
goto cleanup;
}
--
2.9.2
5:50 a.m.
New subject: [libvirt] [PATCH 04/10] qemu: Improve error message in virDomainGetVcpus
On Wed, Aug 03, 2016 at 10:10:56AM +0200, Peter Krempa wrote:
If the VM is offline we can't retrieve the runtime statistical
information. Pinning could be retrieved but there are separate APIs for
that.
---
src/qemu/qemu_driver.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
ACK
3:10 a.m.
New subject: [libvirt] [PATCH 05/10] qemu: domain: Rename qemuDomainDetectVcpuPids to qemuDomainRefreshVcpuInfo
The function will eventually do more useful stuff than just detection of
thread ids.
---
src/qemu/qemu_domain.c | 10 +++++-----
src/qemu/qemu_domain.h | 5 +++--
src/qemu/qemu_driver.c | 4 ++--
src/qemu/qemu_process.c | 6 +++---
4 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index efc1fb5..5cde841 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5624,20 +5624,20 @@ qemuDomainGetVcpuPid(virDomainObjPtr vm,
/**
- * qemuDomainDetectVcpuPids:
+ * qemuDomainRefreshVcpuInfo:
* @driver: qemu driver data
* @vm: domain object
* @asyncJob: current asynchronous job type
*
- * Updates vCPU thread ids in the private data of @vm.
+ * Updates vCPU information private data of @vm.
*
* Returns number of detected vCPU threads on success, -1 on error and reports
* an appropriate error, -2 if the domain doesn't exist any more.
*/
int
-qemuDomainDetectVcpuPids(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- int asyncJob)
+qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ int asyncJob)
{
virDomainVcpuDefPtr vcpu;
size_t maxvcpus = virDomainDefGetVcpusMax(vm->def);
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 9e10ae4..3193427 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -647,8 +647,9 @@ int qemuDomainDefValidateMemoryHotplug(const virDomainDef *def,
bool qemuDomainHasVcpuPids(virDomainObjPtr vm);
pid_t qemuDomainGetVcpuPid(virDomainObjPtr vm, unsigned int vcpuid);
-int qemuDomainDetectVcpuPids(virQEMUDriverPtr driver, virDomainObjPtr vm,
- int asyncJob);
+int qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ int asyncJob);
bool qemuDomainSupportsNicdev(virDomainDefPtr def,
virDomainNetDefPtr net);
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 061c16f..453572b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4639,7 +4639,7 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver,
vcpuinfo->online = true;
- if ((rc = qemuDomainDetectVcpuPids(driver, vm, QEMU_ASYNC_JOB_NONE)) <= 0) {
+ if ((rc = qemuDomainRefreshVcpuInfo(driver, vm, QEMU_ASYNC_JOB_NONE)) <= 0) {
/* vcpu pids were not detected, skip setting of affinity */
if (rc == 0)
ret = 0;
@@ -4689,7 +4689,7 @@ qemuDomainHotplugDelVcpu(virQEMUDriverPtr driver,
goto cleanup;
}
- if ((rc = qemuDomainDetectVcpuPids(driver, vm, QEMU_ASYNC_JOB_NONE)) < 0) {
+ if ((rc = qemuDomainRefreshVcpuInfo(driver, vm, QEMU_ASYNC_JOB_NONE)) < 0) {
/* rollback only if domain didn't exit */
if (rc == -2)
goto cleanup;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index a4935d2..9136ba5 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -5189,8 +5189,8 @@ qemuProcessLaunch(virConnectPtr conn,
if (qemuSetupCpusetMems(vm) < 0)
goto cleanup;
- VIR_DEBUG("Detecting VCPU PIDs");
- if (qemuDomainDetectVcpuPids(driver, vm, asyncJob) < 0)
+ VIR_DEBUG("Refreshing VCPU info");
+ if (qemuDomainRefreshVcpuInfo(driver, vm, asyncJob) < 0)
goto cleanup;
VIR_DEBUG("Detecting IOThread PIDs");
@@ -5982,7 +5982,7 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED,
}
VIR_DEBUG("Detecting VCPU PIDs");
- if (qemuDomainDetectVcpuPids(driver, vm, QEMU_ASYNC_JOB_NONE) < 0)
+ if (qemuDomainRefreshVcpuInfo(driver, vm, QEMU_ASYNC_JOB_NONE) < 0)
goto error;
VIR_DEBUG("Detecting IOThread PIDs");
--
2.9.2
5:52 a.m.
New subject: [libvirt] [PATCH 05/10] qemu: domain: Rename qemuDomainDetectVcpuPids to qemuDomainRefreshVcpuInfo
On Wed, Aug 03, 2016 at 10:10:57AM +0200, Peter Krempa wrote:
The function will eventually do more useful stuff than just detection
of
thread ids.
---
src/qemu/qemu_domain.c | 10 +++++-----
src/qemu/qemu_domain.h | 5 +++--
src/qemu/qemu_driver.c | 4 ++--
src/qemu/qemu_process.c | 6 +++---
4 files changed, 13 insertions(+), 12 deletions(-)
ACK
3:10 a.m.
New subject: [libvirt] [PATCH 06/10] qemu: monitor: Rename qemuMonitor(JSON|Text)GetCPUInfo
Use a name that contains the command used to get the information.
---
src/qemu/qemu_monitor.c | 4 ++--
src/qemu/qemu_monitor_json.c | 5 +++--
src/qemu/qemu_monitor_json.h | 4 ++--
src/qemu/qemu_monitor_text.c | 5 +++--
src/qemu/qemu_monitor_text.h | 4 ++--
tests/qemumonitorjsontest.c | 6 +++---
6 files changed, 15 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 1ef18dd..4403bdd 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -1672,9 +1672,9 @@ qemuMonitorGetCPUInfo(qemuMonitorPtr mon,
QEMU_CHECK_MONITOR(mon);
if (mon->json)
- return qemuMonitorJSONGetCPUInfo(mon, pids);
+ return qemuMonitorJSONQueryCPUs(mon, pids);
else
- return qemuMonitorTextGetCPUInfo(mon, pids);
+ return qemuMonitorTextQueryCPUs(mon, pids);
}
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index cf55a61..715e1c7 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -1376,8 +1376,9 @@ qemuMonitorJSONExtractCPUInfo(virJSONValuePtr reply,
}
-int qemuMonitorJSONGetCPUInfo(qemuMonitorPtr mon,
- int **pids)
+int
+qemuMonitorJSONQueryCPUs(qemuMonitorPtr mon,
+ int **pids)
{
int ret = -1;
virJSONValuePtr cmd = qemuMonitorJSONMakeCommand("query-cpus",
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index 7f3222a..174f0ef 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -58,8 +58,8 @@ int qemuMonitorJSONGetStatus(qemuMonitorPtr mon,
int qemuMonitorJSONSystemPowerdown(qemuMonitorPtr mon);
int qemuMonitorJSONSystemReset(qemuMonitorPtr mon);
-int qemuMonitorJSONGetCPUInfo(qemuMonitorPtr mon,
- int **pids);
+int qemuMonitorJSONQueryCPUs(qemuMonitorPtr mon,
+ int **pids);
int qemuMonitorJSONGetVirtType(qemuMonitorPtr mon,
virDomainVirtType *virtType);
int qemuMonitorJSONUpdateVideoMemorySize(qemuMonitorPtr mon,
diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c
index 2c31b52..ca04965 100644
--- a/src/qemu/qemu_monitor_text.c
+++ b/src/qemu/qemu_monitor_text.c
@@ -500,8 +500,9 @@ int qemuMonitorTextSystemReset(qemuMonitorPtr mon)
}
-int qemuMonitorTextGetCPUInfo(qemuMonitorPtr mon,
- int **pids)
+int
+qemuMonitorTextQueryCPUs(qemuMonitorPtr mon,
+ int **pids)
{
char *qemucpus = NULL;
char *line;
diff --git a/src/qemu/qemu_monitor_text.h b/src/qemu/qemu_monitor_text.h
index eeaca52..b7f0cab 100644
--- a/src/qemu/qemu_monitor_text.h
+++ b/src/qemu/qemu_monitor_text.h
@@ -49,8 +49,8 @@ int qemuMonitorTextGetStatus(qemuMonitorPtr mon,
int qemuMonitorTextSystemPowerdown(qemuMonitorPtr mon);
int qemuMonitorTextSystemReset(qemuMonitorPtr mon);
-int qemuMonitorTextGetCPUInfo(qemuMonitorPtr mon,
- int **pids);
+int qemuMonitorTextQueryCPUs(qemuMonitorPtr mon,
+ int **pids);
int qemuMonitorTextGetVirtType(qemuMonitorPtr mon,
virDomainVirtType *virtType);
int qemuMonitorTextGetBalloonInfo(qemuMonitorPtr mon,
diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
index e8946c2..544b569 100644
--- a/tests/qemumonitorjsontest.c
+++ b/tests/qemumonitorjsontest.c
@@ -1203,7 +1203,7 @@ GEN_TEST_FUNC(qemuMonitorJSONDetachCharDev, "serial1")
static int
-testQemuMonitorJSONqemuMonitorJSONGetCPUInfo(const void *data)
+testQemuMonitorJSONqemuMonitorJSONQueryCPUs(const void *data)
{
virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data;
qemuMonitorTestPtr test = qemuMonitorTestNewSimple(true, xmlopt);
@@ -1252,7 +1252,7 @@ testQemuMonitorJSONqemuMonitorJSONGetCPUInfo(const void *data)
"}") < 0)
goto cleanup;
- ncpupids = qemuMonitorJSONGetCPUInfo(qemuMonitorTestGetMonitor(test), &cpupids);
+ ncpupids = qemuMonitorJSONQueryCPUs(qemuMonitorTestGetMonitor(test), &cpupids);
if (ncpupids != 4) {
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -2420,7 +2420,7 @@ mymain(void)
DO_TEST(qemuMonitorJSONSetBlockIoThrottle);
DO_TEST(qemuMonitorJSONGetTargetArch);
DO_TEST(qemuMonitorJSONGetMigrationCapability);
- DO_TEST(qemuMonitorJSONGetCPUInfo);
+ DO_TEST(qemuMonitorJSONQueryCPUs);
DO_TEST(qemuMonitorJSONGetVirtType);
DO_TEST(qemuMonitorJSONSendKey);
DO_TEST(qemuMonitorJSONGetDumpGuestMemoryCapability);
--
2.9.2
5:56 a.m.
New subject: [libvirt] [PATCH 06/10] qemu: monitor: Rename qemuMonitor(JSON|Text)GetCPUInfo
On Wed, Aug 03, 2016 at 10:10:58AM +0200, Peter Krempa wrote:
Use a name that contains the command used to get the information.
---
src/qemu/qemu_monitor.c | 4 ++--
src/qemu/qemu_monitor_json.c | 5 +++--
src/qemu/qemu_monitor_json.h | 4 ++--
src/qemu/qemu_monitor_text.c | 5 +++--
src/qemu/qemu_monitor_text.h | 4 ++--
tests/qemumonitorjsontest.c | 6 +++---
6 files changed, 15 insertions(+), 13 deletions(-)
ACK
3:10 a.m.
New subject: [libvirt] [PATCH 07/10] qemu: domain: Improve vCPU data checking in qemuDomainDetectVcpuPids
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;
+
+ 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);
--
2.9.2
7:54 a.m.
New subject: [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);
8 a.m.
New subject: [libvirt] [PATCH 07/10] qemu: domain: Improve vCPU data checking in qemuDomainDetectVcpuPids
On Wed, Aug 03, 2016 at 10:10:59AM +0200, Peter Krempa wrote:
s/qemuDomainDetectVcpuPids/qemuDomainRefreshVcpu/ in $subject
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(-)
ACK
3:11 a.m.
New subject: [libvirt] [PATCH 08/10] qemu: domain: Simplify return values of qemuDomainRefreshVcpuInfo
Call the vcpu thread info validation separately to decrease complexity
of returned values by qemuDomainRefreshVcpuInfo.
This function now returns 0 on success and -1 on error. Certain
failures of qemu to report data are still considered as success. Any
error reported now is fatal.
---
src/qemu/qemu_domain.c | 16 +++++-----------
src/qemu/qemu_driver.c | 20 ++++++++++----------
src/qemu/qemu_process.c | 6 ++++++
3 files changed, 21 insertions(+), 21 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index f27f2f7..4cdd012 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5671,10 +5671,10 @@ qemuDomainValidateVcpuInfo(virDomainObjPtr vm)
* @vm: domain object
* @asyncJob: current asynchronous job type
*
- * Updates vCPU information private data of @vm.
+ * Updates vCPU information private data of @vm. Due to historical reasons this
+ * function returns success even if some data were not reported by qemu.
*
- * Returns number of detected vCPU threads on success, -1 on error and reports
- * an appropriate error, -2 if the domain doesn't exist any more.
+ * Returns 0 on success and -1 on fatal error.
*/
int
qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
@@ -5722,10 +5722,7 @@ qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
return -1;
ncpupids = qemuMonitorGetCPUInfo(qemuDomainGetMonitor(vm), &cpupids);
- if (qemuDomainObjExitMonitor(driver, vm) < 0) {
- ret = -2;
- goto cleanup;
- }
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
/* 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
@@ -5745,10 +5742,7 @@ qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
QEMU_DOMAIN_VCPU_PRIVATE(vcpu)->tid = 0;
}
- if (qemuDomainValidateVcpuInfo(vm) < 0)
- goto cleanup;
-
- ret = ncpupids;
+ ret = 0;
cleanup:
VIR_FREE(cpupids);
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 453572b..2199258 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4615,6 +4615,7 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver,
{
qemuDomainObjPrivatePtr priv = vm->privateData;
virDomainVcpuDefPtr vcpuinfo = virDomainDefGetVcpu(vm->def, vcpu);
+ qemuDomainVcpuPrivatePtr vcpupriv = QEMU_DOMAIN_VCPU_PRIVATE(vcpuinfo);
int ret = -1;
int rc;
int oldvcpus = virDomainDefGetVcpus(vm->def);
@@ -4639,15 +4640,14 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver,
vcpuinfo->online = true;
- if ((rc = qemuDomainRefreshVcpuInfo(driver, vm, QEMU_ASYNC_JOB_NONE)) <= 0) {
- /* vcpu pids were not detected, skip setting of affinity */
- if (rc == 0)
- ret = 0;
+ if (qemuDomainRefreshVcpuInfo(driver, vm, QEMU_ASYNC_JOB_NONE) < 0)
+ goto cleanup;
+ if (qemuDomainValidateVcpuInfo(vm) < 0)
goto cleanup;
- }
- if (qemuProcessSetupVcpu(vm, vcpu) < 0)
+ if (vcpupriv->tid > 0 &&
+ qemuProcessSetupVcpu(vm, vcpu) < 0)
goto cleanup;
ret = 0;
@@ -4689,11 +4689,11 @@ qemuDomainHotplugDelVcpu(virQEMUDriverPtr driver,
goto cleanup;
}
- if ((rc = qemuDomainRefreshVcpuInfo(driver, vm, QEMU_ASYNC_JOB_NONE)) < 0) {
- /* rollback only if domain didn't exit */
- if (rc == -2)
- goto cleanup;
+ if (qemuDomainRefreshVcpuInfo(driver, vm, QEMU_ASYNC_JOB_NONE) < 0)
+ goto cleanup;
+ if (qemuDomainValidateVcpuInfo(vm) < 0) {
+ /* rollback vcpu count if the setting has failed */
virDomainAuditVcpu(vm, oldvcpus, oldvcpus - 1, "update", false);
vcpuinfo->online = true;
goto cleanup;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 9136ba5..2e405af 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -5193,6 +5193,9 @@ qemuProcessLaunch(virConnectPtr conn,
if (qemuDomainRefreshVcpuInfo(driver, vm, asyncJob) < 0)
goto cleanup;
+ if (qemuDomainValidateVcpuInfo(vm) < 0)
+ goto cleanup;
+
VIR_DEBUG("Detecting IOThread PIDs");
if (qemuProcessDetectIOThreadPIDs(driver, vm, asyncJob) < 0)
goto cleanup;
@@ -5985,6 +5988,9 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED,
if (qemuDomainRefreshVcpuInfo(driver, vm, QEMU_ASYNC_JOB_NONE) < 0)
goto error;
+ if (qemuDomainValidateVcpuInfo(vm) < 0)
+ goto error;
+
VIR_DEBUG("Detecting IOThread PIDs");
if (qemuProcessDetectIOThreadPIDs(driver, vm, QEMU_ASYNC_JOB_NONE) < 0)
goto error;
--
2.9.2
8:03 a.m.
New subject: [libvirt] [PATCH 08/10] qemu: domain: Simplify return values of qemuDomainRefreshVcpuInfo
On 08/03/2016 04:11 AM, Peter Krempa wrote:
Call the vcpu thread info validation separately to decrease
complexity
of returned values by qemuDomainRefreshVcpuInfo.
This function now returns 0 on success and -1 on error. Certain
failures of qemu to report data are still considered as success. Any
error reported now is fatal.
---
src/qemu/qemu_domain.c | 16 +++++-----------
src/qemu/qemu_driver.c | 20 ++++++++++----------
src/qemu/qemu_process.c | 6 ++++++
3 files changed, 21 insertions(+), 21 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index f27f2f7..4cdd012 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5671,10 +5671,10 @@ qemuDomainValidateVcpuInfo(virDomainObjPtr vm)
* @vm: domain object
* @asyncJob: current asynchronous job type
*
- * Updates vCPU information private data of @vm.
+ * Updates vCPU information private data of @vm. Due to historical reasons this
+ * function returns success even if some data were not reported by qemu.
*
- * Returns number of detected vCPU threads on success, -1 on error and reports
- * an appropriate error, -2 if the domain doesn't exist any more.
+ * Returns 0 on success and -1 on fatal error.
*/
int
qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
@@ -5722,10 +5722,7 @@ qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
return -1;
ncpupids = qemuMonitorGetCPUInfo(qemuDomainGetMonitor(vm), &cpupids);
- if (qemuDomainObjExitMonitor(driver, vm) < 0) {
- ret = -2;
- goto cleanup;
- }
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
This is missing a "goto cleanup" which is added in the next patch, but
needs to be here!
/* 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
@@ -5745,10 +5742,7 @@ qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
QEMU_DOMAIN_VCPU_PRIVATE(vcpu)->tid = 0;
}
- if (qemuDomainValidateVcpuInfo(vm) < 0)
- goto cleanup;
-
- ret = ncpupids;
+ ret = 0;
cleanup:
VIR_FREE(cpupids);
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 453572b..2199258 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4615,6 +4615,7 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver,
{
qemuDomainObjPrivatePtr priv = vm->privateData;
virDomainVcpuDefPtr vcpuinfo = virDomainDefGetVcpu(vm->def, vcpu);
+ qemuDomainVcpuPrivatePtr vcpupriv = QEMU_DOMAIN_VCPU_PRIVATE(vcpuinfo);
int ret = -1;
int rc;
int oldvcpus = virDomainDefGetVcpus(vm->def);
@@ -4639,15 +4640,14 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver,
vcpuinfo->online = true;
- if ((rc = qemuDomainRefreshVcpuInfo(driver, vm, QEMU_ASYNC_JOB_NONE)) <= 0) {
- /* vcpu pids were not detected, skip setting of affinity */
- if (rc == 0)
- ret = 0;
+ if (qemuDomainRefreshVcpuInfo(driver, vm, QEMU_ASYNC_JOB_NONE) < 0)
+ goto cleanup;
+ if (qemuDomainValidateVcpuInfo(vm) < 0)
goto cleanup;
- }
- if (qemuProcessSetupVcpu(vm, vcpu) < 0)
+ if (vcpupriv->tid > 0 &&
+ qemuProcessSetupVcpu(vm, vcpu) < 0)
goto cleanup;
ret = 0;
@@ -4689,11 +4689,11 @@ qemuDomainHotplugDelVcpu(virQEMUDriverPtr driver,
goto cleanup;
}
- if ((rc = qemuDomainRefreshVcpuInfo(driver, vm, QEMU_ASYNC_JOB_NONE)) < 0) {
- /* rollback only if domain didn't exit */
- if (rc == -2)
- goto cleanup;
+ if (qemuDomainRefreshVcpuInfo(driver, vm, QEMU_ASYNC_JOB_NONE) < 0)
+ goto cleanup;
Typing what I'm thinking - just to be sure it matches your expectations
of this code. So we no longer care to distinguish between failure and
failure due to domain exit. This just seems to mean we'll get that audit
after a possible domain exit and reset the vcpuinfo->online (won't
really matter if the domain exited).
ACK - with the goto cleanup added at least.
John
+ if (qemuDomainValidateVcpuInfo(vm) < 0) {
+ /* rollback vcpu count if the setting has failed */
virDomainAuditVcpu(vm, oldvcpus, oldvcpus - 1, "update", false);
vcpuinfo->online = true;
goto cleanup;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 9136ba5..2e405af 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -5193,6 +5193,9 @@ qemuProcessLaunch(virConnectPtr conn,
if (qemuDomainRefreshVcpuInfo(driver, vm, asyncJob) < 0)
goto cleanup;
+ if (qemuDomainValidateVcpuInfo(vm) < 0)
+ goto cleanup;
+
VIR_DEBUG("Detecting IOThread PIDs");
if (qemuProcessDetectIOThreadPIDs(driver, vm, asyncJob) < 0)
goto cleanup;
@@ -5985,6 +5988,9 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED,
if (qemuDomainRefreshVcpuInfo(driver, vm, QEMU_ASYNC_JOB_NONE) < 0)
goto error;
+ if (qemuDomainValidateVcpuInfo(vm) < 0)
+ goto error;
+
VIR_DEBUG("Detecting IOThread PIDs");
if (qemuProcessDetectIOThreadPIDs(driver, vm, QEMU_ASYNC_JOB_NONE) < 0)
goto error;
8:58 a.m.
New subject: [libvirt] [PATCH 08/10] qemu: domain: Simplify return values of qemuDomainRefreshVcpuInfo
On Wed, Aug 03, 2016 at 09:03:50AM -0400, John Ferlan wrote:
On 08/03/2016 04:11 AM, Peter Krempa wrote:
> Call the vcpu thread info validation separately to decrease complexity
> of returned values by qemuDomainRefreshVcpuInfo.
>
> This function now returns 0 on success and -1 on error. Certain
> failures of qemu to report data are still considered as success. Any
> error reported now is fatal.
> ---
> src/qemu/qemu_domain.c | 16 +++++-----------
> src/qemu/qemu_driver.c | 20 ++++++++++----------
> src/qemu/qemu_process.c | 6 ++++++
> 3 files changed, 21 insertions(+), 21 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index f27f2f7..4cdd012 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -5671,10 +5671,10 @@ qemuDomainValidateVcpuInfo(virDomainObjPtr vm)
> * @vm: domain object
> * @asyncJob: current asynchronous job type
> *
> - * Updates vCPU information private data of @vm.
> + * Updates vCPU information private data of @vm. Due to historical reasons this
> + * function returns success even if some data were not reported by qemu.
> *
> - * Returns number of detected vCPU threads on success, -1 on error and reports
> - * an appropriate error, -2 if the domain doesn't exist any more.
> + * Returns 0 on success and -1 on fatal error.
> */
> int
> qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
> @@ -5722,10 +5722,7 @@ qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
> if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
> return -1;
> ncpupids = qemuMonitorGetCPUInfo(qemuDomainGetMonitor(vm), &cpupids);
> - if (qemuDomainObjExitMonitor(driver, vm) < 0) {
> - ret = -2;
> - goto cleanup;
> - }
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
This is missing a "goto cleanup" which is added in the next patch, but
needs to be here!
>
> /* 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
> @@ -5745,10 +5742,7 @@ qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
> QEMU_DOMAIN_VCPU_PRIVATE(vcpu)->tid = 0;
> }
>
> - if (qemuDomainValidateVcpuInfo(vm) < 0)
> - goto cleanup;
> -
> - ret = ncpupids;
> + ret = 0;
>
> cleanup:
> VIR_FREE(cpupids);
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 453572b..2199258 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -4615,6 +4615,7 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver,
> {
> qemuDomainObjPrivatePtr priv = vm->privateData;
> virDomainVcpuDefPtr vcpuinfo = virDomainDefGetVcpu(vm->def, vcpu);
> + qemuDomainVcpuPrivatePtr vcpupriv = QEMU_DOMAIN_VCPU_PRIVATE(vcpuinfo);
> int ret = -1;
> int rc;
> int oldvcpus = virDomainDefGetVcpus(vm->def);
> @@ -4639,15 +4640,14 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver,
>
> vcpuinfo->online = true;
>
> - if ((rc = qemuDomainRefreshVcpuInfo(driver, vm, QEMU_ASYNC_JOB_NONE)) <= 0)
{
> - /* vcpu pids were not detected, skip setting of affinity */
> - if (rc == 0)
> - ret = 0;
> + if (qemuDomainRefreshVcpuInfo(driver, vm, QEMU_ASYNC_JOB_NONE) < 0)
> + goto cleanup;
>
> + if (qemuDomainValidateVcpuInfo(vm) < 0)
> goto cleanup;
> - }
>
> - if (qemuProcessSetupVcpu(vm, vcpu) < 0)
> + if (vcpupriv->tid > 0 &&
> + qemuProcessSetupVcpu(vm, vcpu) < 0)
> goto cleanup;
>
> ret = 0;
> @@ -4689,11 +4689,11 @@ qemuDomainHotplugDelVcpu(virQEMUDriverPtr driver,
> goto cleanup;
> }
>
> - if ((rc = qemuDomainRefreshVcpuInfo(driver, vm, QEMU_ASYNC_JOB_NONE)) < 0)
{
> - /* rollback only if domain didn't exit */
> - if (rc == -2)
> - goto cleanup;
> + if (qemuDomainRefreshVcpuInfo(driver, vm, QEMU_ASYNC_JOB_NONE) < 0)
> + goto cleanup;
Typing what I'm thinking - just to be sure it matches your expectations
of this code. So we no longer care to distinguish between failure and
failure due to domain exit. This just seems to mean we'll get that audit
after a possible domain exit and reset the vcpuinfo->online (won't
really matter if the domain exited).
The logic is still the same as before this patch. Function
qemuDomainRefreshVcpuInfo fails with error only in case the domain exited.
It's safe to rollback only if qemuDomainValidateVcpuInfo fails, this is what
was extracted from qemuDomainRefreshVcpuInfo.
ACK - with the goto cleanup added at least.
John
>
> + if (qemuDomainValidateVcpuInfo(vm) < 0) {
> + /* rollback vcpu count if the setting has failed */
> virDomainAuditVcpu(vm, oldvcpus, oldvcpus - 1, "update", false);
> vcpuinfo->online = true;
> goto cleanup;
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 9136ba5..2e405af 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -5193,6 +5193,9 @@ qemuProcessLaunch(virConnectPtr conn,
> if (qemuDomainRefreshVcpuInfo(driver, vm, asyncJob) < 0)
> goto cleanup;
>
> + if (qemuDomainValidateVcpuInfo(vm) < 0)
> + goto cleanup;
> +
> VIR_DEBUG("Detecting IOThread PIDs");
> if (qemuProcessDetectIOThreadPIDs(driver, vm, asyncJob) < 0)
> goto cleanup;
> @@ -5985,6 +5988,9 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED,
> if (qemuDomainRefreshVcpuInfo(driver, vm, QEMU_ASYNC_JOB_NONE) < 0)
> goto error;
>
> + if (qemuDomainValidateVcpuInfo(vm) < 0)
> + goto error;
> +
> VIR_DEBUG("Detecting IOThread PIDs");
> if (qemuProcessDetectIOThreadPIDs(driver, vm, QEMU_ASYNC_JOB_NONE) < 0)
> goto error;
>
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
3:11 a.m.
New subject: [libvirt] [PATCH 09/10] qemu: monitor: Return structures from qemuMonitorGetCPUInfo
The function will gradually add more returned data. Return a struct for
every vCPU containing the data.
---
src/qemu/qemu_domain.c | 26 ++++++++++-------------
src/qemu/qemu_monitor.c | 56 ++++++++++++++++++++++++++++++++++++++++++-------
src/qemu/qemu_monitor.h | 13 +++++++++++-
3 files changed, 72 insertions(+), 23 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 4cdd012..9d389f7 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5682,10 +5682,11 @@ qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
int asyncJob)
{
virDomainVcpuDefPtr vcpu;
+ qemuDomainVcpuPrivatePtr vcpupriv;
+ qemuMonitorCPUInfoPtr info = NULL;
size_t maxvcpus = virDomainDefGetVcpusMax(vm->def);
- pid_t *cpupids = NULL;
- int ncpupids;
size_t i;
+ int rc;
int ret = -1;
/*
@@ -5721,31 +5722,26 @@ qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
return -1;
- ncpupids = qemuMonitorGetCPUInfo(qemuDomainGetMonitor(vm), &cpupids);
+
+ rc = qemuMonitorGetCPUInfo(qemuDomainGetMonitor(vm), &info, maxvcpus);
+
if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
- /* 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) {
- virResetLastError();
- ret = 0;
+ if (rc < 0)
goto cleanup;
- }
for (i = 0; i < maxvcpus; i++) {
vcpu = virDomainDefGetVcpu(vm->def, i);
+ vcpupriv = QEMU_DOMAIN_VCPU_PRIVATE(vcpu);
- if (i < ncpupids)
- QEMU_DOMAIN_VCPU_PRIVATE(vcpu)->tid = cpupids[i];
- else
- QEMU_DOMAIN_VCPU_PRIVATE(vcpu)->tid = 0;
+ vcpupriv->tid = info[i].tid;
}
ret = 0;
cleanup:
- VIR_FREE(cpupids);
+ qemuMonitorCPUInfoFree(info, maxvcpus);
return ret;
}
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 4403bdd..0011ceb 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -1656,25 +1656,67 @@ qemuMonitorSystemReset(qemuMonitorPtr mon)
}
+void
+qemuMonitorCPUInfoFree(qemuMonitorCPUInfoPtr cpus,
+ size_t ncpus ATTRIBUTE_UNUSED)
+{
+ if (!cpus)
+ return;
+
+ VIR_FREE(cpus);
+}
+
+
/**
* qemuMonitorGetCPUInfo:
* @mon: monitor
- * @pids: returned array of thread ids corresponding to the vCPUs
+ * @cpus: pointer filled by array of qemuMonitorCPUInfo structures
+ * @maxvcpus: total possible number of vcpus
+ *
+ * Detects VCPU information. If qemu doesn't support or fails reporting
+ * information this function will return success as other parts of libvirt
+ * are able to cope with that.
*
- * Detects the vCPU thread ids. Returns count of detected vCPUs on success,
- * 0 if qemu didn't report thread ids (does not report libvirt error),
- * -1 on error (reports libvirt error).
+ * Returns 0 on success (including if qemu didn't report any data) and
+ * -1 on error (reports libvirt error).
*/
int
qemuMonitorGetCPUInfo(qemuMonitorPtr mon,
- int **pids)
+ qemuMonitorCPUInfoPtr *vcpus,
+ size_t maxvcpus)
{
+ qemuMonitorCPUInfoPtr info = NULL;
+ int *pids = NULL;
+ size_t i;
+ int ret = -1;
+ int rc;
+
QEMU_CHECK_MONITOR(mon);
+ if (VIR_ALLOC_N(info, maxvcpus) < 0)
+ return -1;
+
if (mon->json)
- return qemuMonitorJSONQueryCPUs(mon, pids);
+ rc = qemuMonitorJSONQueryCPUs(mon, &pids);
else
- return qemuMonitorTextQueryCPUs(mon, pids);
+ rc = qemuMonitorTextQueryCPUs(mon, &pids);
+
+ if (rc < 0) {
+ virResetLastError();
+ ret = 0;
+ goto cleanup;
+ }
+
+ for (i = 0; i < rc; i++)
+ info[i].tid = pids[i];
+
+ VIR_STEAL(*vcpus, info);
+ ret = 0;
+
+ cleanup:
+ qemuMonitorCPUInfoFree(info, maxvcpus);
+ VIR_FREE(pids);
+ return ret;
}
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 805656b..1ef002a 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -390,8 +390,19 @@ int qemuMonitorGetStatus(qemuMonitorPtr mon,
int qemuMonitorSystemReset(qemuMonitorPtr mon);
int qemuMonitorSystemPowerdown(qemuMonitorPtr mon);
+
+struct _qemuMonitorCPUInfo {
+ pid_t tid;
+};
+typedef struct _qemuMonitorCPUInfo qemuMonitorCPUInfo;
+typedef qemuMonitorCPUInfo *qemuMonitorCPUInfoPtr;
+
+void qemuMonitorCPUInfoFree(qemuMonitorCPUInfoPtr list,
+ size_t nitems);
int qemuMonitorGetCPUInfo(qemuMonitorPtr mon,
- int **pids);
+ qemuMonitorCPUInfoPtr *vcpus,
+ size_t maxvcpus);
+
int qemuMonitorGetVirtType(qemuMonitorPtr mon,
virDomainVirtType *virtType);
int qemuMonitorGetBalloonInfo(qemuMonitorPtr mon,
--
2.9.2
8:21 a.m.
New subject: [libvirt] [PATCH 09/10] qemu: monitor: Return structures from qemuMonitorGetCPUInfo
On 08/03/2016 04:11 AM, Peter Krempa wrote:
The function will gradually add more returned data. Return a struct
for
every vCPU containing the data.
---
src/qemu/qemu_domain.c | 26 ++++++++++-------------
src/qemu/qemu_monitor.c | 56 ++++++++++++++++++++++++++++++++++++++++++-------
src/qemu/qemu_monitor.h | 13 +++++++++++-
3 files changed, 72 insertions(+), 23 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 4cdd012..9d389f7 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5682,10 +5682,11 @@ qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
int asyncJob)
{
virDomainVcpuDefPtr vcpu;
+ qemuDomainVcpuPrivatePtr vcpupriv;
+ qemuMonitorCPUInfoPtr info = NULL;
size_t maxvcpus = virDomainDefGetVcpusMax(vm->def);
- pid_t *cpupids = NULL;
- int ncpupids;
size_t i;
+ int rc;
int ret = -1;
/*
@@ -5721,31 +5722,26 @@ qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
return -1;
- ncpupids = qemuMonitorGetCPUInfo(qemuDomainGetMonitor(vm), &cpupids);
+
+ rc = qemuMonitorGetCPUInfo(qemuDomainGetMonitor(vm), &info, maxvcpus);
+
if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
- /* 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) {
- virResetLastError();
- ret = 0;
+ if (rc < 0)
goto cleanup;
- }
for (i = 0; i < maxvcpus; i++) {
vcpu = virDomainDefGetVcpu(vm->def, i);
+ vcpupriv = QEMU_DOMAIN_VCPU_PRIVATE(vcpu);
- if (i < ncpupids)
- QEMU_DOMAIN_VCPU_PRIVATE(vcpu)->tid = cpupids[i];
- else
- QEMU_DOMAIN_VCPU_PRIVATE(vcpu)->tid = 0;
+ vcpupriv->tid = info[i].tid;
}
FWIW:
Once we reach this point the VIR_DOMAIN_VIRT_QEMU check is the only way
qemuDomainHasVcpuPids can return 0 in qemuDomainValidateVcpuInfo... So I
wonder - would it be useful to alter that function too so that we're
sure things match up properly vis-a-vis the 'tid' and online checks?
And my alter, I was thinking along the lines of a similar check for
VIR_DOMAIN_VIRT_QEMU...
ret = 0;
cleanup:
- VIR_FREE(cpupids);
+ qemuMonitorCPUInfoFree(info, maxvcpus);
return ret;
}
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 4403bdd..0011ceb 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -1656,25 +1656,67 @@ qemuMonitorSystemReset(qemuMonitorPtr mon)
}
+void
+qemuMonitorCPUInfoFree(qemuMonitorCPUInfoPtr cpus,
+ size_t ncpus ATTRIBUTE_UNUSED)
+{
+ if (!cpus)
+ return;
+
+ VIR_FREE(cpus);
+}
+
+
/**
* qemuMonitorGetCPUInfo:
* @mon: monitor
- * @pids: returned array of thread ids corresponding to the vCPUs
+ * @cpus: pointer filled by array of qemuMonitorCPUInfo structures
+ * @maxvcpus: total possible number of vcpus
+ *
+ * Detects VCPU information. If qemu doesn't support or fails reporting
+ * information this function will return success as other parts of libvirt
+ * are able to cope with that.
*
- * Detects the vCPU thread ids. Returns count of detected vCPUs on success,
- * 0 if qemu didn't report thread ids (does not report libvirt error),
- * -1 on error (reports libvirt error).
+ * Returns 0 on success (including if qemu didn't report any data) and
+ * -1 on error (reports libvirt error).
*/
int
qemuMonitorGetCPUInfo(qemuMonitorPtr mon,
- int **pids)
+ qemuMonitorCPUInfoPtr *vcpus,
+ size_t maxvcpus)
{
+ qemuMonitorCPUInfoPtr info = NULL;
+ int *pids = NULL;
+ size_t i;
+ int ret = -1;
+ int rc;
+
QEMU_CHECK_MONITOR(mon);
+ if (VIR_ALLOC_N(info, maxvcpus) < 0)
+ return -1;
+
if (mon->json)
- return qemuMonitorJSONQueryCPUs(mon, pids);
+ rc = qemuMonitorJSONQueryCPUs(mon, &pids);
else
- return qemuMonitorTextQueryCPUs(mon, pids);
+ rc = qemuMonitorTextQueryCPUs(mon, &pids);
+
+ if (rc < 0) {
If qemuMonitorJSONExtractCPUInfo() finds ncpus == 0, then it returns an
error which won't be "cleaned up" unless rc <= 0
+ virResetLastError();
+ ret = 0;
+ goto cleanup;
So, ret == 0, we go to cleanup, it cleans up 'info', then our caller
finds rc == 0, so it can fall into that for maxvcpus loop looking 'info'
and then of course freeing 'info' again.
+ }
+
+ for (i = 0; i < rc; i++)
+ info[i].tid = pids[i];
Still no "gaps" in our "internal" pids list which is fine, but I
assume
could change in the future... Just keeping it fresh in my mind ;-)
ACK with the necessary adjustments for the above rc check condition
which I think would be along the lines of:
if (rc <= 0)
virResetLastError();
if (rc < 0)
goto cleanup;
But I could be wrong...
John
+
+ VIR_STEAL(*vcpus, info);
+ ret = 0;
+
+ cleanup:
+ qemuMonitorCPUInfoFree(info, maxvcpus);
+ VIR_FREE(pids);
+ return ret;
}
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 805656b..1ef002a 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -390,8 +390,19 @@ int qemuMonitorGetStatus(qemuMonitorPtr mon,
int qemuMonitorSystemReset(qemuMonitorPtr mon);
int qemuMonitorSystemPowerdown(qemuMonitorPtr mon);
+
+struct _qemuMonitorCPUInfo {
+ pid_t tid;
+};
+typedef struct _qemuMonitorCPUInfo qemuMonitorCPUInfo;
+typedef qemuMonitorCPUInfo *qemuMonitorCPUInfoPtr;
+
+void qemuMonitorCPUInfoFree(qemuMonitorCPUInfoPtr list,
+ size_t nitems);
int qemuMonitorGetCPUInfo(qemuMonitorPtr mon,
- int **pids);
+ qemuMonitorCPUInfoPtr *vcpus,
+ size_t maxvcpus);
+
int qemuMonitorGetVirtType(qemuMonitorPtr mon,
virDomainVirtType *virtType);
int qemuMonitorGetBalloonInfo(qemuMonitorPtr mon,
3:11 a.m.
New subject: [libvirt] [PATCH 09/10] qemu: monitor: Return structures from qemuMonitorGetCPUInfo
On Wed, Aug 03, 2016 at 09:21:28 -0400, John Ferlan wrote:
On 08/03/2016 04:11 AM, Peter Krempa wrote:
> The function will gradually add more returned data. Return a struct for
> every vCPU containing the data.
> ---
> src/qemu/qemu_domain.c | 26 ++++++++++-------------
> src/qemu/qemu_monitor.c | 56 ++++++++++++++++++++++++++++++++++++++++++-------
> src/qemu/qemu_monitor.h | 13 +++++++++++-
> 3 files changed, 72 insertions(+), 23 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 4cdd012..9d389f7 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
[...]
> - }
>
> for (i = 0; i < maxvcpus; i++) {
> vcpu = virDomainDefGetVcpu(vm->def, i);
> + vcpupriv = QEMU_DOMAIN_VCPU_PRIVATE(vcpu);
>
> - if (i < ncpupids)
> - QEMU_DOMAIN_VCPU_PRIVATE(vcpu)->tid = cpupids[i];
> - else
> - QEMU_DOMAIN_VCPU_PRIVATE(vcpu)->tid = 0;
> + vcpupriv->tid = info[i].tid;
> }
FWIW:
Once we reach this point the VIR_DOMAIN_VIRT_QEMU check is the only waya
qemuDomainHasVcpuPids can return 0 in qemuDomainValidateVcpuInfo... So I
Not really. There are actually two options:
1) The thread id's were not set here due to the VIR_DOMAIN_VIRT_QEMU
check
This is necessary as qemu would return garbage thread ids as
described by the rather huge comment above the check.
2) Qemu itself would not report the thread ids
Some vintage versions were not doing it. I don't really know if it
is possible since we support only certain versions of qemu now but
I'm not really going to check at this point.
wonder - would it be useful to alter that function too so that
we're
sure things match up properly vis-a-vis the 'tid' and online checks?
And my alter, I was thinking along the lines of a similar check for
VIR_DOMAIN_VIRT_QEMU...
>
> ret = 0;
>
> cleanup:
> - VIR_FREE(cpupids);
> + qemuMonitorCPUInfoFree(info, maxvcpus);
> return ret;
> }
>
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 4403bdd..0011ceb 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -1656,25 +1656,67 @@ qemuMonitorSystemReset(qemuMonitorPtr mon)
> }
>
>
> +void
> +qemuMonitorCPUInfoFree(qemuMonitorCPUInfoPtr cpus,
> + size_t ncpus ATTRIBUTE_UNUSED)
> +{
> + if (!cpus)
> + return;
> +
> + VIR_FREE(cpus);
> +}
> +
> +
> /**
> * qemuMonitorGetCPUInfo:
> * @mon: monitor
> - * @pids: returned array of thread ids corresponding to the vCPUs
> + * @cpus: pointer filled by array of qemuMonitorCPUInfo structures
> + * @maxvcpus: total possible number of vcpus
> + *
> + * Detects VCPU information. If qemu doesn't support or fails reporting
> + * information this function will return success as other parts of libvirt
> + * are able to cope with that.
> *
> - * Detects the vCPU thread ids. Returns count of detected vCPUs on success,
> - * 0 if qemu didn't report thread ids (does not report libvirt error),
> - * -1 on error (reports libvirt error).
> + * Returns 0 on success (including if qemu didn't report any data) and
> + * -1 on error (reports libvirt error).
> */
> int
> qemuMonitorGetCPUInfo(qemuMonitorPtr mon,
> - int **pids)
> + qemuMonitorCPUInfoPtr *vcpus,
> + size_t maxvcpus)
> {
> + qemuMonitorCPUInfoPtr info = NULL;
> + int *pids = NULL;
> + size_t i;
> + int ret = -1;
> + int rc;
> +
> QEMU_CHECK_MONITOR(mon);
>
> + if (VIR_ALLOC_N(info, maxvcpus) < 0)
> + return -1;
> +
> if (mon->json)
> - return qemuMonitorJSONQueryCPUs(mon, pids);
> + rc = qemuMonitorJSONQueryCPUs(mon, &pids);
> else
> - return qemuMonitorTextQueryCPUs(mon, pids);
> + rc = qemuMonitorTextQueryCPUs(mon, &pids);
> +
> + if (rc < 0) {
If qemuMonitorJSONExtractCPUInfo() finds ncpus == 0, then it returns an
error which won't be "cleaned up" unless rc <= 0
qemuMonitorJSONExtractCPUInfo does not set any error if the returned
value is 0.
> + virResetLastError();
> + ret = 0;
> + goto cleanup;
So, ret == 0, we go to cleanup, it cleans up 'info', then our caller
finds rc == 0, so it can fall into that for maxvcpus loop looking 'info'
and then of course freeing 'info' again.
The idea is that qemuMonitorGetCPUInfo returns the fully allocated array
of structures if it retuns success. This is including if there are no
threads reported by qemu.
There is a different bug there actually. In case when the error is reset
the returning pointer needs to be set.
I'll post a fixed version.
Peter
3:11 a.m.
New subject: [libvirt] [PATCH 10/10] qemu: monitor: Return struct from qemuMonitor(Text|Json)QueryCPUs
Prepare to extract more data by returning a array of structs rather than
just an array of thread ids. Additionally report fatal errors separately
from qemu not being able to produce data.
---
src/qemu/qemu_monitor.c | 31 ++++++++++++-------
src/qemu/qemu_monitor.h | 6 ++++
src/qemu/qemu_monitor_json.c | 71 ++++++++++++++++++++++----------------------
src/qemu/qemu_monitor_json.h | 2 +-
src/qemu/qemu_monitor_text.c | 37 +++++++++++------------
src/qemu/qemu_monitor_text.h | 2 +-
tests/qemumonitorjsontest.c | 31 ++++++++++++++-----
7 files changed, 104 insertions(+), 76 deletions(-)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 0011ceb..578b078 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -1666,6 +1666,16 @@ qemuMonitorCPUInfoFree(qemuMonitorCPUInfoPtr cpus,
VIR_FREE(cpus);
}
+void
+qemuMonitorQueryCpusFree(struct qemuMonitorQueryCpusEntry *entries,
+ size_t nentries ATTRIBUTE_UNUSED)
+{
+ if (!entries)
+ return;
+
+ VIR_FREE(entries);
+}
+
/**
* qemuMonitorGetCPUInfo:
@@ -1686,10 +1696,10 @@ qemuMonitorGetCPUInfo(qemuMonitorPtr mon,
size_t maxvcpus)
{
qemuMonitorCPUInfoPtr info = NULL;
- int *pids = NULL;
+ struct qemuMonitorQueryCpusEntry *cpuentries = NULL;
size_t i;
int ret = -1;
- int rc;
+ int ncpuentries;
QEMU_CHECK_MONITOR(mon);
@@ -1697,25 +1707,26 @@ qemuMonitorGetCPUInfo(qemuMonitorPtr mon,
return -1;
if (mon->json)
- rc = qemuMonitorJSONQueryCPUs(mon, &pids);
+ ncpuentries = qemuMonitorJSONQueryCPUs(mon, &cpuentries);
else
- rc = qemuMonitorTextQueryCPUs(mon, &pids);
+ ncpuentries = qemuMonitorTextQueryCPUs(mon, &cpuentries);
+
+ if (ncpuentries < 0) {
+ if (ncpuentries == -2)
+ ret = 0;
- if (rc < 0) {
- virResetLastError();
- ret = 0;
goto cleanup;
}
- for (i = 0; i < rc; i++)
- info[i].tid = pids[i];
+ for (i = 0; i < ncpuentries; i++)
+ info[i].tid = cpuentries[i].tid;
VIR_STEAL(*vcpus, info);
ret = 0;
cleanup:
qemuMonitorCPUInfoFree(info, maxvcpus);
- VIR_FREE(pids);
+ qemuMonitorQueryCpusFree(cpuentries, ncpuentries);
return ret;
}
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 1ef002a..6022b9d 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -390,6 +390,12 @@ int qemuMonitorGetStatus(qemuMonitorPtr mon,
int qemuMonitorSystemReset(qemuMonitorPtr mon);
int qemuMonitorSystemPowerdown(qemuMonitorPtr mon);
+struct qemuMonitorQueryCpusEntry {
+ pid_t tid;
+};
+void qemuMonitorQueryCpusFree(struct qemuMonitorQueryCpusEntry *entries,
+ size_t nentries);
+
struct _qemuMonitorCPUInfo {
pid_t tid;
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 715e1c7..8018860 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -1323,69 +1323,65 @@ int qemuMonitorJSONSystemReset(qemuMonitorPtr mon)
* { "CPU": 1, "current": false, "halted": true,
"pc": 7108165 } ]
*/
static int
-qemuMonitorJSONExtractCPUInfo(virJSONValuePtr reply,
- int **pids)
+qemuMonitorJSONExtractCPUInfo(virJSONValuePtr data,
+ struct qemuMonitorQueryCpusEntry **entries)
{
- virJSONValuePtr data;
+ struct qemuMonitorQueryCpusEntry *cpus = NULL;
int ret = -1;
size_t i;
- int *threads = NULL;
ssize_t ncpus;
- if (!(data = virJSONValueObjectGetArray(reply, "return"))) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("cpu reply was missing return data"));
- goto cleanup;
- }
-
- if ((ncpus = virJSONValueArraySize(data)) <= 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("cpu information was empty"));
- goto cleanup;
- }
+ if ((ncpus = virJSONValueArraySize(data)) <= 0)
+ return -2;
- if (VIR_ALLOC_N(threads, ncpus) < 0)
+ if (VIR_ALLOC_N(cpus, ncpus) < 0)
goto cleanup;
for (i = 0; i < ncpus; i++) {
virJSONValuePtr entry = virJSONValueArrayGet(data, i);
- int thread;
+ int thread = 0;
if (!entry) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("cpu information was missing an array element"));
+ ret = -2;
goto cleanup;
}
- if (virJSONValueObjectGetNumberInt(entry, "thread_id", &thread)
< 0) {
- /* Some older qemu versions don't report the thread_id,
- * so treat this as non-fatal, simply returning no data */
- ret = 0;
- goto cleanup;
- }
+ /* Some older qemu versions don't report the thread_id so treat this as
+ * non-fatal, simply returning no data */
+ ignore_value(virJSONValueObjectGetNumberInt(entry, "thread_id",
&thread));
- threads[i] = thread;
+ cpus[i].tid = thread;
}
- *pids = threads;
- threads = NULL;
+ VIR_STEAL(*entries, cpus);
ret = ncpus;
cleanup:
- VIR_FREE(threads);
+ qemuMonitorQueryCpusFree(cpus, ncpus);
return ret;
}
+/**
+ * qemuMonitorJSONQueryCPUs:
+ *
+ * @mon: monitor object
+ * @entries: filled with detected entries on success
+ *
+ * Queries qemu for cpu-related information. Failure to execute the command or
+ * extract results does not produce an error as libvirt can continue without
+ * this information.
+ *
+ * Returns number of cpu data entries returned in @entries on success, -1 on a
+ * fatal error (oom ...) and -2 if the query failed gracefully.
+ */
int
qemuMonitorJSONQueryCPUs(qemuMonitorPtr mon,
- int **pids)
+ struct qemuMonitorQueryCpusEntry **entries)
{
int ret = -1;
- virJSONValuePtr cmd = qemuMonitorJSONMakeCommand("query-cpus",
- NULL);
+ virJSONValuePtr cmd = qemuMonitorJSONMakeCommand("query-cpus", NULL);
virJSONValuePtr reply = NULL;
-
- *pids = NULL;
+ virJSONValuePtr data;
if (!cmd)
return -1;
@@ -1393,10 +1389,13 @@ qemuMonitorJSONQueryCPUs(qemuMonitorPtr mon,
if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0)
goto cleanup;
- if (qemuMonitorJSONCheckError(cmd, reply) < 0)
+ if (!(data = virJSONValueObjectGetArray(reply, "return"))) {
+ ret = -2;
goto cleanup;
+ }
+
+ ret = qemuMonitorJSONExtractCPUInfo(data, entries);
- ret = qemuMonitorJSONExtractCPUInfo(reply, pids);
cleanup:
virJSONValueFree(cmd);
virJSONValueFree(reply);
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index 174f0ef..24b23d2 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -59,7 +59,7 @@ int qemuMonitorJSONSystemPowerdown(qemuMonitorPtr mon);
int qemuMonitorJSONSystemReset(qemuMonitorPtr mon);
int qemuMonitorJSONQueryCPUs(qemuMonitorPtr mon,
- int **pids);
+ struct qemuMonitorQueryCpusEntry **entries);
int qemuMonitorJSONGetVirtType(qemuMonitorPtr mon,
virDomainVirtType *virtType);
int qemuMonitorJSONUpdateVideoMemorySize(qemuMonitorPtr mon,
diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c
index ca04965..648c7db 100644
--- a/src/qemu/qemu_monitor_text.c
+++ b/src/qemu/qemu_monitor_text.c
@@ -502,12 +502,14 @@ int qemuMonitorTextSystemReset(qemuMonitorPtr mon)
int
qemuMonitorTextQueryCPUs(qemuMonitorPtr mon,
- int **pids)
+ struct qemuMonitorQueryCpusEntry **entries)
{
char *qemucpus = NULL;
char *line;
- pid_t *cpupids = NULL;
- size_t ncpupids = 0;
+ struct qemuMonitorQueryCpusEntry *cpus = NULL;
+ size_t ncpus;
+ struct qemuMonitorQueryCpusEntry cpu = {0};
+ int ret = -2; /* -2 denotes a non-fatal error to get the data */
if (qemuMonitorHMPCommand(mon, "info cpus", &qemucpus) < 0)
return -1;
@@ -529,15 +531,17 @@ qemuMonitorTextQueryCPUs(qemuMonitorPtr mon,
/* Extract host Thread ID */
if ((offset = strstr(line, "thread_id=")) == NULL)
- goto error;
+ goto cleanup;
if (virStrToLong_i(offset + strlen("thread_id="), &end, 10,
&tid) < 0)
- goto error;
+ goto cleanup;
if (end == NULL || !c_isspace(*end))
- goto error;
+ goto cleanup;
- if (VIR_APPEND_ELEMENT_COPY(cpupids, ncpupids, tid) < 0)
- goto error;
+ cpu.tid = tid;
+
+ if (VIR_APPEND_ELEMENT_COPY(cpus, ncpus, cpu) < 0)
+ goto cleanup;
VIR_DEBUG("tid=%d", tid);
@@ -547,20 +551,13 @@ qemuMonitorTextQueryCPUs(qemuMonitorPtr mon,
line = strchr(offset, '\n');
} while (line != NULL);
- /* Validate we got data for all VCPUs we expected */
- VIR_FREE(qemucpus);
- *pids = cpupids;
- return ncpupids;
+ VIR_STEAL(*entries, cpus);
+ ret = ncpus;
- error:
+ cleanup:
+ qemuMonitorQueryCpusFree(cpus, ncpus);
VIR_FREE(qemucpus);
- VIR_FREE(cpupids);
-
- /* Returning 0 to indicate non-fatal failure, since
- * older QEMU does not have VCPU<->PID mapping and
- * we don't want to fail on that
- */
- return 0;
+ return ret;
}
diff --git a/src/qemu/qemu_monitor_text.h b/src/qemu/qemu_monitor_text.h
index b7f0cab..f9e0f82 100644
--- a/src/qemu/qemu_monitor_text.h
+++ b/src/qemu/qemu_monitor_text.h
@@ -50,7 +50,7 @@ int qemuMonitorTextSystemPowerdown(qemuMonitorPtr mon);
int qemuMonitorTextSystemReset(qemuMonitorPtr mon);
int qemuMonitorTextQueryCPUs(qemuMonitorPtr mon,
- int **pids);
+ struct qemuMonitorQueryCpusEntry **entries);
int qemuMonitorTextGetVirtType(qemuMonitorPtr mon,
virDomainVirtType *virtType);
int qemuMonitorTextGetBalloonInfo(qemuMonitorPtr mon,
diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
index 544b569..f848d80 100644
--- a/tests/qemumonitorjsontest.c
+++ b/tests/qemumonitorjsontest.c
@@ -1201,6 +1201,16 @@ GEN_TEST_FUNC(qemuMonitorJSONNBDServerStart, "localhost",
12345)
GEN_TEST_FUNC(qemuMonitorJSONNBDServerAdd, "vda", true)
GEN_TEST_FUNC(qemuMonitorJSONDetachCharDev, "serial1")
+static bool
+testQemuMonitorJSONqemuMonitorJSONQueryCPUsEqual(struct qemuMonitorQueryCpusEntry *a,
+ struct qemuMonitorQueryCpusEntry *b)
+{
+ if (a->tid != b->tid)
+ return false;
+
+ return true;
+}
+
static int
testQemuMonitorJSONqemuMonitorJSONQueryCPUs(const void *data)
@@ -1208,9 +1218,14 @@ testQemuMonitorJSONqemuMonitorJSONQueryCPUs(const void *data)
virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data;
qemuMonitorTestPtr test = qemuMonitorTestNewSimple(true, xmlopt);
int ret = -1;
- pid_t *cpupids = NULL;
- pid_t expected_cpupids[] = {17622, 17624, 17626, 17628};
- int ncpupids;
+ struct qemuMonitorQueryCpusEntry *cpudata = NULL;
+ struct qemuMonitorQueryCpusEntry expect[] = {
+ {17622},
+ {17624},
+ {17626},
+ {17628},
+ };
+ int ncpupids = 0;
size_t i;
if (!test)
@@ -1252,7 +1267,7 @@ testQemuMonitorJSONqemuMonitorJSONQueryCPUs(const void *data)
"}") < 0)
goto cleanup;
- ncpupids = qemuMonitorJSONQueryCPUs(qemuMonitorTestGetMonitor(test), &cpupids);
+ ncpupids = qemuMonitorJSONQueryCPUs(qemuMonitorTestGetMonitor(test), &cpudata);
if (ncpupids != 4) {
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -1261,10 +1276,10 @@ testQemuMonitorJSONqemuMonitorJSONQueryCPUs(const void *data)
}
for (i = 0; i < ncpupids; i++) {
- if (cpupids[i] != expected_cpupids[i]) {
+ if (!testQemuMonitorJSONqemuMonitorJSONQueryCPUsEqual(cpudata + i,
+ expect + i)) {
virReportError(VIR_ERR_INTERNAL_ERROR,
- "Expecting cpupids[%zu] = %d but got %d",
- i, expected_cpupids[i], cpupids[i]);
+ "vcpu entry %zu does not match expected data", i);
goto cleanup;
}
}
@@ -1272,7 +1287,7 @@ testQemuMonitorJSONqemuMonitorJSONQueryCPUs(const void *data)
ret = 0;
cleanup:
- VIR_FREE(cpupids);
+ qemuMonitorQueryCpusFree(cpudata, ncpupids);
qemuMonitorTestFree(test);
return ret;
}
--
2.9.2
11:04 a.m.
New subject: [libvirt] [PATCH 10/10] qemu: monitor: Return struct from qemuMonitor(Text|Json)QueryCPUs
On 08/03/2016 04:11 AM, Peter Krempa wrote:
Prepare to extract more data by returning a array of structs rather
than
just an array of thread ids. Additionally report fatal errors separately
from qemu not being able to produce data.
---
src/qemu/qemu_monitor.c | 31 ++++++++++++-------
src/qemu/qemu_monitor.h | 6 ++++
src/qemu/qemu_monitor_json.c | 71 ++++++++++++++++++++++----------------------
src/qemu/qemu_monitor_json.h | 2 +-
src/qemu/qemu_monitor_text.c | 37 +++++++++++------------
src/qemu/qemu_monitor_text.h | 2 +-
tests/qemumonitorjsontest.c | 31 ++++++++++++++-----
7 files changed, 104 insertions(+), 76 deletions(-)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 0011ceb..578b078 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -1666,6 +1666,16 @@ qemuMonitorCPUInfoFree(qemuMonitorCPUInfoPtr cpus,
VIR_FREE(cpus);
}
+void
+qemuMonitorQueryCpusFree(struct qemuMonitorQueryCpusEntry *entries,
+ size_t nentries ATTRIBUTE_UNUSED)
+{
+ if (!entries)
+ return;
[1] Maybe this should be a 'int' parameter and a <= 0 check...
+
+ VIR_FREE(entries);
+}
+
/**
* qemuMonitorGetCPUInfo:
@@ -1686,10 +1696,10 @@ qemuMonitorGetCPUInfo(qemuMonitorPtr mon,
size_t maxvcpus)
{
qemuMonitorCPUInfoPtr info = NULL;
- int *pids = NULL;
+ struct qemuMonitorQueryCpusEntry *cpuentries = NULL;
size_t i;
int ret = -1;
- int rc;
+ int ncpuentries;
QEMU_CHECK_MONITOR(mon);
@@ -1697,25 +1707,26 @@ qemuMonitorGetCPUInfo(qemuMonitorPtr mon,
return -1;
if (mon->json)
- rc = qemuMonitorJSONQueryCPUs(mon, &pids);
+ ncpuentries = qemuMonitorJSONQueryCPUs(mon, &cpuentries);
else
- rc = qemuMonitorTextQueryCPUs(mon, &pids);
+ ncpuentries = qemuMonitorTextQueryCPUs(mon, &cpuentries);
+
+ if (ncpuentries < 0) {
+ if (ncpuentries == -2)
+ ret = 0;
Similar to previous patch w/r/t "ret = 0" and returning to caller
- if (rc < 0) {
- virResetLastError();
- ret = 0;
goto cleanup;
}
- for (i = 0; i < rc; i++)
- info[i].tid = pids[i];
+ for (i = 0; i < ncpuentries; i++)
+ info[i].tid = cpuentries[i].tid;
VIR_STEAL(*vcpus, info);
ret = 0;
cleanup:
qemuMonitorCPUInfoFree(info, maxvcpus);
- VIR_FREE(pids);
+ qemuMonitorQueryCpusFree(cpuentries, ncpuentries);
[1]Although not "currently" used, the ncpuentries is an int being passed
to a function that wants size_t
And yes, that's from a Coverity warning
return ret;
}
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 1ef002a..6022b9d 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -390,6 +390,12 @@ int qemuMonitorGetStatus(qemuMonitorPtr mon,
int qemuMonitorSystemReset(qemuMonitorPtr mon);
int qemuMonitorSystemPowerdown(qemuMonitorPtr mon);
+struct qemuMonitorQueryCpusEntry {
+ pid_t tid;
+};
+void qemuMonitorQueryCpusFree(struct qemuMonitorQueryCpusEntry *entries,
+ size_t nentries);
+
struct _qemuMonitorCPUInfo {
pid_t tid;
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 715e1c7..8018860 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -1323,69 +1323,65 @@ int qemuMonitorJSONSystemReset(qemuMonitorPtr mon)
* { "CPU": 1, "current": false, "halted": true,
"pc": 7108165 } ]
*/
static int
-qemuMonitorJSONExtractCPUInfo(virJSONValuePtr reply,
- int **pids)
+qemuMonitorJSONExtractCPUInfo(virJSONValuePtr data,
+ struct qemuMonitorQueryCpusEntry **entries)
{
- virJSONValuePtr data;
+ struct qemuMonitorQueryCpusEntry *cpus = NULL;
int ret = -1;
size_t i;
- int *threads = NULL;
ssize_t ncpus;
- if (!(data = virJSONValueObjectGetArray(reply, "return"))) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("cpu reply was missing return data"));
- goto cleanup;
- }
-
- if ((ncpus = virJSONValueArraySize(data)) <= 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("cpu information was empty"));
- goto cleanup;
- }
+ if ((ncpus = virJSONValueArraySize(data)) <= 0)
+ return -2;
- if (VIR_ALLOC_N(threads, ncpus) < 0)
+ if (VIR_ALLOC_N(cpus, ncpus) < 0)
goto cleanup;
for (i = 0; i < ncpus; i++) {
virJSONValuePtr entry = virJSONValueArrayGet(data, i);
- int thread;
+ int thread = 0;
if (!entry) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("cpu information was missing an array
element"));
+ ret = -2;
goto cleanup;
}
- if (virJSONValueObjectGetNumberInt(entry, "thread_id", &thread)
< 0) {
- /* Some older qemu versions don't report the thread_id,
- * so treat this as non-fatal, simply returning no data */
- ret = 0;
- goto cleanup;
- }
+ /* Some older qemu versions don't report the thread_id so treat this as
+ * non-fatal, simply returning no data */
+ ignore_value(virJSONValueObjectGetNumberInt(entry, "thread_id",
&thread));
So we're going to still make 'ncpus' calls to get the same answer...
Then we're going to return 5 elements with {0, 0, 0, 0, 0} and the
caller is going to do what? I think eventually we'll end up failing in
qemuDomainValidateVcpuInfo.
- threads[i] = thread;
+ cpus[i].tid = thread;
}
- *pids = threads;
- threads = NULL;
+ VIR_STEAL(*entries, cpus);
ret = ncpus;
cleanup:
- VIR_FREE(threads);
+ qemuMonitorQueryCpusFree(cpus, ncpus);
return ret;
}
+/**
+ * qemuMonitorJSONQueryCPUs:
+ *
+ * @mon: monitor object
+ * @entries: filled with detected entries on success
+ *
+ * Queries qemu for cpu-related information. Failure to execute the command or
+ * extract results does not produce an error as libvirt can continue without
+ * this information.
+ *
+ * Returns number of cpu data entries returned in @entries on success, -1 on a
+ * fatal error (oom ...) and -2 if the query failed gracefully.
+ */
int
qemuMonitorJSONQueryCPUs(qemuMonitorPtr mon,
- int **pids)
+ struct qemuMonitorQueryCpusEntry **entries)
{
int ret = -1;
- virJSONValuePtr cmd = qemuMonitorJSONMakeCommand("query-cpus",
- NULL);
+ virJSONValuePtr cmd = qemuMonitorJSONMakeCommand("query-cpus", NULL);
virJSONValuePtr reply = NULL;
-
- *pids = NULL;
+ virJSONValuePtr data;
if (!cmd)
return -1;
@@ -1393,10 +1389,13 @@ qemuMonitorJSONQueryCPUs(qemuMonitorPtr mon,
if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0)
goto cleanup;
- if (qemuMonitorJSONCheckError(cmd, reply) < 0)
+ if (!(data = virJSONValueObjectGetArray(reply, "return"))) {
+ ret = -2;
goto cleanup;
+ }
+
+ ret = qemuMonitorJSONExtractCPUInfo(data, entries);
- ret = qemuMonitorJSONExtractCPUInfo(reply, pids);
cleanup:
virJSONValueFree(cmd);
virJSONValueFree(reply);
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index 174f0ef..24b23d2 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -59,7 +59,7 @@ int qemuMonitorJSONSystemPowerdown(qemuMonitorPtr mon);
int qemuMonitorJSONSystemReset(qemuMonitorPtr mon);
int qemuMonitorJSONQueryCPUs(qemuMonitorPtr mon,
- int **pids);
+ struct qemuMonitorQueryCpusEntry **entries);
int qemuMonitorJSONGetVirtType(qemuMonitorPtr mon,
virDomainVirtType *virtType);
int qemuMonitorJSONUpdateVideoMemorySize(qemuMonitorPtr mon,
diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c
index ca04965..648c7db 100644
--- a/src/qemu/qemu_monitor_text.c
+++ b/src/qemu/qemu_monitor_text.c
@@ -502,12 +502,14 @@ int qemuMonitorTextSystemReset(qemuMonitorPtr mon)
int
qemuMonitorTextQueryCPUs(qemuMonitorPtr mon,
- int **pids)
+ struct qemuMonitorQueryCpusEntry **entries)
{
char *qemucpus = NULL;
char *line;
- pid_t *cpupids = NULL;
- size_t ncpupids = 0;
+ struct qemuMonitorQueryCpusEntry *cpus = NULL;
+ size_t ncpus;
Need to initialize to 0 (surprised neither compiler nor Coverity picked
up on it).
+ struct qemuMonitorQueryCpusEntry cpu = {0};
+ int ret = -2; /* -2 denotes a non-fatal error to get the data */
if (qemuMonitorHMPCommand(mon, "info cpus", &qemucpus) < 0)
return -1;
@@ -529,15 +531,17 @@ qemuMonitorTextQueryCPUs(qemuMonitorPtr mon,
/* Extract host Thread ID */
if ((offset = strstr(line, "thread_id=")) == NULL)
- goto error;
+ goto cleanup;
if (virStrToLong_i(offset + strlen("thread_id="), &end, 10,
&tid) < 0)
- goto error;
+ goto cleanup;
if (end == NULL || !c_isspace(*end))
- goto error;
+ goto cleanup;
- if (VIR_APPEND_ELEMENT_COPY(cpupids, ncpupids, tid) < 0)
- goto error;
+ cpu.tid = tid;
+
+ if (VIR_APPEND_ELEMENT_COPY(cpus, ncpus, cpu) < 0)
+ goto cleanup;
VIR_DEBUG("tid=%d", tid);
@@ -547,20 +551,13 @@ qemuMonitorTextQueryCPUs(qemuMonitorPtr mon,
line = strchr(offset, '\n');
} while (line != NULL);
- /* Validate we got data for all VCPUs we expected */
- VIR_FREE(qemucpus);
- *pids = cpupids;
- return ncpupids;
+ VIR_STEAL(*entries, cpus);
+ ret = ncpus;
- error:
+ cleanup:
+ qemuMonitorQueryCpusFree(cpus, ncpus);
VIR_FREE(qemucpus);
- VIR_FREE(cpupids);
-
- /* Returning 0 to indicate non-fatal failure, since
- * older QEMU does not have VCPU<->PID mapping and
- * we don't want to fail on that
- */
- return 0;
+ return ret;
}
diff --git a/src/qemu/qemu_monitor_text.h b/src/qemu/qemu_monitor_text.h
index b7f0cab..f9e0f82 100644
--- a/src/qemu/qemu_monitor_text.h
+++ b/src/qemu/qemu_monitor_text.h
@@ -50,7 +50,7 @@ int qemuMonitorTextSystemPowerdown(qemuMonitorPtr mon);
int qemuMonitorTextSystemReset(qemuMonitorPtr mon);
int qemuMonitorTextQueryCPUs(qemuMonitorPtr mon,
- int **pids);
+ struct qemuMonitorQueryCpusEntry **entries);
int qemuMonitorTextGetVirtType(qemuMonitorPtr mon,
virDomainVirtType *virtType);
int qemuMonitorTextGetBalloonInfo(qemuMonitorPtr mon,
diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
index 544b569..f848d80 100644
--- a/tests/qemumonitorjsontest.c
+++ b/tests/qemumonitorjsontest.c
Just a thought... should we have a test that returns "0" entries - does
it make sense?
I think this is close, but needs a few tweaks or explanation before ACK
John
@@ -1201,6 +1201,16 @@ GEN_TEST_FUNC(qemuMonitorJSONNBDServerStart,
"localhost", 12345)
GEN_TEST_FUNC(qemuMonitorJSONNBDServerAdd, "vda", true)
GEN_TEST_FUNC(qemuMonitorJSONDetachCharDev, "serial1")
+static bool
+testQemuMonitorJSONqemuMonitorJSONQueryCPUsEqual(struct qemuMonitorQueryCpusEntry *a,
+ struct qemuMonitorQueryCpusEntry *b)
+{
+ if (a->tid != b->tid)
+ return false;
+
+ return true;
+}
+
static int
testQemuMonitorJSONqemuMonitorJSONQueryCPUs(const void *data)
@@ -1208,9 +1218,14 @@ testQemuMonitorJSONqemuMonitorJSONQueryCPUs(const void *data)
virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data;
qemuMonitorTestPtr test = qemuMonitorTestNewSimple(true, xmlopt);
int ret = -1;
- pid_t *cpupids = NULL;
- pid_t expected_cpupids[] = {17622, 17624, 17626, 17628};
- int ncpupids;
+ struct qemuMonitorQueryCpusEntry *cpudata = NULL;
+ struct qemuMonitorQueryCpusEntry expect[] = {
+ {17622},
+ {17624},
+ {17626},
+ {17628},
+ };
+ int ncpupids = 0;
size_t i;
if (!test)
@@ -1252,7 +1267,7 @@ testQemuMonitorJSONqemuMonitorJSONQueryCPUs(const void *data)
"}") < 0)
goto cleanup;
- ncpupids = qemuMonitorJSONQueryCPUs(qemuMonitorTestGetMonitor(test), &cpupids);
+ ncpupids = qemuMonitorJSONQueryCPUs(qemuMonitorTestGetMonitor(test), &cpudata);
if (ncpupids != 4) {
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -1261,10 +1276,10 @@ testQemuMonitorJSONqemuMonitorJSONQueryCPUs(const void *data)
}
for (i = 0; i < ncpupids; i++) {
- if (cpupids[i] != expected_cpupids[i]) {
+ if (!testQemuMonitorJSONqemuMonitorJSONQueryCPUsEqual(cpudata + i,
+ expect + i)) {
virReportError(VIR_ERR_INTERNAL_ERROR,
- "Expecting cpupids[%zu] = %d but got %d",
- i, expected_cpupids[i], cpupids[i]);
+ "vcpu entry %zu does not match expected data", i);
goto cleanup;
}
}
@@ -1272,7 +1287,7 @@ testQemuMonitorJSONqemuMonitorJSONQueryCPUs(const void *data)
ret = 0;
cleanup:
- VIR_FREE(cpupids);
+ qemuMonitorQueryCpusFree(cpudata, ncpupids);
qemuMonitorTestFree(test);
return ret;
}
noon
New subject: [libvirt] [PATCH 10/10] qemu: monitor: Return struct from qemuMonitor(Text|Json)QueryCPUs
On Wed, Aug 03, 2016 at 12:04:02 -0400, John Ferlan wrote:
On 08/03/2016 04:11 AM, Peter Krempa wrote:
> Prepare to extract more data by returning a array of structs rather than
> just an array of thread ids. Additionally report fatal errors separately
> from qemu not being able to produce data.
> ---
> src/qemu/qemu_monitor.c | 31 ++++++++++++-------
> src/qemu/qemu_monitor.h | 6 ++++
> src/qemu/qemu_monitor_json.c | 71 ++++++++++++++++++++++----------------------
> src/qemu/qemu_monitor_json.h | 2 +-
> src/qemu/qemu_monitor_text.c | 37 +++++++++++------------
> src/qemu/qemu_monitor_text.h | 2 +-
> tests/qemumonitorjsontest.c | 31 ++++++++++++++-----
> 7 files changed, 104 insertions(+), 76 deletions(-)
>
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 0011ceb..578b078 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -1666,6 +1666,16 @@ qemuMonitorCPUInfoFree(qemuMonitorCPUInfoPtr cpus,
> VIR_FREE(cpus);
> }
>
> +void
> +qemuMonitorQueryCpusFree(struct qemuMonitorQueryCpusEntry *entries,
> + size_t nentries ATTRIBUTE_UNUSED)
> +{
> + if (!entries)
> + return;
[1] Maybe this should be a 'int' parameter and a <= 0 check...
What?! That's a freeing function. That does not make any sense.
> +
> + VIR_FREE(entries);
> +}
> +
12:12 p.m.
New subject: [libvirt] [PATCH 10/10] qemu: monitor: Return struct from qemuMonitor(Text|Json)QueryCPUs
On 08/03/2016 01:00 PM, Peter Krempa wrote:
On Wed, Aug 03, 2016 at 12:04:02 -0400, John Ferlan wrote:
>
>
> On 08/03/2016 04:11 AM, Peter Krempa wrote:
>> Prepare to extract more data by returning a array of structs rather than
>> just an array of thread ids. Additionally report fatal errors separately
>> from qemu not being able to produce data.
>> ---
>> src/qemu/qemu_monitor.c | 31 ++++++++++++-------
>> src/qemu/qemu_monitor.h | 6 ++++
>> src/qemu/qemu_monitor_json.c | 71 ++++++++++++++++++++++----------------------
>> src/qemu/qemu_monitor_json.h | 2 +-
>> src/qemu/qemu_monitor_text.c | 37 +++++++++++------------
>> src/qemu/qemu_monitor_text.h | 2 +-
>> tests/qemumonitorjsontest.c | 31 ++++++++++++++-----
>> 7 files changed, 104 insertions(+), 76 deletions(-)
>>
>> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
>> index 0011ceb..578b078 100644
>> --- a/src/qemu/qemu_monitor.c
>> +++ b/src/qemu/qemu_monitor.c
>> @@ -1666,6 +1666,16 @@ qemuMonitorCPUInfoFree(qemuMonitorCPUInfoPtr cpus,
>> VIR_FREE(cpus);
>> }
>>
>> +void
>> +qemuMonitorQueryCpusFree(struct qemuMonitorQueryCpusEntry *entries,
>> + size_t nentries ATTRIBUTE_UNUSED)
>> +{
>> + if (!entries)
>> + return;
>
> [1] Maybe this should be a 'int' parameter and a <= 0 check...
What?! That's a freeing function. That does not make any sense.
oh right - my eyes read nentries... and I even noted in the caller that
nentries is not used.... Anyway, I suppose part of me was trying to
forward think why you would add nentries and that passing a -2 or -1
here may not end quickly...
John
3032
days inactive
3033
days old
29 comments
5 participants
participants (5)
-
Jiri Denemark -
John Ferlan -
Ján Tomko -
Pavel Hrdina -
Peter Krempa