On 08.07.2013 21:20, John Ferlan wrote:

Add a new qemuMonitorJSONGetObjectListPaths() method to support invocation of the 'qom-list' JSON monitor command with a provided path.

The returned list of paired data fields of "name" and "type" that can be used to peruse QOM configuration data and eventually utilize for the balloon statistics.

The test does a "{"execute":"qom-list", "arguments": { "path": "/"}}" which returns "{"return": [{"name": "machine", "type": "child<container>"}, {"name": "type", "type": "string"}]}" resulting in a return of an array of 2 elements with [0].name="machine", [0].type="child<container>". The [1] entry appears to be a header that could be used some day via a command such as "virsh qemuobject --list" to format output. --- src/qemu/qemu_monitor_json.c | 102 +++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 16 +++++++ tests/qemumonitorjsontest.c | 79 +++++++++++++++++++++++++++++++++ 3 files changed, 197 insertions(+)

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 3383c88..fc2b65f 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4540,6 +4540,108 @@ cleanup: }

+int qemuMonitorJSONGetObjectListPaths(qemuMonitorPtr mon, + const char *path, + qemuMonitorJSONListPathPtr **paths) +{ + int ret; + virJSONValuePtr cmd; + virJSONValuePtr reply = NULL; + virJSONValuePtr data; + qemuMonitorJSONListPathPtr *pathlist = NULL; + int n = 0; + size_t i; + + *paths = NULL; + + if (!(cmd = qemuMonitorJSONMakeCommand("qom-list", + "s:path", path, + NULL))) + return -1; + + ret = qemuMonitorJSONCommand(mon, cmd, &reply); + + if (ret == 0) + ret = qemuMonitorJSONCheckError(cmd, reply); + + if (ret < 0) + goto cleanup; + + ret = -1; + + if (!(data = virJSONValueObjectGet(reply, "return"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("qom-list reply was missing return data")); + goto cleanup; + } + + if ((n = virJSONValueArraySize(data)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("qom-list reply data was not an array")); + goto cleanup; + } + + /* null-terminated list */

Do we need NULL terminated list ... [1]

+ if (VIR_ALLOC_N(pathlist, n + 1) < 0) { + virReportOOMError();

This can be dropped now. But as you've said in one of previous e-mails of your, you already did that.

+ goto cleanup; + } + + for (i = 0; i < n; i++) { + virJSONValuePtr child = virJSONValueArrayGet(data, i); + const char *tmp; + qemuMonitorJSONListPathPtr info; + + if (VIR_ALLOC(info) < 0) { + virReportOOMError(); + goto cleanup; + } + + pathlist[i] = info; + + if (!(tmp = virJSONValueObjectGetString(child, "name"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("qom-list reply data was missing 'name'")); + goto cleanup; + } + + if (VIR_STRDUP(info->name, tmp) < 0) + goto cleanup; + + if (virJSONValueObjectHasKey(child, "type")) { + if (!(tmp = virJSONValueObjectGetString(child, "type"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("qom-list reply has malformed 'type' data")); + goto cleanup; + } + if (VIR_STRDUP(info->type, tmp) < 0) + goto cleanup; + } + } + + ret = n;

1: ... if we are returning number of items in array? I'm not saying it's something wrong, just curious.

+ *paths = pathlist; + +cleanup: + if (ret < 0 && pathlist) { + for (i = 0; i < n; i++) + qemuMonitorJSONListPathFree(pathlist[i]); + VIR_FREE(pathlist); + } + virJSONValueFree(cmd); + virJSONValueFree(reply); + return ret; +} + +void qemuMonitorJSONListPathFree(qemuMonitorJSONListPathPtr paths) +{ + if (!paths) + return; + VIR_FREE(paths->name); + VIR_FREE(paths->type); + VIR_FREE(paths); +} + int qemuMonitorJSONGetObjectProps(qemuMonitorPtr mon, const char *type, char ***props) diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index d79b86b..e7ce145 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -332,6 +332,22 @@ int qemuMonitorJSONGetKVMState(qemuMonitorPtr mon, int qemuMonitorJSONGetObjectTypes(qemuMonitorPtr mon, char ***types) ATTRIBUTE_NONNULL(2); + +typedef struct _qemuMonitorJSONListPath qemuMonitorJSONListPath; +typedef qemuMonitorJSONListPath *qemuMonitorJSONListPathPtr; + +struct _qemuMonitorJSONListPath { + char *name; + char *type; +}; + +int qemuMonitorJSONGetObjectListPaths(qemuMonitorPtr mon, + const char *path, + qemuMonitorJSONListPathPtr **paths) + ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); + +void qemuMonitorJSONListPathFree(qemuMonitorJSONListPathPtr paths); + int qemuMonitorJSONGetObjectProps(qemuMonitorPtr mon, const char *type, char ***props) diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index acc94ca..380d377 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -23,6 +23,7 @@ #include "testutilsqemu.h" #include "qemumonitortestutils.h" #include "qemu/qemu_conf.h" +#include "qemu/qemu_monitor_json.h" #include "virthread.h" #include "virerror.h" #include "virstring.h" @@ -595,6 +596,83 @@ cleanup: }

+/* + * This test will request to return a list of paths for "/". It should be + * a simple list of 1 real element that being the "machine". The following + * is the execution and expected return: + * + * {"execute":"qom-list", "arguments": { "path": "/"}}" + * {"return": [{"name": "machine", "type": "child<container>"}, \ + * {"name": "type", "type": "string"}]} + */ +static int +testQemuMonitorJSONGetListPaths(const void *data) +{ + const virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data; + qemuMonitorTestPtr test = qemuMonitorTestNew(true, xmlopt); + int ret = -1; + qemuMonitorJSONListPathPtr *paths; + int npaths = 0; + int i;

And after Daniel's patches, this needs to be size_t i;

+ + if (!test) + return -1; + + if (qemuMonitorTestAddItem(test, "qom-list", + "{ " + " \"return\": [ " + " {\"name\": \"machine\", " + " \"type\": \"child<container>\"}, " + " {\"name\": \"type\", " + " \"type\": \"string\"} " + " ]" + "}") < 0) + goto cleanup; + + /* present with path */ + if ((npaths = qemuMonitorJSONGetObjectListPaths( + qemuMonitorTestGetMonitor(test), + "/", + &paths)) < 0) + goto cleanup; + + if (npaths != 2) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "npaths was %d, expected 1", npaths); + goto cleanup; + } + +#define CHECK(i, wantname, wanttype) \ + do { \ + if (STRNEQ(paths[i]->name, (wantname))) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, \ + "name was %s, expected %s", \ + paths[i]->name, (wantname)); \ + goto cleanup; \ + } \ + if (STRNEQ_NULLABLE(paths[i]->type, (wanttype))) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, \ + "type was %s, expected %s", \ + NULLSTR(paths[i]->type), (wanttype)); \ + goto cleanup; \ + } \ + } while (0) + + CHECK(0, "machine", "child<container>"); + +#undef CHECK + + ret = 0; + +cleanup: + qemuMonitorTestFree(test); + for (i = 0; i < npaths; i++) + qemuMonitorJSONListPathFree(paths[i]); + VIR_FREE(paths); + return ret; +} + + static int mymain(void) { @@ -623,6 +701,7 @@ mymain(void) DO_TEST(GetCommands); DO_TEST(GetTPMModels); DO_TEST(GetCommandLineOptionParameters); + DO_TEST(GetListPaths);

virObjectUnref(xmlopt);

Michal

On 08.07.2013 21:20, John Ferlan wrote:

Add a new qemuMonitorJSONGetObjectProperty() method to support invocation of the 'qom-get' JSON monitor command with a provided path, property, and expected data type return. The qemuMonitorJSONObjectProperty is similar to virTypedParameter; however, a future patch will extend it a bit to include a void pointer to balloon driver statistic data.

The provided test will execute a qom-get on "/machine/i440fx" which will return a property "realized". --- src/qemu/qemu_monitor_json.c | 86 ++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 34 ++++++++++++++++++ tests/qemumonitorjsontest.c | 48 +++++++++++++++++++++++++ 3 files changed, 168 insertions(+)

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index fc2b65f..db107f1 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4642,6 +4642,92 @@ void qemuMonitorJSONListPathFree(qemuMonitorJSONListPathPtr paths) VIR_FREE(paths); }

+ +int qemuMonitorJSONGetObjectProperty(qemuMonitorPtr mon, + const char *path, + const char *property, + qemuMonitorJSONObjectPropertyPtr prop) +{ + int ret; + virJSONValuePtr cmd; + virJSONValuePtr reply = NULL; + virJSONValuePtr data; + const char *tmp; + + if (!(cmd = qemuMonitorJSONMakeCommand("qom-get", + "s:path", path, + "s:property", property, + NULL))) + return -1; + + ret = qemuMonitorJSONCommand(mon, cmd, &reply); + + if (ret == 0) + ret = qemuMonitorJSONCheckError(cmd, reply); + + if (ret < 0) + goto cleanup; + + ret = -1; + + if (!(data = virJSONValueObjectGet(reply, "return"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("qom-get reply was missing return data")); + goto cleanup; + } + + switch (prop->type) {

switch ((qemuMonitorJSONObjectPropertyType) prop->type) {

+ /* Simple cases of boolean, int, long, uint, ulong, double, and string + * will receive return value as part of {"return": xxx} statement + */ + case QEMU_MONITOR_OBJECT_PROPERTY_BOOLEAN: + ret = virJSONValueGetBoolean(data, &prop->val.b); + break; + case QEMU_MONITOR_OBJECT_PROPERTY_INT: + ret = virJSONValueGetNumberInt(data, &prop->val.i); + break; + case QEMU_MONITOR_OBJECT_PROPERTY_LONG: + ret = virJSONValueGetNumberLong(data, &prop->val.l); + break; + case QEMU_MONITOR_OBJECT_PROPERTY_UINT: + ret = virJSONValueGetNumberUint(data, &prop->val.ui); + break; + case QEMU_MONITOR_OBJECT_PROPERTY_ULONG: + ret = virJSONValueGetNumberUlong(data, &prop->val.ul); + break; + case QEMU_MONITOR_OBJECT_PROPERTY_DOUBLE: + ret = virJSONValueGetNumberDouble(data, &prop->val.d); + break; + case QEMU_MONITOR_OBJECT_PROPERTY_STRING: + tmp = virJSONValueGetString(data); + if (tmp && VIR_STRDUP(prop->val.str, tmp) < 0) + goto cleanup; + if (tmp) + ret = 0; + break; + case QEMU_MONITOR_OBJECT_PROPERTY_LAST: + default:

Drop 'default' as it prevents compiler to check completeness coverage of enum in this switch(). The _LAST can be dropped as well among with virReportError() and subsequent cleanup then.

+ virReportError(VIR_ERR_INTERNAL_ERROR, + _("qom-get invalid object property type %d"), + prop->type); + goto cleanup; + } + + if (ret == -1) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("qom-get reply was missing return data")); + goto cleanup; + } + + ret = 0; +cleanup: + virJSONValueFree(cmd); + virJSONValueFree(reply); + + return ret; +} + + int qemuMonitorJSONGetObjectProps(qemuMonitorPtr mon, const char *type, char ***props) diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index e7ce145..e6bbd62 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -348,6 +348,40 @@ int qemuMonitorJSONGetObjectListPaths(qemuMonitorPtr mon,

void qemuMonitorJSONListPathFree(qemuMonitorJSONListPathPtr paths);

+/* Flags for the 'type' field in _qemuMonitorJSONObjectProperty */ +typedef enum { + QEMU_MONITOR_OBJECT_PROPERTY_BOOLEAN=1, + QEMU_MONITOR_OBJECT_PROPERTY_INT, + QEMU_MONITOR_OBJECT_PROPERTY_LONG, + QEMU_MONITOR_OBJECT_PROPERTY_UINT, + QEMU_MONITOR_OBJECT_PROPERTY_ULONG, + QEMU_MONITOR_OBJECT_PROPERTY_DOUBLE, + QEMU_MONITOR_OBJECT_PROPERTY_STRING, + + QEMU_MONITOR_OBJECT_PROPERTY_LAST +} qemuMonitorJSONObjectPropertyType; + +typedef struct _qemuMonitorJSONObjectProperty qemuMonitorJSONObjectProperty; +typedef qemuMonitorJSONObjectProperty *qemuMonitorJSONObjectPropertyPtr; +struct _qemuMonitorJSONObjectProperty { + int type; /* qemuMonitorJSONObjectPropertyType */ + union { + bool b; + int i;

Huh, syntax-check fails at this ^^^ line thinking @i is a loop variable. I guess you'll need to add an exception to cfg.mk: diff --git a/cfg.mk b/cfg.mk index c6a097e..2936280 100644 --- a/cfg.mk +++ b/cfg.mk @@ -985,4 +985,4 @@ exclude_file_name_regexp--sc_prohibit_include_public_headers_brackets = \ ^(python/|tools/|examples/|include/libvirt/(virterror|libvirt-(qemu|lxc))\.h$$) exclude_file_name_regexp--sc_prohibit_int_ijk = \ - ^(src/remote_protocol-structs|src/remote/remote_protocol.x|cfg.mk|include/)$ + ^(src/remote_protocol-structs|src/remote/remote_protocol.x|cfg.mk|include/|src/qemu/qemu_monitor_json.h)$

+ long long l; + unsigned int ui; + unsigned long long ul; + double d; + char *str; + } val; +}; + +int qemuMonitorJSONGetObjectProperty(qemuMonitorPtr mon, + const char *path, + const char *property, + qemuMonitorJSONObjectPropertyPtr prop) + ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4); + int qemuMonitorJSONGetObjectProps(qemuMonitorPtr mon, const char *type, char ***props)

Michal

On 07/11/2013 08:05 AM, Michal Privoznik wrote:

On 08.07.2013 21:20, John Ferlan wrote:

...

Add a new qemuMonitorJSONGetObjectProperty() method to support invocation of the 'qom-get' JSON monitor command with a provided path, property, and expected data type return. The qemuMonitorJSONObjectProperty is similar to virTypedParameter; however, a future patch will extend it a bit to include a void pointer to balloon driver statistic data.

The provided test will execute a qom-get on "/machine/i440fx" which will return a property "realized". --- src/qemu/qemu_monitor_json.c | 86 ++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 34 ++++++++++++++++++

Does the new type need to live in the .h file, or can it be internal to the .c file? That is, I think qemuMonitorJSONGetObjectProperty can be marked static, since it is only used within the same file, and then all the types that it uses moved into the .c file as well.

...

+struct _qemuMonitorJSONObjectProperty { + int type; /* qemuMonitorJSONObjectPropertyType */ + union { + bool b; + int i;

Huh, syntax-check fails at this ^^^ line thinking @i is a loop variable. I guess you'll need to add an exception to cfg.mk:

Or rename the variable 'si'.

diff --git a/cfg.mk b/cfg.mk index c6a097e..2936280 100644 --- a/cfg.mk +++ b/cfg.mk @@ -985,4 +985,4 @@ exclude_file_name_regexp--sc_prohibit_include_public_headers_brackets = \ ^(python/|tools/|examples/|include/libvirt/(virterror|libvirt-(qemu|lxc))\.h$$)

exclude_file_name_regexp--sc_prohibit_int_ijk = \ - ^(src/remote_protocol-structs|src/remote/remote_protocol.x|cfg.mk|include/)$ + ^(src/remote_protocol-structs|src/remote/remote_protocol.x|cfg.mk|include/|src/qemu/qemu_monitor_json.h)$

Exempting entire files because of one line seems a bit overkill; maybe we can instead tweak the cfg.mk rule to have an exclude='exempt from syntax-check' line, then place a magic comment on this use (see how sc_prohibit_strtol uses that mechanism for one-line exemptions instead of entire-file exemptions). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 07/11/2013 10:05 AM, Michal Privoznik wrote:

On 08.07.2013 21:20, John Ferlan wrote:

...

At vm startup, reconnect, and attach - check for the presence of the balloon driver and save the path in the private area of the driver. This path will remain constant throughout the life of the domain and can then be used rather than attempting to find the path each time balloon driver statistics are fetched or the collection period changes. The qom object model model requires setting object properties after device startup. That is, it's not possible to pass the period along via the startup code as it won't be recognized. If a balloon driver path is found a check of the existing collection period will be made against the saved domain value in order to determine if an adjustment needs to be made to the period to start or stop collecting stats --- src/qemu/qemu_monitor.c | 130 +++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor.h | 2 + src/qemu/qemu_monitor_json.c | 42 ++++++++++++++ src/qemu/qemu_monitor_json.h | 3 + src/qemu/qemu_process.c | 21 ++++++- 5 files changed, 196 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index fe5ab0a..038c9e8 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -83,6 +83,9 @@ struct _qemuMonitor {

/* cache of query-command-line-options results */ virJSONValuePtr options; + + /* If found, path to the virtio memballoon driver */ + char *balloonpath; };

static virClassPtr qemuMonitorClass; @@ -248,6 +251,7 @@ static void qemuMonitorDispose(void *obj) virCondDestroy(&mon->notify); VIR_FREE(mon->buffer); virJSONValueFree(mon->options); + VIR_FREE(mon->balloonpath); }

@@ -929,6 +933,107 @@ qemuMonitorSetOptions(qemuMonitorPtr mon, virJSONValuePtr options) mon->options = options; }

+/* 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 in the + * returned 'type' field using the syntax "child<virtio-balloon-pci>". + * + * 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. + * + * This procedure will be call recursively until found or the qom-list is + * exhausted. + * + * Returns: + * + * 1 - Found + * 0 - Not found still looking + * -1 - Error bail out + * + * NOTE: This assumes we have already called qemuDomainObjEnterMonitor() + */ +static int +qemuMonitorFindBalloonObjectPath(qemuMonitorPtr mon, + virDomainObjPtr vm, + const char *curpath) +{ + int i,j;

size_t i,j, npaths = 0, nprops = 0;

yep...

...

+ int npaths = 0; + int nprops = 0; + int ret = 0; + char *nextpath = NULL; + qemuMonitorJSONListPathPtr *paths = NULL; + qemuMonitorJSONListPathPtr *bprops = NULL; + + /* Not supported */ + if (vm->def->memballoon && + vm->def->memballoon->model != VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO) { + VIR_DEBUG("Model must be virtio to get memballoon path"); + return -1; + } + + /* Already set and won't change */ + if (mon->balloonpath) + return 1; + + VIR_DEBUG("Searching for Balloon Object Path starting at %s", curpath); + + npaths = qemuMonitorJSONGetObjectListPaths(mon, curpath, &paths); + + for (i = 0; i < npaths && ret == 0; i++) { + + if (STREQ_NULLABLE(paths[i]->type, "link<virtio-balloon-pci>")) { + VIR_DEBUG("Path to <virtio-balloon-pci> is '%s/%s'", + curpath, paths[i]->name); + if (virAsprintf(&nextpath, "%s/%s", curpath, paths[i]->name) < 0) { + ret = -1; + goto cleanup; + } + + /* Now look at the each of the property entries to determine + * whether "guest-stats-polling-interval" exists. If not, + * then this version of qemu/kvm does not support the feature. + */ + nprops = qemuMonitorJSONGetObjectListPaths(mon, nextpath, &bprops); + for (j = 0; j < nprops; j++) { + if (STREQ(bprops[j]->name, "guest-stats-polling-interval")) { + mon->balloonpath = nextpath; + nextpath = NULL; + ret = 1; + goto cleanup;

Maybe I'd add VIR_DEBUG() here to log the path.

Funny - I did have one at one time along with a boatload more to print each return along the way. It got lost while I was stripping out the extraneous debug code.

...

+ } + } + + /* If we get here, we found the path, but not the property */ + VIR_DEBUG("Property 'guest-stats-polling-interval' not found"); + 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) { + virReportOOMError(); + ret = -1; + goto cleanup; + } + ret = qemuMonitorFindBalloonObjectPath(mon, vm, nextpath); + } + } + +cleanup: + for (i = 0; i < npaths; i++) + qemuMonitorJSONListPathFree(paths[i]); + VIR_FREE(paths); + for (j = 0; j < nprops; j++) + qemuMonitorJSONListPathFree(bprops[j]); + VIR_FREE(bprops); + VIR_FREE(nextpath); + return ret; +} + int qemuMonitorHMPCommandWithFd(qemuMonitorPtr mon, const char *cmd, int scm_fd, @@ -1390,6 +1495,31 @@ int qemuMonitorGetMemoryStats(qemuMonitorPtr mon, return ret; }

+int qemuMonitorSetMemoryStatsPeriod(qemuMonitorPtr mon, + int period) +{ + int ret = -1; + VIR_DEBUG("mon=%p period=%d", mon, period); + + if (!mon) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("monitor must not be NULL")); + return -1; + } + + if (!mon->json) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("JSON monitor is required")); + return -1; + } + + if (qemuMonitorFindBalloonObjectPath(mon, mon->vm, "/") == 1) { + ret = qemuMonitorJSONSetMemoryStatsPeriod(mon, mon->balloonpath, + period); + } + return ret; +} + int qemuMonitorBlockIOStatusToError(const char *status) { diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 78011ee..12b730a 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -265,6 +265,8 @@ int qemuMonitorGetBalloonInfo(qemuMonitorPtr mon, int qemuMonitorGetMemoryStats(qemuMonitorPtr mon, virDomainMemoryStatPtr stats, unsigned int nr_stats); +int qemuMonitorSetMemoryStatsPeriod(qemuMonitorPtr mon, + int period);

int qemuMonitorBlockIOStatusToError(const char *status); virHashTablePtr qemuMonitorGetBlockInfo(qemuMonitorPtr mon); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 9487955..2d7f9b6 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1495,6 +1495,48 @@ cleanup: }

+/* + * Using the provided balloonpath, determine if we need to set the

s/balloonpath/balloon path/

balloonpath is the variable name...

...

+ * collection interval property to enable statistics gathering. + */ +int +qemuMonitorJSONSetMemoryStatsPeriod(qemuMonitorPtr mon, + char *balloonpath, + int period) +{ + qemuMonitorJSONObjectProperty prop; + + /* Get the current value of the stats polling interval */ + memset(&prop, 0, sizeof(qemuMonitorJSONObjectProperty)); + prop.type = QEMU_MONITOR_OBJECT_PROPERTY_INT; + if (qemuMonitorJSONGetObjectProperty(mon, balloonpath, + "guest-stats-polling-interval", + &prop) < 0) { + VIR_DEBUG("Failed to get polling interval for balloon driver"); + return 0;

Shouldn't we return -1 here? I mean, if caller requires us to set an interval, we should error out if we fail. Why are we querying for the interval prior setting it anyway?

I struggled with this return as well when I first added the code - I think this was added before saving the path and before checking in the path code that we our path had a polling-interval property. So, considering the "current" design it's duplicitous to get the value and check it against the current setting before setting, so I've removed it.

...

+ } + + VIR_DEBUG("memballoon period=%d 'guest-stats-polling-interval'=%d", + period, prop.val.i); + + /* Same value - no need to set */ + if (period == prop.val.i) + return 0; + + /* Set to the value in memballoon (could enable or disable) */ + memset(&prop, 0, sizeof(qemuMonitorJSONObjectProperty)); + prop.type = QEMU_MONITOR_OBJECT_PROPERTY_INT; + prop.val.i = period; + if (qemuMonitorJSONSetObjectProperty(mon, balloonpath, + "guest-stats-polling-interval", + &prop) < 0) { + VIR_DEBUG("Failed to set polling interval for balloon driver");

This is unnecessary as long as qemuMonitorJSONSetObjectProperty reports error on retval < 0, which it does.

OK gone.

...

+ return -1; + } + return 0; +} + + int qemuMonitorJSONGetBlockInfo(qemuMonitorPtr mon, virHashTablePtr table) { diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index fd0dedd..b0068ff 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -61,6 +61,9 @@ int qemuMonitorJSONGetBalloonInfo(qemuMonitorPtr mon, int qemuMonitorJSONGetMemoryStats(qemuMonitorPtr mon, virDomainMemoryStatPtr stats, unsigned int nr_stats); +int qemuMonitorJSONSetMemoryStatsPeriod(qemuMonitorPtr mon, + char *balloonpath, + int period); int qemuMonitorJSONGetBlockInfo(qemuMonitorPtr mon, virHashTablePtr table); int qemuMonitorJSONGetBlockStatsInfo(qemuMonitorPtr mon, diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index f8c652f..fd3b7a8 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3091,6 +3091,13 @@ qemuProcessReconnect(void *opaque) if (qemuProcessRecoverJob(driver, obj, conn, &oldjob) < 0) goto error;

+ if (obj->def->memballoon) { + qemuDomainObjEnterMonitor(driver, obj); + qemuMonitorSetMemoryStatsPeriod(priv->mon, + obj->def->memballoon->period); + qemuDomainObjExitMonitor(driver, obj); + } +

Why do we want to enable this on ProcessReconnect?

Before I had a way to dynamically set the period I only had the period in the XML file, thus this was the only way to "get" the stats to show up for a running guest when I installed the new code and restarted libvirtd. Tks, John

...

/* update domain state XML with possibly updated state in virDomainObj */ if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, obj) < 0) goto error; @@ -3910,6 +3917,9 @@ int qemuProcessStart(virConnectPtr conn, goto cleanup; } qemuDomainObjEnterMonitor(driver, vm); + if (vm->def->memballoon) + qemuMonitorSetMemoryStatsPeriod(priv->mon, + vm->def->memballoon->period);

It makes sense here.

...

if (qemuMonitorSetBalloon(priv->mon, cur_balloon) < 0) { qemuDomainObjExitMonitor(driver, vm); goto cleanup; @@ -4439,11 +4449,18 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED, if (!virDomainObjIsActive(vm)) goto cleanup;

- if (running) + if (running) { virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, VIR_DOMAIN_RUNNING_UNPAUSED); - else + if (vm->def->memballoon) { + qemuDomainObjEnterMonitor(driver, vm); + qemuMonitorSetMemoryStatsPeriod(priv->mon, + vm->def->memballoon->period); + qemuDomainObjExitMonitor(driver, vm); + } + } else { virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, reason); + }

VIR_DEBUG("Writing domain status to disk"); if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0)

