[libvirt] [PATCH 00/10] qemu: balloon QOM-path related cleanups
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
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
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
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
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
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 */
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
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
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
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
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
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;
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
--- 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
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);
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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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
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
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
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
participants (2)
-
John Ferlan -
Ján Tomko