8:58 a.m.
While reviewing the patch adding virtio-balloon-ccw,
I found that we are not reporting errors consistently.
This turned out to be on purpose.
This series
* moves the object path search into qemu_monitor_json
* reduces the number of allocations during search
(more a cosmetic chagne than optimization)
* moves the balloon model checking out of the monitor code
* vm->def should not be accessed without a virDomainObj lock
* this should also get rid of the errors on dommemstat with
'none' balloon reported by:
https://www.redhat.com/archives/libvir-list/2015-May/msg01110.html
* changes qemuMonitorFindBalloonObjectPath to void
to make it obvious that errors are ignored
Ján Tomko (10):
Move qemuMonitorFindObjectPath to qemu_monitor_json
Introduce qemuMonitorJSONFindLinkPath
Remove path argument from qemuMonitorJSONFindLinkPath
Add endjob label to qemuDomainMemoryStats
Invert the condition in qemuDomainMemoryStats
Only call qemuMonitorGetMemoryStats for virtio memballoon
Check for balloon model in qemuDomainSetMemoryStatsPeriod
Only call SetMemoryStatsPeriod for virtio memballoon
Do not access the domain definition in
qemuMonitorFindBalloonObjectPath
Turn qemuMonitorFindBalloonObjectPath into a void function
src/qemu/qemu_driver.c | 49 +++++++++++++-----
src/qemu/qemu_monitor.c | 116 +++++--------------------------------------
src/qemu/qemu_monitor_json.c | 92 ++++++++++++++++++++++++++++++++++
src/qemu/qemu_monitor_json.h | 3 ++
src/qemu/qemu_process.c | 4 +-
5 files changed, 146 insertions(+), 118 deletions(-)
--
2.3.6
8:58 a.m.
New subject: [libvirt] [PATCH 01/10] Move qemuMonitorFindObjectPath to qemu_monitor_json
This function is specific to the JSON monitor.
---
src/qemu/qemu_monitor.c | 76 ++------------------------------------------
src/qemu/qemu_monitor_json.c | 72 +++++++++++++++++++++++++++++++++++++++++
src/qemu/qemu_monitor_json.h | 4 +++
3 files changed, 78 insertions(+), 74 deletions(-)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index e9c57f1..d761f51 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -1069,78 +1069,6 @@ qemuMonitorSetOptions(qemuMonitorPtr mon, virJSONValuePtr options)
/**
- * Search the qom objects by it's known name. The name is compared against
- * filed 'type' formatted as 'link<%name>'.
- *
- * This procedure will be call recursively until found or the qom-list is
- * exhausted.
- *
- * Returns:
- *
- * 0 - Found
- * -1 - Error bail out
- * -2 - Not found
- *
- * NOTE: This assumes we have already called qemuDomainObjEnterMonitor()
- */
-static int
-qemuMonitorFindObjectPath(qemuMonitorPtr mon,
- const char *curpath,
- const char *name,
- char **path)
-{
- ssize_t i, npaths = 0;
- int ret = -2;
- char *nextpath = NULL;
- char *type = NULL;
- qemuMonitorJSONListPathPtr *paths = NULL;
-
- if (virAsprintf(&type, "link<%s>", name) < 0)
- return -1;
-
- VIR_DEBUG("Searching for '%s' Object Path starting at
'%s'", type, curpath);
-
- npaths = qemuMonitorJSONGetObjectListPaths(mon, curpath, &paths);
- if (npaths < 0)
- goto cleanup;
-
- for (i = 0; i < npaths && ret == -2; i++) {
-
- if (STREQ_NULLABLE(paths[i]->type, type)) {
- VIR_DEBUG("Path to '%s' is '%s/%s'", type, curpath,
paths[i]->name);
- ret = 0;
- if (virAsprintf(path, "%s/%s", curpath, paths[i]->name) < 0)
{
- *path = NULL;
- ret = -1;
- }
- goto cleanup;
- }
-
- /* Type entries that begin with "child<" are a branch that can be
- * traversed looking for more entries
- */
- if (paths[i]->type && STRPREFIX(paths[i]->type,
"child<")) {
- if (virAsprintf(&nextpath, "%s/%s", curpath, paths[i]->name)
< 0) {
- ret = -1;
- goto cleanup;
- }
-
- ret = qemuMonitorFindObjectPath(mon, nextpath, name, path);
- VIR_FREE(nextpath);
- }
- }
-
- cleanup:
- for (i = 0; i < npaths; i++)
- qemuMonitorJSONListPathFree(paths[i]);
- VIR_FREE(paths);
- VIR_FREE(nextpath);
- VIR_FREE(type);
- return ret;
-}
-
-
-/**
* Search the qom objects for the balloon driver object by it's known name
* of "virtio-balloon-pci". The entry for the driver will be found by using
* function "qemuMonitorFindObjectPath".
@@ -1183,7 +1111,7 @@ qemuMonitorFindBalloonObjectPath(qemuMonitorPtr mon,
return -1;
}
- if (qemuMonitorFindObjectPath(mon, curpath, "virtio-balloon-pci",
&path) < 0)
+ if (qemuMonitorJSONFindObjectPath(mon, curpath, "virtio-balloon-pci",
&path) < 0)
return -1;
nprops = qemuMonitorJSONGetObjectListPaths(mon, path, &bprops);
@@ -1232,7 +1160,7 @@ qemuMonitorUpdateVideoMemorySize(qemuMonitorPtr mon,
QEMU_CHECK_MONITOR(mon);
if (mon->json) {
- ret = qemuMonitorFindObjectPath(mon, "/", videoName, &path);
+ ret = qemuMonitorJSONFindObjectPath(mon, "/", videoName, &path);
if (ret < 0) {
if (ret == -2)
virReportError(VIR_ERR_INTERNAL_ERROR,
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index e6da804..69c342d 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -6642,3 +6642,75 @@ qemuMonitorJSONGetMemoryDeviceInfo(qemuMonitorPtr mon,
virJSONValueFree(reply);
return ret;
}
+
+
+/**
+ * Search the qom objects by it's known name. The name is compared against
+ * filed 'type' formatted as 'link<%name>'.
+ *
+ * This procedure will be call recursively until found or the qom-list is
+ * exhausted.
+ *
+ * Returns:
+ *
+ * 0 - Found
+ * -1 - Error bail out
+ * -2 - Not found
+ *
+ * NOTE: This assumes we have already called qemuDomainObjEnterMonitor()
+ */
+int
+qemuMonitorJSONFindObjectPath(qemuMonitorPtr mon,
+ const char *curpath,
+ const char *name,
+ char **path)
+{
+ ssize_t i, npaths = 0;
+ int ret = -2;
+ char *nextpath = NULL;
+ char *type = NULL;
+ qemuMonitorJSONListPathPtr *paths = NULL;
+
+ if (virAsprintf(&type, "link<%s>", name) < 0)
+ return -1;
+
+ VIR_DEBUG("Searching for '%s' Object Path starting at
'%s'", type, curpath);
+
+ npaths = qemuMonitorJSONGetObjectListPaths(mon, curpath, &paths);
+ if (npaths < 0)
+ goto cleanup;
+
+ for (i = 0; i < npaths && ret == -2; i++) {
+
+ if (STREQ_NULLABLE(paths[i]->type, type)) {
+ VIR_DEBUG("Path to '%s' is '%s/%s'", type, curpath,
paths[i]->name);
+ ret = 0;
+ if (virAsprintf(path, "%s/%s", curpath, paths[i]->name) < 0)
{
+ *path = NULL;
+ ret = -1;
+ }
+ goto cleanup;
+ }
+
+ /* Type entries that begin with "child<" are a branch that can be
+ * traversed looking for more entries
+ */
+ if (paths[i]->type && STRPREFIX(paths[i]->type,
"child<")) {
+ if (virAsprintf(&nextpath, "%s/%s", curpath, paths[i]->name)
< 0) {
+ ret = -1;
+ goto cleanup;
+ }
+
+ ret = qemuMonitorJSONFindObjectPath(mon, nextpath, name, path);
+ VIR_FREE(nextpath);
+ }
+ }
+
+ cleanup:
+ for (i = 0; i < npaths; i++)
+ qemuMonitorJSONListPathFree(paths[i]);
+ VIR_FREE(paths);
+ VIR_FREE(nextpath);
+ VIR_FREE(type);
+ return ret;
+}
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index 05c9b29..fcc2c86 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -482,4 +482,8 @@ int qemuMonitorJSONGetMemoryDeviceInfo(qemuMonitorPtr mon,
virHashTablePtr info)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
+int qemuMonitorJSONFindObjectPath(qemuMonitorPtr mon,
+ const char *curpath,
+ const char *name,
+ char **path);
#endif /* QEMU_MONITOR_JSON_H */
--
2.3.6
8:58 a.m.
New subject: [libvirt] [PATCH 02/10] Introduce qemuMonitorJSONFindLinkPath
When traversing through the QOM tree, we're looking for
a link to a device, e.g.:
link<virtio-balloon-pci>
Introduce a helper that will format the link name at the start,
instead of doing it every time while recursing through the tree.
---
src/qemu/qemu_monitor.c | 4 ++--
src/qemu/qemu_monitor_json.c | 55 ++++++++++++++++++++++++++++++--------------
src/qemu/qemu_monitor_json.h | 8 +++----
3 files changed, 44 insertions(+), 23 deletions(-)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index d761f51..4a5e13c 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -1111,7 +1111,7 @@ qemuMonitorFindBalloonObjectPath(qemuMonitorPtr mon,
return -1;
}
- if (qemuMonitorJSONFindObjectPath(mon, curpath, "virtio-balloon-pci",
&path) < 0)
+ if (qemuMonitorJSONFindLinkPath(mon, curpath, "virtio-balloon-pci",
&path) < 0)
return -1;
nprops = qemuMonitorJSONGetObjectListPaths(mon, path, &bprops);
@@ -1160,7 +1160,7 @@ qemuMonitorUpdateVideoMemorySize(qemuMonitorPtr mon,
QEMU_CHECK_MONITOR(mon);
if (mon->json) {
- ret = qemuMonitorJSONFindObjectPath(mon, "/", videoName, &path);
+ ret = qemuMonitorJSONFindLinkPath(mon, "/", videoName, &path);
if (ret < 0) {
if (ret == -2)
virReportError(VIR_ERR_INTERNAL_ERROR,
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 69c342d..6fafe81 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -6645,21 +6645,18 @@ qemuMonitorJSONGetMemoryDeviceInfo(qemuMonitorPtr mon,
/**
- * Search the qom objects by it's known name. The name is compared against
- * filed 'type' formatted as 'link<%name>'.
+ * Recursively search for a QOM object link.
*
- * This procedure will be call recursively until found or the qom-list is
- * exhausted.
+ * For @name, this function finds the first QOM object
+ * named @name, recursively going through all the "child<>"
+ * entries, starting from @curpath.
*
* Returns:
- *
* 0 - Found
- * -1 - Error bail out
+ * -1 - Error - bail out
* -2 - Not found
- *
- * NOTE: This assumes we have already called qemuDomainObjEnterMonitor()
*/
-int
+static int
qemuMonitorJSONFindObjectPath(qemuMonitorPtr mon,
const char *curpath,
const char *name,
@@ -6668,13 +6665,9 @@ qemuMonitorJSONFindObjectPath(qemuMonitorPtr mon,
ssize_t i, npaths = 0;
int ret = -2;
char *nextpath = NULL;
- char *type = NULL;
qemuMonitorJSONListPathPtr *paths = NULL;
- if (virAsprintf(&type, "link<%s>", name) < 0)
- return -1;
-
- VIR_DEBUG("Searching for '%s' Object Path starting at
'%s'", type, curpath);
+ VIR_DEBUG("Searching for '%s' Object Path starting at
'%s'", name, curpath);
npaths = qemuMonitorJSONGetObjectListPaths(mon, curpath, &paths);
if (npaths < 0)
@@ -6682,8 +6675,8 @@ qemuMonitorJSONFindObjectPath(qemuMonitorPtr mon,
for (i = 0; i < npaths && ret == -2; i++) {
- if (STREQ_NULLABLE(paths[i]->type, type)) {
- VIR_DEBUG("Path to '%s' is '%s/%s'", type, curpath,
paths[i]->name);
+ if (STREQ_NULLABLE(paths[i]->type, name)) {
+ VIR_DEBUG("Path to '%s' is '%s/%s'", name, curpath,
paths[i]->name);
ret = 0;
if (virAsprintf(path, "%s/%s", curpath, paths[i]->name) < 0)
{
*path = NULL;
@@ -6711,6 +6704,34 @@ qemuMonitorJSONFindObjectPath(qemuMonitorPtr mon,
qemuMonitorJSONListPathFree(paths[i]);
VIR_FREE(paths);
VIR_FREE(nextpath);
- VIR_FREE(type);
+ return ret;
+}
+
+
+/**
+ * Recursively search for a QOM object link.
+ *
+ * For @name, this function finds the first QOM object
+ * pointed to by a link in the form of 'link<@name>'
+ *
+ * Returns:
+ * 0 - Found
+ * -1 - Error
+ * -2 - Not found
+ */
+int
+qemuMonitorJSONFindLinkPath(qemuMonitorPtr mon,
+ const char *curpath,
+ const char *name,
+ char **path)
+{
+ char *linkname = NULL;
+ int ret = -1;
+
+ if (virAsprintf(&linkname, "link<%s>", name) < 0)
+ return -1;
+
+ ret = qemuMonitorJSONFindObjectPath(mon, curpath, linkname, path);
+ VIR_FREE(linkname);
return ret;
}
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index fcc2c86..953266c 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -482,8 +482,8 @@ int qemuMonitorJSONGetMemoryDeviceInfo(qemuMonitorPtr mon,
virHashTablePtr info)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
-int qemuMonitorJSONFindObjectPath(qemuMonitorPtr mon,
- const char *curpath,
- const char *name,
- char **path);
+int qemuMonitorJSONFindLinkPath(qemuMonitorPtr mon,
+ const char *curpath,
+ const char *name,
+ char **path);
#endif /* QEMU_MONITOR_JSON_H */
--
2.3.6
6:47 a.m.
New subject: [libvirt] [PATCH 02/10] Introduce qemuMonitorJSONFindLinkPath
On 06/04/2015 09:58 AM, Ján Tomko wrote:
When traversing through the QOM tree, we're looking for
a link to a device, e.g.:
link<virtio-balloon-pci>
Introduce a helper that will format the link name at the start,
instead of doing it every time while recursing through the tree.
---
src/qemu/qemu_monitor.c | 4 ++--
src/qemu/qemu_monitor_json.c | 55 ++++++++++++++++++++++++++++++--------------
src/qemu/qemu_monitor_json.h | 8 +++----
3 files changed, 44 insertions(+), 23 deletions(-)
Should we add any ATTRIBUTE_NONNULL() to the new API defs in the .h
file? For the 4 args? Like other external API's... I'm OK avoiding it
for patch 1 since patch 2 is the eventual API.
ACK 1 & 2
John
8:58 a.m.
New subject: [libvirt] [PATCH 03/10] Remove path argument from qemuMonitorJSONFindLinkPath
All the callers use "/" anyway.
---
src/qemu/qemu_monitor.c | 11 +++++------
src/qemu/qemu_monitor_json.c | 3 +--
src/qemu/qemu_monitor_json.h | 1 -
3 files changed, 6 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 4a5e13c..9add05c 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -1085,8 +1085,7 @@ qemuMonitorSetOptions(qemuMonitorPtr mon, virJSONValuePtr options)
* NOTE: This assumes we have already called qemuDomainObjEnterMonitor()
*/
static int
-qemuMonitorFindBalloonObjectPath(qemuMonitorPtr mon,
- const char *curpath)
+qemuMonitorFindBalloonObjectPath(qemuMonitorPtr mon)
{
ssize_t i, nprops = 0;
int ret = -1;
@@ -1111,7 +1110,7 @@ qemuMonitorFindBalloonObjectPath(qemuMonitorPtr mon,
return -1;
}
- if (qemuMonitorJSONFindLinkPath(mon, curpath, "virtio-balloon-pci",
&path) < 0)
+ if (qemuMonitorJSONFindLinkPath(mon, "virtio-balloon-pci", &path) <
0)
return -1;
nprops = qemuMonitorJSONGetObjectListPaths(mon, path, &bprops);
@@ -1160,7 +1159,7 @@ qemuMonitorUpdateVideoMemorySize(qemuMonitorPtr mon,
QEMU_CHECK_MONITOR(mon);
if (mon->json) {
- ret = qemuMonitorJSONFindLinkPath(mon, "/", videoName, &path);
+ ret = qemuMonitorJSONFindLinkPath(mon, videoName, &path);
if (ret < 0) {
if (ret == -2)
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -1643,7 +1642,7 @@ qemuMonitorGetMemoryStats(qemuMonitorPtr mon,
QEMU_CHECK_MONITOR(mon);
if (mon->json) {
- ignore_value(qemuMonitorFindBalloonObjectPath(mon, "/"));
+ ignore_value(qemuMonitorFindBalloonObjectPath(mon));
mon->ballooninit = true;
return qemuMonitorJSONGetMemoryStats(mon, mon->balloonpath,
stats, nr_stats);
@@ -1676,7 +1675,7 @@ qemuMonitorSetMemoryStatsPeriod(qemuMonitorPtr mon,
if (period < 0)
return -1;
- if (qemuMonitorFindBalloonObjectPath(mon, "/") == 0) {
+ if (qemuMonitorFindBalloonObjectPath(mon) == 0) {
ret = qemuMonitorJSONSetMemoryStatsPeriod(mon, mon->balloonpath,
period);
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 6fafe81..13c57d2 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -6721,7 +6721,6 @@ qemuMonitorJSONFindObjectPath(qemuMonitorPtr mon,
*/
int
qemuMonitorJSONFindLinkPath(qemuMonitorPtr mon,
- const char *curpath,
const char *name,
char **path)
{
@@ -6731,7 +6730,7 @@ qemuMonitorJSONFindLinkPath(qemuMonitorPtr mon,
if (virAsprintf(&linkname, "link<%s>", name) < 0)
return -1;
- ret = qemuMonitorJSONFindObjectPath(mon, curpath, linkname, path);
+ ret = qemuMonitorJSONFindObjectPath(mon, "/", linkname, path);
VIR_FREE(linkname);
return ret;
}
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index 953266c..ae8ef7c 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -483,7 +483,6 @@ int qemuMonitorJSONGetMemoryDeviceInfo(qemuMonitorPtr mon,
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
int qemuMonitorJSONFindLinkPath(qemuMonitorPtr mon,
- const char *curpath,
const char *name,
char **path);
#endif /* QEMU_MONITOR_JSON_H */
--
2.3.6
6:47 a.m.
New subject: [libvirt] [PATCH 03/10] Remove path argument from qemuMonitorJSONFindLinkPath
On 06/04/2015 09:58 AM, Ján Tomko wrote:
All the callers use "/" anyway.
---
src/qemu/qemu_monitor.c | 11 +++++------
src/qemu/qemu_monitor_json.c | 3 +--
src/qemu/qemu_monitor_json.h | 1 -
3 files changed, 6 insertions(+), 9 deletions(-)
Originally (commit id 'ffdf82a9d'), qemuMonitorFindBalloonObjectPath was
recursive and would need to adjust the path argument when a "child<"
element was found in order to perform proper tree traversal.
Now that it's a couple levels deeper - we don't need curpath although
perhaps someone, some day could find a use for it. I'm not objecting to
removing, just pointing out we could keep it as perhaps "startpath"
which if NULL, would default to "/". And yes, that could be left as an
exercise for such a future usage.
ACK to what's here, your call on whether to change the name/usage...
John
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 4a5e13c..9add05c 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -1085,8 +1085,7 @@ qemuMonitorSetOptions(qemuMonitorPtr mon, virJSONValuePtr options)
* NOTE: This assumes we have already called qemuDomainObjEnterMonitor()
*/
static int
-qemuMonitorFindBalloonObjectPath(qemuMonitorPtr mon,
- const char *curpath)
+qemuMonitorFindBalloonObjectPath(qemuMonitorPtr mon)
{
ssize_t i, nprops = 0;
int ret = -1;
@@ -1111,7 +1110,7 @@ qemuMonitorFindBalloonObjectPath(qemuMonitorPtr mon,
return -1;
}
- if (qemuMonitorJSONFindLinkPath(mon, curpath, "virtio-balloon-pci",
&path) < 0)
+ if (qemuMonitorJSONFindLinkPath(mon, "virtio-balloon-pci", &path) <
0)
return -1;
nprops = qemuMonitorJSONGetObjectListPaths(mon, path, &bprops);
@@ -1160,7 +1159,7 @@ qemuMonitorUpdateVideoMemorySize(qemuMonitorPtr mon,
QEMU_CHECK_MONITOR(mon);
if (mon->json) {
- ret = qemuMonitorJSONFindLinkPath(mon, "/", videoName, &path);
+ ret = qemuMonitorJSONFindLinkPath(mon, videoName, &path);
if (ret < 0) {
if (ret == -2)
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -1643,7 +1642,7 @@ qemuMonitorGetMemoryStats(qemuMonitorPtr mon,
QEMU_CHECK_MONITOR(mon);
if (mon->json) {
- ignore_value(qemuMonitorFindBalloonObjectPath(mon, "/"));
+ ignore_value(qemuMonitorFindBalloonObjectPath(mon));
mon->ballooninit = true;
return qemuMonitorJSONGetMemoryStats(mon, mon->balloonpath,
stats, nr_stats);
@@ -1676,7 +1675,7 @@ qemuMonitorSetMemoryStatsPeriod(qemuMonitorPtr mon,
if (period < 0)
return -1;
- if (qemuMonitorFindBalloonObjectPath(mon, "/") == 0) {
+ if (qemuMonitorFindBalloonObjectPath(mon) == 0) {
ret = qemuMonitorJSONSetMemoryStatsPeriod(mon, mon->balloonpath,
period);
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 6fafe81..13c57d2 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -6721,7 +6721,6 @@ qemuMonitorJSONFindObjectPath(qemuMonitorPtr mon,
*/
int
qemuMonitorJSONFindLinkPath(qemuMonitorPtr mon,
- const char *curpath,
const char *name,
char **path)
{
@@ -6731,7 +6730,7 @@ qemuMonitorJSONFindLinkPath(qemuMonitorPtr mon,
if (virAsprintf(&linkname, "link<%s>", name) < 0)
return -1;
- ret = qemuMonitorJSONFindObjectPath(mon, curpath, linkname, path);
+ ret = qemuMonitorJSONFindObjectPath(mon, "/", linkname, path);
VIR_FREE(linkname);
return ret;
}
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index 953266c..ae8ef7c 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -483,7 +483,6 @@ int qemuMonitorJSONGetMemoryDeviceInfo(qemuMonitorPtr mon,
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
int qemuMonitorJSONFindLinkPath(qemuMonitorPtr mon,
- const char *curpath,
const char *name,
char **path);
#endif /* QEMU_MONITOR_JSON_H */
8:58 a.m.
New subject: [libvirt] [PATCH 04/10] Add endjob label to qemuDomainMemoryStats
Reduce the indentation level.
---
src/qemu/qemu_driver.c | 35 +++++++++++++++++++----------------
1 file changed, 19 insertions(+), 16 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index e031a17..818862b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11577,6 +11577,7 @@ qemuDomainMemoryStats(virDomainPtr dom,
unsigned int flags)
{
virQEMUDriverPtr driver = dom->conn->privateData;
+ qemuDomainObjPrivatePtr priv;
virDomainObjPtr vm;
int ret = -1;
@@ -11594,27 +11595,29 @@ qemuDomainMemoryStats(virDomainPtr dom,
if (!virDomainObjIsActive(vm)) {
virReportError(VIR_ERR_OPERATION_INVALID,
"%s", _("domain is not running"));
- } else {
- qemuDomainObjPrivatePtr priv = vm->privateData;
- qemuDomainObjEnterMonitor(driver, vm);
- ret = qemuMonitorGetMemoryStats(priv->mon, stats, nr_stats);
- if (qemuDomainObjExitMonitor(driver, vm) < 0)
- ret = -1;
+ goto endjob;
+ }
- if (ret >= 0 && ret < nr_stats) {
- long rss;
- if (qemuGetProcessInfo(NULL, NULL, &rss, vm->pid, 0) < 0) {
- virReportError(VIR_ERR_OPERATION_FAILED, "%s",
- _("cannot get RSS for domain"));
- } else {
- stats[ret].tag = VIR_DOMAIN_MEMORY_STAT_RSS;
- stats[ret].val = rss;
- ret++;
- }
+ priv = vm->privateData;
+ qemuDomainObjEnterMonitor(driver, vm);
+ ret = qemuMonitorGetMemoryStats(priv->mon, stats, nr_stats);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
+ if (ret >= 0 && ret < nr_stats) {
+ long rss;
+ if (qemuGetProcessInfo(NULL, NULL, &rss, vm->pid, 0) < 0) {
+ virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+ _("cannot get RSS for domain"));
+ } else {
+ stats[ret].tag = VIR_DOMAIN_MEMORY_STAT_RSS;
+ stats[ret].val = rss;
+ ret++;
}
+
}
+ endjob:
qemuDomainObjEndJob(driver, vm);
cleanup:
--
2.3.6
8:58 a.m.
New subject: [libvirt] [PATCH 05/10] Invert the condition in qemuDomainMemoryStats
It only makes sense if qemuMonitorGetMemoryStats is called,
but the following patch will make that call conditional.
---
src/qemu/qemu_driver.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 818862b..50eebf9 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11580,6 +11580,7 @@ qemuDomainMemoryStats(virDomainPtr dom,
qemuDomainObjPrivatePtr priv;
virDomainObjPtr vm;
int ret = -1;
+ long rss;
virCheckFlags(0, -1);
@@ -11604,17 +11605,16 @@ qemuDomainMemoryStats(virDomainPtr dom,
if (qemuDomainObjExitMonitor(driver, vm) < 0)
ret = -1;
- if (ret >= 0 && ret < nr_stats) {
- long rss;
- if (qemuGetProcessInfo(NULL, NULL, &rss, vm->pid, 0) < 0) {
- virReportError(VIR_ERR_OPERATION_FAILED, "%s",
- _("cannot get RSS for domain"));
- } else {
- stats[ret].tag = VIR_DOMAIN_MEMORY_STAT_RSS;
- stats[ret].val = rss;
- ret++;
- }
+ if (ret < 0 || ret >= nr_stats)
+ goto endjob;
+ if (qemuGetProcessInfo(NULL, NULL, &rss, vm->pid, 0) < 0) {
+ virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+ _("cannot get RSS for domain"));
+ } else {
+ stats[ret].tag = VIR_DOMAIN_MEMORY_STAT_RSS;
+ stats[ret].val = rss;
+ ret++;
}
endjob:
--
2.3.6
8:58 a.m.
New subject: [libvirt] [PATCH 06/10] Only call qemuMonitorGetMemoryStats for virtio memballoon
There is nothing to get from the monitor for model='none'.
---
src/qemu/qemu_driver.c | 19 ++++++++++++-------
1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 50eebf9..4690406 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11599,14 +11599,19 @@ qemuDomainMemoryStats(virDomainPtr dom,
goto endjob;
}
- priv = vm->privateData;
- qemuDomainObjEnterMonitor(driver, vm);
- ret = qemuMonitorGetMemoryStats(priv->mon, stats, nr_stats);
- if (qemuDomainObjExitMonitor(driver, vm) < 0)
- ret = -1;
+ if (vm->def->memballoon &&
+ vm->def->memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO) {
+ priv = vm->privateData;
+ qemuDomainObjEnterMonitor(driver, vm);
+ ret = qemuMonitorGetMemoryStats(priv->mon, stats, nr_stats);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
- if (ret < 0 || ret >= nr_stats)
- goto endjob;
+ if (ret < 0 || ret >= nr_stats)
+ goto endjob;
+ } else {
+ ret = 0;
+ }
if (qemuGetProcessInfo(NULL, NULL, &rss, vm->pid, 0) < 0) {
virReportError(VIR_ERR_OPERATION_FAILED, "%s",
--
2.3.6
6:54 a.m.
New subject: [libvirt] [PATCH 06/10] Only call qemuMonitorGetMemoryStats for virtio memballoon
On 06/04/2015 09:58 AM, Ján Tomko wrote:
There is nothing to get from the monitor for model='none'.
---
src/qemu/qemu_driver.c | 19 ++++++++++++-------
1 file changed, 12 insertions(+), 7 deletions(-)
ACK 4-6
John
8:58 a.m.
New subject: [libvirt] [PATCH 07/10] Check for balloon model in qemuDomainSetMemoryStatsPeriod
There's no point in calling the monitor if there is no balloon.
---
src/qemu/qemu_driver.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 4690406..bfd59a9 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2459,6 +2459,14 @@ static int qemuDomainSetMemoryStatsPeriod(virDomainPtr dom, int
period,
priv = vm->privateData;
if (def) {
+ if (!def->memballoon ||
+ def->memballoon->model != VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Memory balloon model must be virtio to set the"
+ " collection period"));
+ goto endjob;
+ }
+
qemuDomainObjEnterMonitor(driver, vm);
r = qemuMonitorSetMemoryStatsPeriod(priv->mon, period);
if (qemuDomainObjExitMonitor(driver, vm) < 0)
@@ -2475,6 +2483,13 @@ static int qemuDomainSetMemoryStatsPeriod(virDomainPtr dom, int
period,
}
if (persistentDef) {
+ if (!def->memballoon ||
+ def->memballoon->model != VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Memory balloon model must be virtio to set the"
+ " collection period"));
+ goto endjob;
+ }
persistentDef->memballoon->period = period;
ret = virDomainSaveConfig(cfg->configDir, persistentDef);
goto endjob;
--
2.3.6
6:55 a.m.
New subject: [libvirt] [PATCH 07/10] Check for balloon model in qemuDomainSetMemoryStatsPeriod
On 06/04/2015 09:58 AM, Ján Tomko wrote:
There's no point in calling the monitor if there is no balloon.
---
src/qemu/qemu_driver.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
Coverity found...
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 4690406..bfd59a9 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2459,6 +2459,14 @@ static int qemuDomainSetMemoryStatsPeriod(virDomainPtr dom, int
period,
priv = vm->privateData;
if (def) {
Checking 'def' here...
+ if (!def->memballoon ||
+ def->memballoon->model != VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Memory balloon model must be virtio to set the"
+ " collection period"));
+ goto endjob;
+ }
+
qemuDomainObjEnterMonitor(driver, vm);
r = qemuMonitorSetMemoryStatsPeriod(priv->mon, period);
if (qemuDomainObjExitMonitor(driver, vm) < 0)
@@ -2475,6 +2483,13 @@ static int qemuDomainSetMemoryStatsPeriod(virDomainPtr dom, int
period,
}
if (persistentDef) {
+ if (!def->memballoon ||
Assuming 'def' here.
I think you meant persistentDef->...
ACK w/ the adjustment
John
+ def->memballoon->model !=
VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Memory balloon model must be virtio to set the"
+ " collection period"));
+ goto endjob;
+ }
persistentDef->memballoon->period = period;
ret = virDomainSaveConfig(cfg->configDir, persistentDef);
goto endjob;
9:24 a.m.
New subject: [libvirt] [PATCH 07/10] Check for balloon model in qemuDomainSetMemoryStatsPeriod
On Fri, Jun 05, 2015 at 07:55:52AM -0400, John Ferlan wrote:
On 06/04/2015 09:58 AM, Ján Tomko wrote:
> There's no point in calling the monitor if there is no balloon.
> ---
> src/qemu/qemu_driver.c | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
Coverity found...
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 4690406..bfd59a9 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -2459,6 +2459,14 @@ static int qemuDomainSetMemoryStatsPeriod(virDomainPtr dom,
int period,
> priv = vm->privateData;
>
> if (def) {
Checking 'def' here...
> + if (!def->memballoon ||
> + def->memballoon->model != VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Memory balloon model must be virtio to set
the"
> + " collection period"));
> + goto endjob;
> + }
> +
> qemuDomainObjEnterMonitor(driver, vm);
> r = qemuMonitorSetMemoryStatsPeriod(priv->mon, period);
> if (qemuDomainObjExitMonitor(driver, vm) < 0)
> @@ -2475,6 +2483,13 @@ static int qemuDomainSetMemoryStatsPeriod(virDomainPtr dom,
int period,
> }
>
> if (persistentDef) {
> + if (!def->memballoon ||
Assuming 'def' here.
I think you meant persistentDef->...
ACK w/ the adjustment
I fixed it to use persistentDef (also the typo in 10/10) and pushed the
series.
Thanks for the review.
Jan
8:58 a.m.
New subject: [libvirt] [PATCH 08/10] Only call SetMemoryStatsPeriod for virtio memballoon
---
src/qemu/qemu_process.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 7f154f0..64ee049 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -5517,7 +5517,9 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED,
if (running) {
virDomainObjSetState(vm, VIR_DOMAIN_RUNNING,
VIR_DOMAIN_RUNNING_UNPAUSED);
- if (vm->def->memballoon && vm->def->memballoon->period) {
+ if (vm->def->memballoon &&
+ vm->def->memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO
&&
+ vm->def->memballoon->period) {
qemuDomainObjEnterMonitor(driver, vm);
qemuMonitorSetMemoryStatsPeriod(priv->mon,
vm->def->memballoon->period);
--
2.3.6
6:58 a.m.
New subject: [libvirt] [PATCH 08/10] Only call SetMemoryStatsPeriod for virtio memballoon
On 06/04/2015 09:58 AM, Ján Tomko wrote:
---
src/qemu/qemu_process.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 7f154f0..64ee049 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -5517,7 +5517,9 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED,
if (running) {
virDomainObjSetState(vm, VIR_DOMAIN_RUNNING,
VIR_DOMAIN_RUNNING_UNPAUSED);
- if (vm->def->memballoon && vm->def->memballoon->period)
{
+ if (vm->def->memballoon &&
+ vm->def->memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO
&&
+ vm->def->memballoon->period) {
Setting a period == 0 means to disable collection IIRC... yep, true from
the man page...
ACK if you remove the vm->def->memballoon->period condition
John
qemuDomainObjEnterMonitor(driver, vm);
qemuMonitorSetMemoryStatsPeriod(priv->mon,
vm->def->memballoon->period);
7:07 a.m.
New subject: [libvirt] [PATCH 08/10] Only call SetMemoryStatsPeriod for virtio memballoon
On 06/05/2015 07:58 AM, John Ferlan wrote:
On 06/04/2015 09:58 AM, Ján Tomko wrote:
> ---
> src/qemu/qemu_process.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 7f154f0..64ee049 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -5517,7 +5517,9 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED,
> if (running) {
> virDomainObjSetState(vm, VIR_DOMAIN_RUNNING,
> VIR_DOMAIN_RUNNING_UNPAUSED);
> - if (vm->def->memballoon &&
vm->def->memballoon->period) {
> + if (vm->def->memballoon &&
> + vm->def->memballoon->model ==
VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO &&
> + vm->def->memballoon->period) {
Setting a period == 0 means to disable collection IIRC... yep, true from
the man page...
ACK if you remove the vm->def->memballoon->period condition
John
oh... hmmm brain slower than eyes and fingers... qemuProcessAttach...
nevermind - no need to remove...
John
> qemuDomainObjEnterMonitor(driver, vm);
> qemuMonitorSetMemoryStatsPeriod(priv->mon,
> vm->def->memballoon->period);
>
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
8:58 a.m.
New subject: [libvirt] [PATCH 09/10] Do not access the domain definition in qemuMonitorFindBalloonObjectPath
The monitor code does not hold the virDomainObjPtr lock and should
not access the defitinion.
---
src/qemu/qemu_monitor.c | 10 ----------
1 file changed, 10 deletions(-)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 9add05c..6947b08 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -1091,7 +1091,6 @@ qemuMonitorFindBalloonObjectPath(qemuMonitorPtr mon)
int ret = -1;
char *path = NULL;
qemuMonitorJSONListPathPtr *bprops = NULL;
- virDomainObjPtr vm = mon->vm;
if (mon->balloonpath) {
return 0;
@@ -1101,15 +1100,6 @@ qemuMonitorFindBalloonObjectPath(qemuMonitorPtr mon)
return -1;
}
- /* Not supported */
- if (!vm->def->memballoon ||
- vm->def->memballoon->model != VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Memory balloon model must be virtio to "
- "get memballoon path"));
- return -1;
- }
-
if (qemuMonitorJSONFindLinkPath(mon, "virtio-balloon-pci", &path) <
0)
return -1;
--
2.3.6
7:08 a.m.
New subject: [libvirt] [PATCH 09/10] Do not access the domain definition in qemuMonitorFindBalloonObjectPath
On 06/04/2015 09:58 AM, Ján Tomko wrote:
The monitor code does not hold the virDomainObjPtr lock and should
not access the defitinion.
---
src/qemu/qemu_monitor.c | 10 ----------
1 file changed, 10 deletions(-)
ACK
John
8:58 a.m.
New subject: [libvirt] [PATCH 10/10] Turn qemuMonitorFindBalloonObjectPath into a void function
We were efectively ignoring its errors anyway.
---
src/qemu/qemu_monitor.c | 29 ++++++++++-------------------
1 file changed, 10 insertions(+), 19 deletions(-)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 6947b08..33600f0 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -1076,32 +1076,25 @@ qemuMonitorSetOptions(qemuMonitorPtr mon, virJSONValuePtr
options)
* Once found, check the entry to ensure it has the correct property listed.
* If it does not, then obtaining statistics from QEMU will not be possible.
* This feature was added to QEMU 1.5.
- *
- * Returns:
- *
- * 0 - Found
- * -1 - Not found or error
- *
- * NOTE: This assumes we have already called qemuDomainObjEnterMonitor()
*/
-static int
-qemuMonitorFindBalloonObjectPath(qemuMonitorPtr mon)
+static void
+qemuMonitorInitBalloonObjectPath(qemuMonitorPtr mon)
{
ssize_t i, nprops = 0;
- int ret = -1;
char *path = NULL;
qemuMonitorJSONListPathPtr *bprops = NULL;
if (mon->balloonpath) {
- return 0;
+ return;
} else if (mon->ballooninit) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Cannot determine balloon device path"));
- return -1;
+ return;
}
+ mon->ballooninit = true;
if (qemuMonitorJSONFindLinkPath(mon, "virtio-balloon-pci", &path) <
0)
- return -1;
+ return;
nprops = qemuMonitorJSONGetObjectListPaths(mon, path, &bprops);
if (nprops < 0)
@@ -1112,7 +1105,6 @@ qemuMonitorFindBalloonObjectPath(qemuMonitorPtr mon)
VIR_DEBUG("Found Balloon Object Path %s", path);
mon->balloonpath = path;
path = NULL;
- ret = 0;
goto cleanup;
}
}
@@ -1128,7 +1120,7 @@ qemuMonitorFindBalloonObjectPath(qemuMonitorPtr mon)
qemuMonitorJSONListPathFree(bprops[i]);
VIR_FREE(bprops);
VIR_FREE(path);
- return ret;
+ return;
}
@@ -1632,8 +1624,7 @@ qemuMonitorGetMemoryStats(qemuMonitorPtr mon,
QEMU_CHECK_MONITOR(mon);
if (mon->json) {
- ignore_value(qemuMonitorFindBalloonObjectPath(mon));
- mon->ballooninit = true;
+ qemuMonitorInitBalloonObjectPath(mon);
return qemuMonitorJSONGetMemoryStats(mon, mon->balloonpath,
stats, nr_stats);
} else {
@@ -1665,7 +1656,8 @@ qemuMonitorSetMemoryStatsPeriod(qemuMonitorPtr mon,
if (period < 0)
return -1;
- if (qemuMonitorFindBalloonObjectPath(mon) == 0) {
+ qemuMonitorInitBalloonObjectPath(mon);
+ if (mon->balloonpath) {
ret = qemuMonitorJSONSetMemoryStatsPeriod(mon, mon->balloonpath,
period);
@@ -1678,7 +1670,6 @@ qemuMonitorSetMemoryStatsPeriod(qemuMonitorPtr mon,
if (ret < 0)
virResetLastError();
}
- mon->ballooninit = true;
return ret;
}
--
2.3.6
7:10 a.m.
New subject: [libvirt] [PATCH 10/10] Turn qemuMonitorFindBalloonObjectPath into a void function
On 06/04/2015 09:58 AM, Ján Tomko wrote:
We were efectively ignoring its errors anyway.
typo: effectively
---
src/qemu/qemu_monitor.c | 29 ++++++++++-------------------
1 file changed, 10 insertions(+), 19 deletions(-)
ACK
John
3450
days inactive
3451
days old
19 comments
2 participants
participants (2)
-
John Ferlan -
Ján Tomko