Michal

On 07/11/2013 10:33 AM, Daniel P. Berrange wrote:

On Mon, Jul 08, 2013 at 03:20:31PM -0400, John Ferlan wrote:

...

At vm startup, reconnect, and attach - check for the presence of the balloon driver and save the path in the private area of the driver. This path will remain constant throughout the life of the domain and can then be used rather than attempting to find the path each time balloon driver statistics are fetched or the collection period changes. The qom object model model requires setting object properties after device startup. That is, it's not possible to pass the period along via the startup code as it won't be recognized. If a balloon driver path is found a check of the existing collection period will be made against the saved domain value in order to determine if an adjustment needs to be made to the period to start or stop collecting stats --- src/qemu/qemu_monitor.c | 130 +++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor.h | 2 + src/qemu/qemu_monitor_json.c | 42 ++++++++++++++ src/qemu/qemu_monitor_json.h | 3 + src/qemu/qemu_process.c | 21 ++++++- 5 files changed, 196 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index fe5ab0a..038c9e8 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -83,6 +83,9 @@ struct _qemuMonitor {

/* cache of query-command-line-options results */ virJSONValuePtr options; + + /* If found, path to the virtio memballoon driver */ + char *balloonpath;

Hmm, we can't distinguish between "not searched yet" and "searched, but didn't find one". This means in the latter case we'll repeatedly search every time. Not too bad, but a little wasteful. Perhaps add a 'bool ballooninit' to track whether we've searched yet.

Oh yeah - added that, thanks.

...

+ * Returns: + * + * 1 - Found + * 0 - Not found still looking + * -1 - Error bail out + * + * NOTE: This assumes we have already called qemuDomainObjEnterMonitor() + */ +static int +qemuMonitorFindBalloonObjectPath(qemuMonitorPtr mon, + virDomainObjPtr vm, + const char *curpath) +{ + int i,j;

size_t now

Yep

...

+ int npaths = 0; + int nprops = 0; + int ret = 0; + char *nextpath = NULL; + qemuMonitorJSONListPathPtr *paths = NULL; + qemuMonitorJSONListPathPtr *bprops = NULL; + + /* Not supported */ + if (vm->def->memballoon && + vm->def->memballoon->model != VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO) { + VIR_DEBUG("Model must be virtio to get memballoon path"); + return -1; + }

What about if 'vm->def->memballoon == NULL' ? Shouldn't we return -1 in that case too ? eg this condition should actually be

if (vm->def->memballoon == NULL || vm->def->memballoon->model != VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO) {

Ah - right! Must've been sleeping when I used the && and not a if (!vm->def->memballoon...) John

On Mon, Jul 08, 2013 at 03:20:32PM -0400, John Ferlan wrote:

This patch will add the QEMU_MONITOR_OBJECT_PROPERTY_BALLOON_STATS type and a mechanism in the qemuMonitorObjectProperty to fetch and store an opaque data array assuming that we are provided a count of current elements, a count of maximum elements, and the address of the array store the data. Use the mechanism to fetch balloon driver statistics. --- src/qemu/qemu_monitor.c | 3 +- src/qemu/qemu_monitor_json.c | 206 ++++++++++++++++++++----------------------- src/qemu/qemu_monitor_json.h | 5 ++ 3 files changed, 103 insertions(+), 111 deletions(-)

@@ -4747,6 +4729,9 @@ int qemuMonitorJSONGetObjectProperty(qemuMonitorPtr mon, if (tmp) ret = 0; break; + case QEMU_MONITOR_OBJECT_PROPERTY_BALLOON_STATS: + ret = qemuMonitorJSONGetBalloonStats(data, prop); + break; case QEMU_MONITOR_OBJECT_PROPERTY_LAST: default: virReportError(VIR_ERR_INTERNAL_ERROR, @@ -4810,6 +4795,7 @@ int qemuMonitorJSONSetObjectProperty(qemuMonitorPtr mon, case QEMU_MONITOR_OBJECT_PROPERTY_STRING: MAKE_SET_CMD("s:value", prop->val.str); break; + case QEMU_MONITOR_OBJECT_PROPERTY_BALLOON_STATS: case QEMU_MONITOR_OBJECT_PROPERTY_LAST: default: virReportError(VIR_ERR_INTERNAL_ERROR, @@ -4830,7 +4816,7 @@ cleanup:

return ret; } -

[snip]

@@ -360,6 +361,7 @@ typedef enum { QEMU_MONITOR_OBJECT_PROPERTY_ULONG, QEMU_MONITOR_OBJECT_PROPERTY_DOUBLE, QEMU_MONITOR_OBJECT_PROPERTY_STRING, + QEMU_MONITOR_OBJECT_PROPERTY_BALLOON_STATS,

QEMU_MONITOR_OBJECT_PROPERTY_LAST } qemuMonitorJSONObjectPropertyType; @@ -368,6 +370,8 @@ typedef struct _qemuMonitorJSONObjectProperty qemuMonitorJSONObjectProperty; typedef qemuMonitorJSONObjectProperty *qemuMonitorJSONObjectPropertyPtr; struct _qemuMonitorJSONObjectProperty { int type; /* qemuMonitorJSONObjectPropertyType */ + int curelems; /* Current number elements in **ptr array */ + int maxelems; /* Maximum number elements allowed in any **ptr array */ union { bool b; int i; @@ -376,6 +380,7 @@ struct _qemuMonitorJSONObjectProperty { unsigned long long ul; double d; char *str; + void **ptr; } val;

I still don't really like that you're taking a function which is generic (qemuMonitorJSONGetObjectProperty) and adding in code which is specific to a single monitor command, and storing command specific data structures in a 'void **'. IMHO qemuMonitorJSONGetObjectProperty should only ever return generic data structures, and the conversion to the memory stats struct should be done by the caller. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On Mon, Jul 08, 2013 at 03:20:33PM -0400, John Ferlan wrote:

Add new API in order to set the balloon memory driver statistics collection period in order to allow dynamic period adjustment for the virsh dommemstats to display balloon stats data --- include/libvirt/libvirt.h.in | 3 ++ src/driver.h | 6 ++++ src/libvirt.c | 65 ++++++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 5 ++++ 4 files changed, 79 insertions(+)

diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index b87255a..069cac8 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -1909,6 +1909,9 @@ int virDomainSetMemory (virDomainPtr domain, int virDomainSetMemoryFlags (virDomainPtr domain, unsigned long memory, unsigned int flags); +int virDomainSetMemoryStatsPeriodFlags (virDomainPtr domain, + int period, + unsigned int flags);

s/Flags// in the name - we only do that if we're replacing an existing API.

diff --git a/src/driver.h b/src/driver.h index 31851cb..a8964e9 100644 --- a/src/driver.h +++ b/src/driver.h @@ -207,6 +207,11 @@ typedef int unsigned int flags);

typedef int +(*virDrvDomainSetMemoryStatsPeriodFlags)(virDomainPtr domain, + int period, + unsigned int flags);

s/Flags//

@@ -1158,6 +1163,7 @@ struct _virDriver { virDrvDomainSetMaxMemory domainSetMaxMemory; virDrvDomainSetMemory domainSetMemory; virDrvDomainSetMemoryFlags domainSetMemoryFlags; + virDrvDomainSetMemoryStatsPeriodFlags domainSetMemoryStatsPeriodFlags;

s/Flags//

diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 7c6edf6..d4e4834 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -627,4 +627,9 @@ LIBVIRT_1.1.0 { virDomainMigrateToURI3; } LIBVIRT_1.0.6;

+LIBVIRT_1.1.1 { + global: + virDomainSetMemoryStatsPeriodFlags;

s/Flags// Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On Mon, Jul 08, 2013 at 03:20:34PM -0400, John Ferlan wrote:

Wire up the remote protocol --- src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 15 ++++++++++++++- src/remote_protocol-structs | 6 ++++++ 3 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 7f3e833..762e4b3 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -6610,6 +6610,7 @@ static virDriver remote_driver = { .domainSetMaxMemory = remoteDomainSetMaxMemory, /* 0.3.0 */ .domainSetMemory = remoteDomainSetMemory, /* 0.3.0 */ .domainSetMemoryFlags = remoteDomainSetMemoryFlags, /* 0.9.0 */ + .domainSetMemoryStatsPeriodFlags = remoteDomainSetMemoryStatsPeriodFlags, /* 1.1.1 */ .domainSetMemoryParameters = remoteDomainSetMemoryParameters, /* 0.8.5 */ .domainGetMemoryParameters = remoteDomainGetMemoryParameters, /* 0.8.5 */ .domainSetBlkioParameters = remoteDomainSetBlkioParameters, /* 0.9.0 */ diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 2e9dc1d..dcdc0f3 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -833,6 +833,12 @@ struct remote_domain_set_memory_flags_args { unsigned int flags; };

+struct remote_domain_set_memory_stats_period_flags_args {

s/flags// and in other places below. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|