[libvirt] [PATCH 0/8] Re-enable memballoon driver statistics reporting
https://bugzilla.redhat.com/show_bug.cgi?id=904160 The following patches will provide the support and functionality in order to re-enable getting domain memory statistics from the balloon driver based on the upstream QEMU patch: https://lists.gnu.org/archive/html/qemu-devel/2013-01/msg04835.html Statistics gathering requires usage of the QEMU QObject model, specifically the qom-list command to find the path to the balloon driver and the qom-get/ qom-set commands in order to get/set properties on the object. A future possible extension would be to allow 'virsh qemuobject [--list|--get|--put]' where the --list would list a single path and the get/set would allow viewing/adjusting a specific object path property. The QObject model requires setting properties after the domain (and device) has been started. For the balloon driver, in order to enable collection the property "guest-stats-polling-interval" is used. Since the QObject model has been in place for a while and the balloon driver has been part of that, the capability to gather statistics is only discernible if the property is found. Once found, the property can be changed at any time after startup in order to exend/shorted the collection interval or disable the collection by setting the property back to 0 (zero). Setting the property back does not reset already generated statistics. If statistics haven't been generated at all they are initialized to -1. Thus if values are set at -1, they will not be stored in the output. The only possible visual cue to determine that statistics are disabled is if the polling interval property was 0 (zero). Since it wasn't clear whether adjusting the dommemstats output to provide that visual cue, I left that as a future possible exercise - although it is possible to add it to this patch set where the output would be extended to indicate the current collection interval period or that collection is currently disabled, eg one new row "period #". The collection period interval is saved in a new memballoon stats field as 'period'. This is saved in the xml as "<stats period='10'/>". The capability to dynamically set the period once the domain has started is controlled by a new '--period <value>' option to the dommemstats command. The command was extended to support the --live, --current, and --config options. The implementation uses the existing virDomainSetMemoryFlags with a new 'VIR_DOMAIN_MEM_PERIOD' flag that the qemuDomainSetMemoryFlags() will handle. This was methodology was chosen in preference to generating (a) new driver function(s) to just handle the get/set of the collection period. John Ferlan (8): Add qemuMonitorGetObjectListPaths() method for QMP qom-list command Add qemuMonitorGetObjectProperty() method for QMP qom-get command Add qemuMonitorSetObjectProperty() method for QMP qom-set command Add 'period' for Memballoon statistics gathering capability Determine whether to start balloon memory stats gathering. Add capability to fetch balloon stats If available fetch the balloon driver memory stats Allow balloon driver collection to be adjusted dynamically docs/formatdomain.html.in | 14 ++ docs/schemas/domaincommon.rng | 7 + include/libvirt/libvirt.h.in | 1 + src/conf/domain_conf.c | 27 +++- src/conf/domain_conf.h | 1 + src/libvirt.c | 8 +- src/qemu/qemu_domain.c | 1 + src/qemu/qemu_domain.h | 2 + src/qemu/qemu_driver.c | 61 +++++++- src/qemu/qemu_monitor.c | 79 +++++++++++ src/qemu/qemu_monitor.h | 57 ++++++++ src/qemu/qemu_monitor_json.c | 317 ++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 18 +++ src/qemu/qemu_process.c | 164 +++++++++++++++++++++- tests/qemumonitorjsontest.c | 184 ++++++++++++++++++++++++ tools/virsh-domain-monitor.c | 70 +++++++++- 16 files changed, 1000 insertions(+), 11 deletions(-) -- 1.8.1.4
Add a new qemuMonitorGetObjectListPaths() 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.c | 33 ++++++++++++++++ src/qemu/qemu_monitor.h | 15 +++++++ src/qemu/qemu_monitor_json.c | 93 ++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 6 +++ tests/qemumonitorjsontest.c | 77 ++++++++++++++++++++++++++++++++++++ 5 files changed, 224 insertions(+) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 091e239..f175250 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3435,6 +3435,39 @@ int qemuMonitorGetObjectTypes(qemuMonitorPtr mon, } +int qemuMonitorGetObjectListPaths(qemuMonitorPtr mon, + const char *path, + qemuMonitorListPathPtr **paths) +{ + VIR_DEBUG("mon=%p paths=%p", + mon, paths); + + 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; + } + + return qemuMonitorJSONGetObjectListPaths(mon, path, paths); +} + + +void qemuMonitorListPathFree(qemuMonitorListPathPtr paths) +{ + if (!paths) + return; + VIR_FREE(paths->name); + VIR_FREE(paths->type); + VIR_FREE(paths); +} + + int qemuMonitorGetObjectProps(qemuMonitorPtr mon, const char *type, char ***props) diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 86ef635..2e92f8c 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -679,6 +679,21 @@ int qemuMonitorGetKVMState(qemuMonitorPtr mon, int qemuMonitorGetObjectTypes(qemuMonitorPtr mon, char ***types); + +typedef struct _qemuMonitorListPath qemuMonitorListPath; +typedef qemuMonitorListPath *qemuMonitorListPathPtr; + +struct _qemuMonitorListPath { + char *name; + char *type; +}; + +int qemuMonitorGetObjectListPaths(qemuMonitorPtr mon, + const char *path, + qemuMonitorListPathPtr **paths); + +void qemuMonitorListPathFree(qemuMonitorListPathPtr paths); + int qemuMonitorGetObjectProps(qemuMonitorPtr mon, const char *type, char ***props); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index c0d7960..7d0cc85 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4533,6 +4533,99 @@ cleanup: } +int qemuMonitorJSONGetObjectListPaths(qemuMonitorPtr mon, + const char *path, + qemuMonitorListPathPtr **paths) +{ + int ret; + virJSONValuePtr cmd; + virJSONValuePtr reply = NULL; + virJSONValuePtr data; + qemuMonitorListPathPtr *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 */ + if (VIR_ALLOC_N(pathlist, n + 1) < 0) { + virReportOOMError(); + goto cleanup; + } + + for (i = 0; i < n; i++) { + virJSONValuePtr child = virJSONValueArrayGet(data, i); + const char *tmp; + qemuMonitorListPathPtr 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; + *paths = pathlist; + +cleanup: + if (ret < 0 && pathlist) { + for (i = 0; i < n; i++) + qemuMonitorListPathFree(pathlist[i]); + VIR_FREE(pathlist); + } + 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 d79b86b..20a2364 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -332,6 +332,12 @@ int qemuMonitorJSONGetKVMState(qemuMonitorPtr mon, int qemuMonitorJSONGetObjectTypes(qemuMonitorPtr mon, char ***types) ATTRIBUTE_NONNULL(2); + +int qemuMonitorJSONGetObjectListPaths(qemuMonitorPtr mon, + const char *path, + qemuMonitorListPathPtr **paths) + ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); + int qemuMonitorJSONGetObjectProps(qemuMonitorPtr mon, const char *type, char ***props) diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index acc94ca..ab8a73d 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -595,6 +595,82 @@ 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; + qemuMonitorListPathPtr *paths; + int npaths = 0; + int 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 = qemuMonitorGetObjectListPaths(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++) + qemuMonitorListPathFree(paths[i]); + VIR_FREE(paths); + return ret; +} + + static int mymain(void) { @@ -623,6 +699,7 @@ mymain(void) DO_TEST(GetCommands); DO_TEST(GetTPMModels); DO_TEST(GetCommandLineOptionParameters); + DO_TEST(GetListPaths); virObjectUnref(xmlopt); -- 1.8.1.4
On Tue, Jul 02, 2013 at 09:39:19AM -0400, John Ferlan wrote:
Add a new qemuMonitorGetObjectListPaths() 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.
Top marks for adding a test !
--- src/qemu/qemu_monitor.c | 33 ++++++++++++++++ src/qemu/qemu_monitor.h | 15 +++++++ src/qemu/qemu_monitor_json.c | 93 ++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 6 +++ tests/qemumonitorjsontest.c | 77 ++++++++++++++++++++++++++++++++++++ 5 files changed, 224 insertions(+)
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 86ef635..2e92f8c 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -679,6 +679,21 @@ int qemuMonitorGetKVMState(qemuMonitorPtr mon,
int qemuMonitorGetObjectTypes(qemuMonitorPtr mon, char ***types); + +typedef struct _qemuMonitorListPath qemuMonitorListPath; +typedef qemuMonitorListPath *qemuMonitorListPathPtr; + +struct _qemuMonitorListPath { + char *name; + char *type; +}; + +int qemuMonitorGetObjectListPaths(qemuMonitorPtr mon, + const char *path, + qemuMonitorListPathPtr **paths); + +void qemuMonitorListPathFree(qemuMonitorListPathPtr paths);
The qom related monitor calls are super generic, which also means they are a super PITA to deal with in code. To mitigate this, I think we should try to keep code that uses the qom commands isolated in the qemu_monitor* files, and then expose higher level APIs to the rest of the QEMU driver code. So I think it is ok to expose these APIs in qemu_monitor_json.h, but we should not expose it in qemu_monitor.h 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 07/03/2013 06:56 AM, Daniel P. Berrange wrote:
On Tue, Jul 02, 2013 at 09:39:19AM -0400, John Ferlan wrote:
Add a new qemuMonitorGetObjectListPaths() 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.
Top marks for adding a test !
--- src/qemu/qemu_monitor.c | 33 ++++++++++++++++ src/qemu/qemu_monitor.h | 15 +++++++ src/qemu/qemu_monitor_json.c | 93 ++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 6 +++ tests/qemumonitorjsontest.c | 77 ++++++++++++++++++++++++++++++++++++ 5 files changed, 224 insertions(+)
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 86ef635..2e92f8c 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -679,6 +679,21 @@ int qemuMonitorGetKVMState(qemuMonitorPtr mon,
int qemuMonitorGetObjectTypes(qemuMonitorPtr mon, char ***types); + +typedef struct _qemuMonitorListPath qemuMonitorListPath; +typedef qemuMonitorListPath *qemuMonitorListPathPtr; + +struct _qemuMonitorListPath { + char *name; + char *type; +}; + +int qemuMonitorGetObjectListPaths(qemuMonitorPtr mon, + const char *path, + qemuMonitorListPathPtr **paths); + +void qemuMonitorListPathFree(qemuMonitorListPathPtr paths);
The qom related monitor calls are super generic, which also means they are a super PITA to deal with in code. To mitigate this, I think we should try to keep code that uses the qom commands isolated in the qemu_monitor* files, and then expose higher level APIs to the rest of the QEMU driver code.
So I think it is ok to expose these APIs in qemu_monitor_json.h, but we should not expose it in qemu_monitor.h
Daniel
The model I followed was the same model other entry points used through qemu_monitor and into qemu_monitor_json (and for some into qemu_monitor_text). If I read this correctly, for the first 3 patches it seems you are advocating removing the middle man - that is later rather than calling qemuMonitorGetObjectListPaths call qemuMonitorJSONGetObjectListPaths directly. Similarly for the GetObject & PutObject calls. I cannot think of an issue with the change, but I'm just making sure to avoid continual rework. That also seems to mean modifying the test to include 'qemu_monitor_json.h'... John
On Wed, Jul 03, 2013 at 11:30:46AM -0400, John Ferlan wrote:
On 07/03/2013 06:56 AM, Daniel P. Berrange wrote:
On Tue, Jul 02, 2013 at 09:39:19AM -0400, John Ferlan wrote:
Add a new qemuMonitorGetObjectListPaths() 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.
Top marks for adding a test !
--- src/qemu/qemu_monitor.c | 33 ++++++++++++++++ src/qemu/qemu_monitor.h | 15 +++++++ src/qemu/qemu_monitor_json.c | 93 ++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 6 +++ tests/qemumonitorjsontest.c | 77 ++++++++++++++++++++++++++++++++++++ 5 files changed, 224 insertions(+)
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 86ef635..2e92f8c 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -679,6 +679,21 @@ int qemuMonitorGetKVMState(qemuMonitorPtr mon,
int qemuMonitorGetObjectTypes(qemuMonitorPtr mon, char ***types); + +typedef struct _qemuMonitorListPath qemuMonitorListPath; +typedef qemuMonitorListPath *qemuMonitorListPathPtr; + +struct _qemuMonitorListPath { + char *name; + char *type; +}; + +int qemuMonitorGetObjectListPaths(qemuMonitorPtr mon, + const char *path, + qemuMonitorListPathPtr **paths); + +void qemuMonitorListPathFree(qemuMonitorListPathPtr paths);
The qom related monitor calls are super generic, which also means they are a super PITA to deal with in code. To mitigate this, I think we should try to keep code that uses the qom commands isolated in the qemu_monitor* files, and then expose higher level APIs to the rest of the QEMU driver code.
So I think it is ok to expose these APIs in qemu_monitor_json.h, but we should not expose it in qemu_monitor.h
Daniel
The model I followed was the same model other entry points used through qemu_monitor and into qemu_monitor_json (and for some into qemu_monitor_text).
If I read this correctly, for the first 3 patches it seems you are advocating removing the middle man - that is later rather than calling qemuMonitorGetObjectListPaths call qemuMonitorJSONGetObjectListPaths directly. Similarly for the GetObject & PutObject calls.
No, I'm not recommending that qemu_driver/qemu_process call into qemu_monitor_json.h I'm saying that instead of directly exposing the ugly interface of the QOM commands to callers of qemu_monitor.h, we should have a higher level API exposed. eg, qemu_monitor.h should just expose qemuMonitorGetMemoryStats() (already exists in fact) qemuMonitorSetMemoryStatsRefresPeriod() and then the qemu_monitor.c impls of these methods can call qemuMonitorJSON{GetObjectListPaths,GetObject,PutObject}. No other code outside of the qemu_monitor.c file then has to know about qemuMonitorJSON{GetObjectListPaths,GetObject,PutObject} 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 :|
Add a new qemuMonitorGetObjectProperty() method to support invocation of the 'qom-get' JSON monitor command with a provided path, property, and expected data type return. The qemuMonitorObjectProperty 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.c | 23 ++++++++++++ src/qemu/qemu_monitor.h | 33 +++++++++++++++++ src/qemu/qemu_monitor_json.c | 86 ++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 6 ++++ tests/qemumonitorjsontest.c | 48 +++++++++++++++++++++++++ 5 files changed, 196 insertions(+) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index f175250..6737a63 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3468,6 +3468,29 @@ void qemuMonitorListPathFree(qemuMonitorListPathPtr paths) } +int qemuMonitorGetObjectProperty(qemuMonitorPtr mon, + const char *path, + const char *property, + qemuMonitorObjectPropertyPtr prop) +{ + VIR_DEBUG("mon=%p path=%s property=%s prop=%p type=%d", + mon, path, property, prop, prop->type); + 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; + } + + return qemuMonitorJSONGetObjectProperty(mon, path, property, prop); +} + + int qemuMonitorGetObjectProps(qemuMonitorPtr mon, const char *type, char ***props) diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 2e92f8c..cc22123 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -694,6 +694,39 @@ int qemuMonitorGetObjectListPaths(qemuMonitorPtr mon, void qemuMonitorListPathFree(qemuMonitorListPathPtr paths); +/* Flags for the 'type' field in _qemuMonitorObjectProperty */ +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 +} qemuMonitorObjectPropertyType; + +typedef struct _qemuMonitorObjectProperty qemuMonitorObjectProperty; +typedef qemuMonitorObjectProperty *qemuMonitorObjectPropertyPtr; +struct _qemuMonitorObjectProperty { + int type; /* qemuMonitorObjectPropertyType */ + union { + bool b; + int i; + long long l; + unsigned int ui; + unsigned long long ul; + double d; + char *str; + } val; +}; + +int qemuMonitorGetObjectProperty(qemuMonitorPtr mon, + const char *path, + const char *property, + qemuMonitorObjectPropertyPtr prop); + int qemuMonitorGetObjectProps(qemuMonitorPtr mon, const char *type, char ***props); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 7d0cc85..a725903 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4626,6 +4626,92 @@ cleanup: return ret; } + +int qemuMonitorJSONGetObjectProperty(qemuMonitorPtr mon, + const char *path, + const char *property, + qemuMonitorObjectPropertyPtr 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) { + /* 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: + 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 20a2364..63807df 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -338,6 +338,12 @@ int qemuMonitorJSONGetObjectListPaths(qemuMonitorPtr mon, qemuMonitorListPathPtr **paths) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); +int qemuMonitorJSONGetObjectProperty(qemuMonitorPtr mon, + const char *path, + const char *property, + qemuMonitorObjectPropertyPtr prop) + ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4); + int qemuMonitorJSONGetObjectProps(qemuMonitorPtr mon, const char *type, char ***props) diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index ab8a73d..4544676 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -671,6 +671,53 @@ cleanup: } +/* + * This test will use a path to /machine/i440fx which should exist in order + * to ensure that the qom-get property fetch works properly. The following + * is the execution and expected return: + * + * + * { "execute": "qom-get","arguments": \ + * { "path": "/machine/i440fx","property": "realized"}} + * {"return": true} + */ +static int +testQemuMonitorJSONGetObjectProperty(const void *data) +{ + const virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data; + qemuMonitorTestPtr test = qemuMonitorTestNew(true, xmlopt); + int ret = -1; + qemuMonitorObjectProperty prop; + + if (!test) + return -1; + + if (qemuMonitorTestAddItem(test, "qom-get", + "{ \"return\": true }") < 0) + goto cleanup; + + /* Present with path and property */ + memset(&prop, 0, sizeof(qemuMonitorObjectProperty)); + prop.type = QEMU_MONITOR_OBJECT_PROPERTY_BOOLEAN; + if (qemuMonitorGetObjectProperty(qemuMonitorTestGetMonitor(test), + "/machine/i440fx", + "realized", + &prop) < 0) + goto cleanup; + + if (!prop.val.b) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + "expected true, but false returned"); + goto cleanup; + } + + ret = 0; +cleanup: + qemuMonitorTestFree(test); + return ret; +} + + static int mymain(void) { @@ -700,6 +747,7 @@ mymain(void) DO_TEST(GetTPMModels); DO_TEST(GetCommandLineOptionParameters); DO_TEST(GetListPaths); + DO_TEST(GetObjectProperty); virObjectUnref(xmlopt); -- 1.8.1.4
On Tue, Jul 02, 2013 at 09:39:20AM -0400, John Ferlan wrote:
Add a new qemuMonitorGetObjectProperty() method to support invocation of the 'qom-get' JSON monitor command with a provided path, property, and expected data type return. The qemuMonitorObjectProperty 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.c | 23 ++++++++++++ src/qemu/qemu_monitor.h | 33 +++++++++++++++++ src/qemu/qemu_monitor_json.c | 86 ++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 6 ++++ tests/qemumonitorjsontest.c | 48 +++++++++++++++++++++++++ 5 files changed, 196 insertions(+)
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 2e92f8c..cc22123 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -694,6 +694,39 @@ int qemuMonitorGetObjectListPaths(qemuMonitorPtr mon,
void qemuMonitorListPathFree(qemuMonitorListPathPtr paths);
+/* Flags for the 'type' field in _qemuMonitorObjectProperty */ +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 +} qemuMonitorObjectPropertyType; + +typedef struct _qemuMonitorObjectProperty qemuMonitorObjectProperty; +typedef qemuMonitorObjectProperty *qemuMonitorObjectPropertyPtr; +struct _qemuMonitorObjectProperty { + int type; /* qemuMonitorObjectPropertyType */ + union { + bool b; + int i; + long long l; + unsigned int ui; + unsigned long long ul; + double d; + char *str; + } val; +};
I think the struct/enum should be kept in qemu_monitor_json.h
+ +int qemuMonitorGetObjectProperty(qemuMonitorPtr mon, + const char *path, + const char *property, + qemuMonitorObjectPropertyPtr prop);
and not expose this method in qemu_monitor.h 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 02.07.2013 15:39, John Ferlan wrote:
Add a new qemuMonitorGetObjectProperty() method to support invocation of the 'qom-get' JSON monitor command with a provided path, property, and expected data type return. The qemuMonitorObjectProperty 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.c | 23 ++++++++++++ src/qemu/qemu_monitor.h | 33 +++++++++++++++++ src/qemu/qemu_monitor_json.c | 86 ++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 6 ++++ tests/qemumonitorjsontest.c | 48 +++++++++++++++++++++++++ 5 files changed, 196 insertions(+)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index f175250..6737a63 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3468,6 +3468,29 @@ void qemuMonitorListPathFree(qemuMonitorListPathPtr paths) }
+int qemuMonitorGetObjectProperty(qemuMonitorPtr mon, + const char *path, + const char *property, + qemuMonitorObjectPropertyPtr prop) +{ + VIR_DEBUG("mon=%p path=%s property=%s prop=%p type=%d", + mon, path, property, prop, prop->type); + 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; + } + + return qemuMonitorJSONGetObjectProperty(mon, path, property, prop); +} + + int qemuMonitorGetObjectProps(qemuMonitorPtr mon, const char *type, char ***props) diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 2e92f8c..cc22123 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -694,6 +694,39 @@ int qemuMonitorGetObjectListPaths(qemuMonitorPtr mon,
void qemuMonitorListPathFree(qemuMonitorListPathPtr paths);
+/* Flags for the 'type' field in _qemuMonitorObjectProperty */ +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 +} qemuMonitorObjectPropertyType; + +typedef struct _qemuMonitorObjectProperty qemuMonitorObjectProperty; +typedef qemuMonitorObjectProperty *qemuMonitorObjectPropertyPtr; +struct _qemuMonitorObjectProperty { + int type; /* qemuMonitorObjectPropertyType */ + union { + bool b; + int i; + long long l; + unsigned int ui; + unsigned long long ul; + double d; + char *str; + } val; +}; + +int qemuMonitorGetObjectProperty(qemuMonitorPtr mon, + const char *path, + const char *property, + qemuMonitorObjectPropertyPtr prop); + int qemuMonitorGetObjectProps(qemuMonitorPtr mon, const char *type, char ***props); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 7d0cc85..a725903 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4626,6 +4626,92 @@ cleanup: return ret; }
+ +int qemuMonitorJSONGetObjectProperty(qemuMonitorPtr mon, + const char *path, + const char *property, + qemuMonitorObjectPropertyPtr 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) { + /* 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)
Lose the 'tmp' here as VIR_STRDUP will check it for NULL anyway. In fact, these lines can be rewritten into a single line: ret = VIR_STRDUP(prop->val.str, tmp);
+ goto cleanup; + if (tmp) + ret = 0; + break; + case QEMU_MONITOR_OBJECT_PROPERTY_LAST: + default: + 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; +} + +
Michal
On 07/03/2013 02:03 PM, Michal Privoznik wrote:
On 02.07.2013 15:39, John Ferlan wrote:
+ case QEMU_MONITOR_OBJECT_PROPERTY_STRING: + tmp = virJSONValueGetString(data); + if (tmp && VIR_STRDUP(prop->val.str, tmp) < 0)
Lose the 'tmp' here as VIR_STRDUP will check it for NULL anyway. In fact, these lines can be rewritten into a single line:
ret = VIR_STRDUP(prop->val.str, tmp);
+ goto cleanup; + if (tmp) + ret = 0; + break;
Not at all. Before it returns 0 if GetString returned something. After your change, ret will be 1 in that case, and 0 if GetString returned nothing. Jan
On 03.07.2013 14:12, Ján Tomko wrote:
On 07/03/2013 02:03 PM, Michal Privoznik wrote:
On 02.07.2013 15:39, John Ferlan wrote:
+ case QEMU_MONITOR_OBJECT_PROPERTY_STRING: + tmp = virJSONValueGetString(data); + if (tmp && VIR_STRDUP(prop->val.str, tmp) < 0)
Lose the 'tmp' here as VIR_STRDUP will check it for NULL anyway. In fact, these lines can be rewritten into a single line:
ret = VIR_STRDUP(prop->val.str, tmp);
+ goto cleanup; + if (tmp) + ret = 0; + break;
Not at all. Before it returns 0 if GetString returned something. After your change, ret will be 1 in that case, and 0 if GetString returned nothing.
Jan
But if you take a look a few lines below, we will rewrite ret = 0 anyway. The only exception is if VIR_STRDUP fails. Which I should have checked, right. Michal
On 07/03/2013 08:20 AM, Michal Privoznik wrote:
On 03.07.2013 14:12, Ján Tomko wrote:
On 07/03/2013 02:03 PM, Michal Privoznik wrote:
On 02.07.2013 15:39, John Ferlan wrote:
+ case QEMU_MONITOR_OBJECT_PROPERTY_STRING: + tmp = virJSONValueGetString(data); + if (tmp && VIR_STRDUP(prop->val.str, tmp) < 0)
Lose the 'tmp' here as VIR_STRDUP will check it for NULL anyway. In fact, these lines can be rewritten into a single line:
ret = VIR_STRDUP(prop->val.str, tmp);
+ goto cleanup; + if (tmp) + ret = 0; + break;
Not at all. Before it returns 0 if GetString returned something. After your change, ret will be 1 in that case, and 0 if GetString returned nothing.
Jan
But if you take a look a few lines below, we will rewrite ret = 0 anyway. The only exception is if VIR_STRDUP fails. Which I should have checked, right.
Michal
I didn't want VIR_STRDUP() to be called if 'data' wasn't a string. That is - I don't want an empty "" returned. That allows the error to be reported that there's invalid data. Although it seems the qemuMonitorJSONHumanCommandWithFd() does an extraneous check for VIR_STRDUP(): data = virJSONValueGetString(obj); if (VIR_STRDUP(*reply_str, data ? data : "") < 0) goto cleanup; Not part of this, but I was checking callers for virJSONValueGetString() John
Add a new qemuMonitorSetObjectProperty() method to support invocation of the 'qom-set' JSON monitor command with a provided path, property, and expected data type to set. The test code uses the same "/machine/i440fx" property as the get test and attempts to set the "realized" property to "true" (which it should be set at anyway). --- src/qemu/qemu_monitor.c | 23 ++++++++++++++++ src/qemu/qemu_monitor.h | 5 ++++ src/qemu/qemu_monitor_json.c | 62 ++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 6 +++++ tests/qemumonitorjsontest.c | 59 +++++++++++++++++++++++++++++++++++++++++ 5 files changed, 155 insertions(+) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 6737a63..c46b1e5 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3491,6 +3491,29 @@ int qemuMonitorGetObjectProperty(qemuMonitorPtr mon, } +int qemuMonitorSetObjectProperty(qemuMonitorPtr mon, + const char *path, + const char *property, + qemuMonitorObjectPropertyPtr prop) +{ + VIR_DEBUG("mon=%p path=%s property=%s prop=%p type=%d", + mon, path, property, prop, prop->type); + 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; + } + + return qemuMonitorJSONSetObjectProperty(mon, path, property, prop); +} + + int qemuMonitorGetObjectProps(qemuMonitorPtr mon, const char *type, char ***props) diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index cc22123..b822b97 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -727,6 +727,11 @@ int qemuMonitorGetObjectProperty(qemuMonitorPtr mon, const char *property, qemuMonitorObjectPropertyPtr prop); +int qemuMonitorSetObjectProperty(qemuMonitorPtr mon, + const char *path, + const char *property, + qemuMonitorObjectPropertyPtr prop); + int qemuMonitorGetObjectProps(qemuMonitorPtr mon, const char *type, char ***props); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index a725903..c599626 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4712,6 +4712,68 @@ cleanup: } +#define MAKE_SET_CMD(STRING, VALUE) \ + cmd = qemuMonitorJSONMakeCommand("qom-set", \ + "s:path", path, \ + "s:property", property, \ + STRING, VALUE, \ + NULL) +int qemuMonitorJSONSetObjectProperty(qemuMonitorPtr mon, + const char *path, + const char *property, + qemuMonitorObjectPropertyPtr prop) +{ + int ret = -1; + virJSONValuePtr cmd = NULL; + virJSONValuePtr reply = NULL; + + switch (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: + MAKE_SET_CMD("b:value", prop->val.b); + break; + case QEMU_MONITOR_OBJECT_PROPERTY_INT: + MAKE_SET_CMD("i:value", prop->val.i); + break; + case QEMU_MONITOR_OBJECT_PROPERTY_LONG: + MAKE_SET_CMD("I:value", prop->val.l); + break; + case QEMU_MONITOR_OBJECT_PROPERTY_UINT: + MAKE_SET_CMD("u:value", prop->val.ui); + break; + case QEMU_MONITOR_OBJECT_PROPERTY_ULONG: + MAKE_SET_CMD("U:value", prop->val.ul); + break; + case QEMU_MONITOR_OBJECT_PROPERTY_DOUBLE: + MAKE_SET_CMD("d:value", prop->val.d); + break; + case QEMU_MONITOR_OBJECT_PROPERTY_STRING: + MAKE_SET_CMD("s:value", prop->val.str); + break; + case QEMU_MONITOR_OBJECT_PROPERTY_LAST: + default: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("qom-set invalid object property type %d"), + prop->type); + goto cleanup; + + } + if (!cmd) + return -1; + + if ((ret = qemuMonitorJSONCommand(mon, cmd, &reply)) == 0) + ret = qemuMonitorJSONCheckError(cmd, reply); + +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 63807df..1da4c44 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -344,6 +344,12 @@ int qemuMonitorJSONGetObjectProperty(qemuMonitorPtr mon, qemuMonitorObjectPropertyPtr prop) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4); +int qemuMonitorJSONSetObjectProperty(qemuMonitorPtr mon, + const char *path, + const char *property, + qemuMonitorObjectPropertyPtr prop) + ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4); + int qemuMonitorJSONGetObjectProps(qemuMonitorPtr mon, const char *type, char ***props) diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 4544676..c0987c6 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -718,6 +718,64 @@ cleanup: } +/* + * This test will use a path to /machine/i440fx which should exist in order + * to ensure that the qom-set property set works properly. The test will + * set a true property to true just as a proof of concept. Setting it to + * false is not a good idea... + */ +static int +testQemuMonitorJSONSetObjectProperty(const void *data) +{ + const virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data; + qemuMonitorTestPtr test = qemuMonitorTestNew(true, xmlopt); + int ret = -1; + qemuMonitorObjectProperty prop; + + if (!test) + return -1; + + if (qemuMonitorTestAddItem(test, "qom-set", + "{ \"return\": {} }") < 0) + goto cleanup; + if (qemuMonitorTestAddItem(test, "qom-get", + "{ \"return\": true }") < 0) + goto cleanup; + + /* Let's attempt the setting */ + memset(&prop, 0, sizeof(qemuMonitorObjectProperty)); + prop.type = QEMU_MONITOR_OBJECT_PROPERTY_BOOLEAN; + prop.val.b = true; + if (qemuMonitorSetObjectProperty(qemuMonitorTestGetMonitor(test), + "/machine/i440fx", + "realized", + &prop) < 0) + goto cleanup; + + /* To make sure it worked, fetch the property - if this succeeds then + * we didn't hose things + */ + memset(&prop, 0, sizeof(qemuMonitorObjectProperty)); + prop.type = QEMU_MONITOR_OBJECT_PROPERTY_BOOLEAN; + if (qemuMonitorGetObjectProperty(qemuMonitorTestGetMonitor(test), + "/machine/i440fx", + "realized", + &prop) < 0) + goto cleanup; + + if (!prop.val.b) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + "expected true, but false returned"); + goto cleanup; + } + + ret = 0; +cleanup: + qemuMonitorTestFree(test); + return ret; +} + + static int mymain(void) { @@ -748,6 +806,7 @@ mymain(void) DO_TEST(GetCommandLineOptionParameters); DO_TEST(GetListPaths); DO_TEST(GetObjectProperty); + DO_TEST(SetObjectProperty); virObjectUnref(xmlopt); -- 1.8.1.4
On Tue, Jul 02, 2013 at 09:39:21AM -0400, John Ferlan wrote:
Add a new qemuMonitorSetObjectProperty() method to support invocation of the 'qom-set' JSON monitor command with a provided path, property, and expected data type to set.
The test code uses the same "/machine/i440fx" property as the get test and attempts to set the "realized" property to "true" (which it should be set at anyway). --- src/qemu/qemu_monitor.c | 23 ++++++++++++++++ src/qemu/qemu_monitor.h | 5 ++++ src/qemu/qemu_monitor_json.c | 62 ++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 6 +++++ tests/qemumonitorjsontest.c | 59 +++++++++++++++++++++++++++++++++++++++++ 5 files changed, 155 insertions(+)
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index cc22123..b822b97 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -727,6 +727,11 @@ int qemuMonitorGetObjectProperty(qemuMonitorPtr mon, const char *property, qemuMonitorObjectPropertyPtr prop);
+int qemuMonitorSetObjectProperty(qemuMonitorPtr mon, + const char *path, + const char *property, + qemuMonitorObjectPropertyPtr prop); +
Again, I think we should not expose this method in qemu_monitor.h 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 :|
Add a period in seconds to allow/enable statistics gathering from the Balloon driver for 'virsh dommemstat <domain>'. --- docs/schemas/domaincommon.rng | 7 +++++++ src/conf/domain_conf.c | 27 +++++++++++++++++++++++---- src/conf/domain_conf.h | 1 + 3 files changed, 31 insertions(+), 4 deletions(-) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index cf82878..53e707c 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2915,6 +2915,13 @@ <optional> <ref name="address"/> </optional> + <optional> + <element name="stats"> + <attribute name="period"> + <ref name="positiveInteger"/> + </attribute> + </element> + </optional> </element> </define> <define name="parallel"> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 011de71..cd7dbee 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8571,10 +8571,12 @@ error: static virDomainMemballoonDefPtr virDomainMemballoonDefParseXML(const xmlNodePtr node, + xmlXPathContextPtr ctxt, unsigned int flags) { char *model; virDomainMemballoonDefPtr def; + xmlNodePtr save = ctxt->node; if (VIR_ALLOC(def) < 0) { virReportOOMError(); @@ -8587,18 +8589,24 @@ virDomainMemballoonDefParseXML(const xmlNodePtr node, _("balloon memory must contain model name")); goto error; } + if ((def->model = virDomainMemballoonModelTypeFromString(model)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unknown memory balloon model '%s'"), model); goto error; } + ctxt->node = node; + if (virXPathInt("string(./stats/@period)", ctxt, &def->period) < 0) + def->period = 0; + if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0) goto error; cleanup: VIR_FREE(model); + ctxt->node = save; return def; error: @@ -11911,7 +11919,7 @@ virDomainDefParseXML(xmlDocPtr xml, } if (n > 0) { virDomainMemballoonDefPtr memballoon = - virDomainMemballoonDefParseXML(nodes[0], flags); + virDomainMemballoonDefParseXML(nodes[0], ctxt, flags); if (!memballoon) goto error; @@ -15080,6 +15088,7 @@ virDomainMemballoonDefFormat(virBufferPtr buf, unsigned int flags) { const char *model = virDomainMemballoonModelTypeToString(def->model); + bool noopts = true; if (!model) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -15093,11 +15102,21 @@ virDomainMemballoonDefFormat(virBufferPtr buf, virBufferAddLit(buf, ">\n"); if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) return -1; - virBufferAddLit(buf, " </memballoon>\n"); - } else { - virBufferAddLit(buf, "/>\n"); + noopts = false; } + if (def->period) { + if (noopts) + virBufferAddLit(buf, ">\n"); + virBufferAsprintf(buf, " <stats period='%d'/>\n", def->period); + noopts = false; + } + + if (noopts) + virBufferAddLit(buf, "/>\n"); + else + virBufferAddLit(buf, " </memballoon>\n"); + return 0; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 3817e37..405903f 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1527,6 +1527,7 @@ enum { struct _virDomainMemballoonDef { int model; virDomainDeviceInfo info; + int period; /* seconds between collections */ }; struct _virDomainNVRAMDef { -- 1.8.1.4
On Tue, Jul 02, 2013 at 09:39:22AM -0400, John Ferlan wrote:
Add a period in seconds to allow/enable statistics gathering from the Balloon driver for 'virsh dommemstat <domain>'. --- docs/schemas/domaincommon.rng | 7 +++++++ src/conf/domain_conf.c | 27 +++++++++++++++++++++++---- src/conf/domain_conf.h | 1 + 3 files changed, 31 insertions(+), 4 deletions(-)
It is preferred to include the docs for formatdomain.html.in in the same patch that introduces the rng/parser. So move the docs out of patch 8, into here. Regards, 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 02.07.2013 15:39, John Ferlan wrote:
Add a period in seconds to allow/enable statistics gathering from the Balloon driver for 'virsh dommemstat <domain>'. --- docs/schemas/domaincommon.rng | 7 +++++++ src/conf/domain_conf.c | 27 +++++++++++++++++++++++---- src/conf/domain_conf.h | 1 + 3 files changed, 31 insertions(+), 4 deletions(-)
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index cf82878..53e707c 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2915,6 +2915,13 @@ <optional> <ref name="address"/> </optional> + <optional> + <element name="stats"> + <attribute name="period"> + <ref name="positiveInteger"/> + </attribute> + </element> + </optional> </element> </define> <define name="parallel"> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 011de71..cd7dbee 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8571,10 +8571,12 @@ error:
static virDomainMemballoonDefPtr virDomainMemballoonDefParseXML(const xmlNodePtr node, + xmlXPathContextPtr ctxt, unsigned int flags) { char *model; virDomainMemballoonDefPtr def; + xmlNodePtr save = ctxt->node;
if (VIR_ALLOC(def) < 0) { virReportOOMError(); @@ -8587,18 +8589,24 @@ virDomainMemballoonDefParseXML(const xmlNodePtr node, _("balloon memory must contain model name")); goto error; } + if ((def->model = virDomainMemballoonModelTypeFromString(model)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unknown memory balloon model '%s'"), model); goto error; }
+ ctxt->node = node; + if (virXPathInt("string(./stats/@period)", ctxt, &def->period) < 0) + def->period = 0;
This is silently ignoring invalid integer value in @period. Moreover, it's merging two different cases into one: invalid integer and @period not present. After all, setting def->period to zero is done by VIR_ALLOC. So I think we must distinguish if virXPathInt returned -1 (@period not present in XML) or -2(@period is not valid integer). And we must not forget to check if user hasn't entered period < 0.
+ if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0) goto error;
cleanup: VIR_FREE(model);
+ ctxt->node = save; return def;
error: @@ -11911,7 +11919,7 @@ virDomainDefParseXML(xmlDocPtr xml, } if (n > 0) { virDomainMemballoonDefPtr memballoon = - virDomainMemballoonDefParseXML(nodes[0], flags); + virDomainMemballoonDefParseXML(nodes[0], ctxt, flags); if (!memballoon) goto error;
@@ -15080,6 +15088,7 @@ virDomainMemballoonDefFormat(virBufferPtr buf, unsigned int flags) { const char *model = virDomainMemballoonModelTypeToString(def->model); + bool noopts = true;
if (!model) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -15093,11 +15102,21 @@ virDomainMemballoonDefFormat(virBufferPtr buf, virBufferAddLit(buf, ">\n"); if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) return -1; - virBufferAddLit(buf, " </memballoon>\n"); - } else { - virBufferAddLit(buf, "/>\n"); + noopts = false; }
+ if (def->period) { + if (noopts) + virBufferAddLit(buf, ">\n"); + virBufferAsprintf(buf, " <stats period='%d'/>\n", def->period); + noopts = false; + } + + if (noopts) + virBufferAddLit(buf, "/>\n"); + else + virBufferAddLit(buf, " </memballoon>\n"); + return 0; }
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 3817e37..405903f 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1527,6 +1527,7 @@ enum { struct _virDomainMemballoonDef { int model; virDomainDeviceInfo info; + int period; /* seconds between collections */ };
struct _virDomainNVRAMDef {
Michal
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_domain.c | 1 + src/qemu/qemu_domain.h | 2 + src/qemu/qemu_process.c | 164 +++++++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 165 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 8d79066..5aaf1e1 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -244,6 +244,7 @@ qemuDomainObjPrivateFree(void *data) VIR_FREE(priv->vcpupids); VIR_FREE(priv->lockState); VIR_FREE(priv->origname); + VIR_FREE(priv->balloonpath); virChrdevFree(priv->devs); diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 068a4c3..005fd0f 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -161,6 +161,8 @@ struct _qemuDomainObjPrivate { char *origname; int nbdPort; /* Port used for migration with NBD */ + char *balloonpath; + virChrdevsPtr devs; qemuDomainCleanupCallback *cleanupCallbacks; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index ac5ffcf..9a2add1 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1564,6 +1564,150 @@ qemuProcessLookupPTYs(virDomainChrDefPtr *devices, return 0; } +/* 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 +qemuProcessFindBalloonObjectPath(qemuMonitorPtr mon, + virDomainObjPtr vm, + const char *curpath, + char **balloonpath) +{ + int i,j; + int npaths = 0; + int nprops = 0; + int ret = 0; + char *nextpath = NULL; + qemuMonitorListPathPtr *paths = NULL; + qemuMonitorListPathPtr *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 (*balloonpath) + return 1; + + VIR_DEBUG("Searching for Balloon Object Path starting at %s", curpath); + + npaths = qemuMonitorGetObjectListPaths(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 = qemuMonitorGetObjectListPaths(mon, nextpath, &bprops); + for (j = 0; j < nprops; j++) { + if (STREQ(bprops[j]->name, "guest-stats-polling-interval")) { + *balloonpath = nextpath; + nextpath = NULL; + ret = 1; + goto cleanup; + } + } + + /* 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 = qemuProcessFindBalloonObjectPath(mon, vm, nextpath, + balloonpath); + } + } + +cleanup: + for (i = 0; i < npaths; i++) + qemuMonitorListPathFree(paths[i]); + VIR_FREE(paths); + for (j = 0; j < nprops; j++) + qemuMonitorListPathFree(bprops[j]); + VIR_FREE(bprops); + VIR_FREE(nextpath); + return ret; +} + +/* + * Using the provided balloonpath, determine if we need to set the + * collection interval property to enable statistics gathering. + * + * NOTE: This assumes we have already called qemuDomainObjEnterMonitor() + */ +static void +qemuProcessUpdateBalloonStatsPeriod(qemuMonitorPtr mon, + char *balloonpath, + virDomainObjPtr vm) +{ + qemuMonitorObjectProperty prop; + + /* Get the current value of the stats polling interval */ + memset(&prop, 0, sizeof(qemuMonitorObjectProperty)); + prop.type = QEMU_MONITOR_OBJECT_PROPERTY_INT; + if (qemuMonitorGetObjectProperty(mon, balloonpath, + "guest-stats-polling-interval", + &prop) < 0) { + VIR_DEBUG("Failed to get polling interval for balloon driver"); + return; + } + + VIR_DEBUG("memballoon period=%d 'guest-stats-polling-interval'=%d", + vm->def->memballoon->period, prop.val.i); + + /* Same value - no need to set */ + if (vm->def->memballoon->period == prop.val.i) + return; + + /* Set to the value in memballoon (could enable or disable) */ + memset(&prop, 0, sizeof(qemuMonitorObjectProperty)); + prop.type = QEMU_MONITOR_OBJECT_PROPERTY_INT; + prop.val.i = vm->def->memballoon->period; + if (qemuMonitorSetObjectProperty(mon, balloonpath, + "guest-stats-polling-interval", + &prop) < 0) + VIR_DEBUG("Failed to set polling interval for balloon driver"); + +} + static int qemuProcessFindCharDevicePTYsMonitor(virDomainObjPtr vm, virQEMUCapsPtr qemuCaps, @@ -3047,6 +3191,12 @@ qemuProcessReconnect(void *opaque) if (qemuProcessRecoverJob(driver, obj, conn, &oldjob) < 0) goto error; + qemuDomainObjEnterMonitor(driver, obj); + if (qemuProcessFindBalloonObjectPath(priv->mon, obj, "/", + &priv->balloonpath) == 1) + qemuProcessUpdateBalloonStatsPeriod(priv->mon, priv->balloonpath, obj); + qemuDomainObjExitMonitor(driver, obj); + /* update domain state XML with possibly updated state in virDomainObj */ if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, obj) < 0) goto error; @@ -3867,6 +4017,9 @@ int qemuProcessStart(virConnectPtr conn, goto cleanup; } qemuDomainObjEnterMonitor(driver, vm); + if (qemuProcessFindBalloonObjectPath(priv->mon, vm, "/", + &priv->balloonpath) == 1) + qemuProcessUpdateBalloonStatsPeriod(priv->mon, priv->balloonpath, vm); if (qemuMonitorSetBalloon(priv->mon, cur_balloon) < 0) { qemuDomainObjExitMonitor(driver, vm); goto cleanup; @@ -4400,11 +4553,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 + qemuDomainObjEnterMonitor(driver, vm); + if (qemuProcessFindBalloonObjectPath(priv->mon, vm, "/", + &priv->balloonpath) == 1) + qemuProcessUpdateBalloonStatsPeriod(priv->mon, + priv->balloonpath, vm); + 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) -- 1.8.1.4
On Tue, Jul 02, 2013 at 09:39:23AM -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_domain.c | 1 + src/qemu/qemu_domain.h | 2 + src/qemu/qemu_process.c | 164 +++++++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 165 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 8d79066..5aaf1e1 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -244,6 +244,7 @@ qemuDomainObjPrivateFree(void *data) VIR_FREE(priv->vcpupids); VIR_FREE(priv->lockState); VIR_FREE(priv->origname); + VIR_FREE(priv->balloonpath);
virChrdevFree(priv->devs);
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 068a4c3..005fd0f 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -161,6 +161,8 @@ struct _qemuDomainObjPrivate { char *origname; int nbdPort; /* Port used for migration with NBD */
+ char *balloonpath; + virChrdevsPtr devs;
I think I'd prefer this to be kept as a private property in the struct qemuMonitor.
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index ac5ffcf..9a2add1 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1564,6 +1564,150 @@ qemuProcessLookupPTYs(virDomainChrDefPtr *devices, return 0; }
+/* 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 +qemuProcessFindBalloonObjectPath(qemuMonitorPtr mon, + virDomainObjPtr vm, + const char *curpath, + char **balloonpath) +{ + int i,j; + int npaths = 0; + int nprops = 0; + int ret = 0; + char *nextpath = NULL; + qemuMonitorListPathPtr *paths = NULL; + qemuMonitorListPathPtr *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 (*balloonpath) + return 1; + + VIR_DEBUG("Searching for Balloon Object Path starting at %s", curpath); + + npaths = qemuMonitorGetObjectListPaths(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 = qemuMonitorGetObjectListPaths(mon, nextpath, &bprops); + for (j = 0; j < nprops; j++) { + if (STREQ(bprops[j]->name, "guest-stats-polling-interval")) { + *balloonpath = nextpath; + nextpath = NULL; + ret = 1; + goto cleanup; + } + } + + /* 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 = qemuProcessFindBalloonObjectPath(mon, vm, nextpath, + balloonpath); + } + } + +cleanup: + for (i = 0; i < npaths; i++) + qemuMonitorListPathFree(paths[i]); + VIR_FREE(paths); + for (j = 0; j < nprops; j++) + qemuMonitorListPathFree(bprops[j]); + VIR_FREE(bprops); + VIR_FREE(nextpath); + return ret; +}
I think this method should be kept in qemu_monitor.c and made static to that file We can then populate it on first use.
+/* + * Using the provided balloonpath, determine if we need to set the + * collection interval property to enable statistics gathering. + * + * NOTE: This assumes we have already called qemuDomainObjEnterMonitor() + */ +static void +qemuProcessUpdateBalloonStatsPeriod(qemuMonitorPtr mon, + char *balloonpath, + virDomainObjPtr vm) +{ + qemuMonitorObjectProperty prop; + + /* Get the current value of the stats polling interval */ + memset(&prop, 0, sizeof(qemuMonitorObjectProperty)); + prop.type = QEMU_MONITOR_OBJECT_PROPERTY_INT; + if (qemuMonitorGetObjectProperty(mon, balloonpath, + "guest-stats-polling-interval", + &prop) < 0) { + VIR_DEBUG("Failed to get polling interval for balloon driver"); + return; + } + + VIR_DEBUG("memballoon period=%d 'guest-stats-polling-interval'=%d", + vm->def->memballoon->period, prop.val.i); + + /* Same value - no need to set */ + if (vm->def->memballoon->period == prop.val.i) + return; + + /* Set to the value in memballoon (could enable or disable) */ + memset(&prop, 0, sizeof(qemuMonitorObjectProperty)); + prop.type = QEMU_MONITOR_OBJECT_PROPERTY_INT; + prop.val.i = vm->def->memballoon->period; + if (qemuMonitorSetObjectProperty(mon, balloonpath, + "guest-stats-polling-interval", + &prop) < 0) + VIR_DEBUG("Failed to set polling interval for balloon driver"); + +}
Since I recommended that we don't expose qemuMonitorGetObjectProperty or qemuMonitorSetObjectProperty in qemu_monitor.h, what you'll need todo here is move this code into qemu_monitor.c defining a method something like int qemuMonitorSetBalloonStatsPeriod(qemuMonitorPtr mon, int period); This method can populate the balloonpath field in qemuMOnitorPtr struct on first use. Regards, 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 :|
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.h | 4 +++ src/qemu/qemu_monitor_json.c | 76 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+) diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index b822b97..4199160 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -703,6 +703,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 } qemuMonitorObjectPropertyType; @@ -711,6 +712,8 @@ typedef struct _qemuMonitorObjectProperty qemuMonitorObjectProperty; typedef qemuMonitorObjectProperty *qemuMonitorObjectPropertyPtr; struct _qemuMonitorObjectProperty { int type; /* qemuMonitorObjectPropertyType */ + int curelems; /* Current number elements in **ptr array */ + int maxelems; /* Maximum number elements allowed in any **ptr array */ union { bool b; int i; @@ -719,6 +722,7 @@ struct _qemuMonitorObjectProperty { unsigned long long ul; double d; char *str; + void **ptr; } val; }; diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index c599626..49001a8 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4627,6 +4627,78 @@ cleanup: } +/* Process the balloon driver statistics. The request and data returned + * will be as follows (although the 'child[#]' entry will differ based on + * where it's run). + * + * { "execute": "qom-get","arguments": \ + * { "path": "/machine/i440fx/pci.0/child[7]","property": "guest-stats"} } + * + * {"return": {"stats": \ + * {"stat-swap-out": 0, + * "stat-free-memory": 686350336, + * "stat-minor-faults": 697283, + * "stat-major-faults": 951, + * "stat-total-memory": 1019924480, + * "stat-swap-in": 0}, + * "last-update": 1371221540}} + * + * A value in "stats" can be -1 indicating it's never been collected/stored. + * The 'last-update' value could be used in the future in order to determine + * rates and/or whether data has been collected since a previous cycle. + * It's currently unused. + */ +#define GET_BALLOON_STATS(FIELD, TAG, DIVISOR) \ + if (virJSONValueObjectHasKey(statsdata, FIELD) && \ + (prop->curelems < prop->maxelems)) { \ + if (virJSONValueObjectGetNumberUlong(statsdata, FIELD, &mem) < 0) { \ + VIR_DEBUG("Failed to get '%s' value", FIELD); \ + } else { \ + /* Not being collected? No point in providing bad data */ \ + if (mem != -1UL) { \ + stat[prop->curelems].tag = TAG; \ + stat[prop->curelems].val = mem / DIVISOR; \ + prop->curelems++; \ + } \ + } \ + } + +static int +qemuMonitorJSONGetBalloonStats(virJSONValuePtr data, + qemuMonitorObjectPropertyPtr prop) +{ + int ret = -1; + unsigned long long mem; + virJSONValuePtr statsdata; + virDomainMemoryStatPtr stat = (virDomainMemoryStatPtr)prop->val.ptr; + + VIR_DEBUG("Address of found stat = %p", stat); + + if (!(statsdata = virJSONValueObjectGet(data, "stats"))) { + VIR_DEBUG("data does not include 'stats'"); + goto cleanup; + } + + GET_BALLOON_STATS("stat-swap-in", + VIR_DOMAIN_MEMORY_STAT_SWAP_IN, 1024); + GET_BALLOON_STATS("stat-swap-out", + VIR_DOMAIN_MEMORY_STAT_SWAP_OUT, 1024); + GET_BALLOON_STATS("stat-major-faults", + VIR_DOMAIN_MEMORY_STAT_MAJOR_FAULT, 1); + GET_BALLOON_STATS("stat-minor-faults", + VIR_DOMAIN_MEMORY_STAT_MINOR_FAULT, 1); + GET_BALLOON_STATS("stat-free-memory", + VIR_DOMAIN_MEMORY_STAT_UNUSED, 1024); + GET_BALLOON_STATS("stat-total-memory", + VIR_DOMAIN_MEMORY_STAT_AVAILABLE, 1024); + + ret = 0; + +cleanup: + virJSONValueFree(statsdata); + return ret; +} + int qemuMonitorJSONGetObjectProperty(qemuMonitorPtr mon, const char *path, const char *property, @@ -4689,6 +4761,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, @@ -4752,6 +4827,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, -- 1.8.1.4
On Tue, Jul 02, 2013 at 09:39:24AM -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.h | 4 +++ src/qemu/qemu_monitor_json.c | 76 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+)
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index b822b97..4199160 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -703,6 +703,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 } qemuMonitorObjectPropertyType; @@ -711,6 +712,8 @@ typedef struct _qemuMonitorObjectProperty qemuMonitorObjectProperty; typedef qemuMonitorObjectProperty *qemuMonitorObjectPropertyPtr; struct _qemuMonitorObjectProperty { int type; /* qemuMonitorObjectPropertyType */ + int curelems; /* Current number elements in **ptr array */ + int maxelems; /* Maximum number elements allowed in any **ptr array */ union { bool b; int i; @@ -719,6 +722,7 @@ struct _qemuMonitorObjectProperty { unsigned long long ul; double d; char *str; + void **ptr;
Hmm, not sure I really like the look of this opaque pointer.
} val; };
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index c599626..49001a8 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4627,6 +4627,78 @@ cleanup: }
+/* Process the balloon driver statistics. The request and data returned + * will be as follows (although the 'child[#]' entry will differ based on + * where it's run). + * + * { "execute": "qom-get","arguments": \ + * { "path": "/machine/i440fx/pci.0/child[7]","property": "guest-stats"} } + * + * {"return": {"stats": \ + * {"stat-swap-out": 0, + * "stat-free-memory": 686350336, + * "stat-minor-faults": 697283, + * "stat-major-faults": 951, + * "stat-total-memory": 1019924480, + * "stat-swap-in": 0}, + * "last-update": 1371221540}} + * + * A value in "stats" can be -1 indicating it's never been collected/stored. + * The 'last-update' value could be used in the future in order to determine + * rates and/or whether data has been collected since a previous cycle. + * It's currently unused. + */ +#define GET_BALLOON_STATS(FIELD, TAG, DIVISOR) \ + if (virJSONValueObjectHasKey(statsdata, FIELD) && \ + (prop->curelems < prop->maxelems)) { \ + if (virJSONValueObjectGetNumberUlong(statsdata, FIELD, &mem) < 0) { \ + VIR_DEBUG("Failed to get '%s' value", FIELD); \ + } else { \ + /* Not being collected? No point in providing bad data */ \ + if (mem != -1UL) { \ + stat[prop->curelems].tag = TAG; \ + stat[prop->curelems].val = mem / DIVISOR; \ + prop->curelems++; \ + } \ + } \ + } + +static int +qemuMonitorJSONGetBalloonStats(virJSONValuePtr data, + qemuMonitorObjectPropertyPtr prop) +{ + int ret = -1; + unsigned long long mem; + virJSONValuePtr statsdata; + virDomainMemoryStatPtr stat = (virDomainMemoryStatPtr)prop->val.ptr; + + VIR_DEBUG("Address of found stat = %p", stat); + + if (!(statsdata = virJSONValueObjectGet(data, "stats"))) { + VIR_DEBUG("data does not include 'stats'"); + goto cleanup; + } + + GET_BALLOON_STATS("stat-swap-in", + VIR_DOMAIN_MEMORY_STAT_SWAP_IN, 1024); + GET_BALLOON_STATS("stat-swap-out", + VIR_DOMAIN_MEMORY_STAT_SWAP_OUT, 1024); + GET_BALLOON_STATS("stat-major-faults", + VIR_DOMAIN_MEMORY_STAT_MAJOR_FAULT, 1); + GET_BALLOON_STATS("stat-minor-faults", + VIR_DOMAIN_MEMORY_STAT_MINOR_FAULT, 1); + GET_BALLOON_STATS("stat-free-memory", + VIR_DOMAIN_MEMORY_STAT_UNUSED, 1024); + GET_BALLOON_STATS("stat-total-memory", + VIR_DOMAIN_MEMORY_STAT_AVAILABLE, 1024); + + ret = 0; + +cleanup: + virJSONValueFree(statsdata); + return ret; +}
You'll want an '#undef GET_BALLOON_STATS' call.
+ int qemuMonitorJSONGetObjectProperty(qemuMonitorPtr mon, const char *path, const char *property, @@ -4689,6 +4761,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, @@ -4752,6 +4827,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,
We already have a method qemuMonitor{JSON}GetMemoryStats which was using the legacy 'query-balloon' interface that QEMU has removed now. IMHO we should just update that existing API to fetch the data using the new QOM based monitor commands. Then existing code using that API in the QEMU driver will "just work". 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 :|
In the 'virsh dommemstats <domain>' qemu driver code, if we have a balloonpath and we hadn't already collected the data (somehow) from the "query-balloon" command, then use qom-get to attempt to fill in the domain memory stats data from the balloon driver. --- src/qemu/qemu_driver.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f51e766..c9a66ff 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9499,6 +9499,7 @@ qemuDomainMemoryStats(virDomainPtr dom, virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm; int ret = -1; + qemuMonitorObjectProperty prop; virCheckFlags(0, -1); @@ -9518,6 +9519,22 @@ qemuDomainMemoryStats(virDomainPtr dom, qemuDomainObjPrivatePtr priv = vm->privateData; qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorGetMemoryStats(priv->mon, stats, nr_stats); + + /* The above call should only return the 'actual' memory due to + * changes that disabled statistics gathering from query-balloon. + * If so, let's check if we have a path to the balloon device + * and then try to gather more stats from there if possible + */ + if ((ret == 0 || ret == 1) && priv->balloonpath) { + memset(&prop, 0, sizeof(qemuMonitorObjectProperty)); + prop.type = QEMU_MONITOR_OBJECT_PROPERTY_BALLOON_STATS; + prop.curelems = ret; + prop.maxelems = nr_stats; + prop.val.ptr = (void **)stats; + if (qemuMonitorGetObjectProperty(priv->mon, priv->balloonpath, + "guest-stats", &prop) == 0) + ret = prop.curelems; + } qemuDomainObjExitMonitor(driver, vm); if (ret >= 0 && ret < nr_stats) { -- 1.8.1.4
On Tue, Jul 02, 2013 at 09:39:25AM -0400, John Ferlan wrote:
In the 'virsh dommemstats <domain>' qemu driver code, if we have a balloonpath and we hadn't already collected the data (somehow) from the "query-balloon" command, then use qom-get to attempt to fill in the domain memory stats data from the balloon driver. --- src/qemu/qemu_driver.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f51e766..c9a66ff 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9499,6 +9499,7 @@ qemuDomainMemoryStats(virDomainPtr dom, virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm; int ret = -1; + qemuMonitorObjectProperty prop;
virCheckFlags(0, -1);
@@ -9518,6 +9519,22 @@ qemuDomainMemoryStats(virDomainPtr dom, qemuDomainObjPrivatePtr priv = vm->privateData; qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorGetMemoryStats(priv->mon, stats, nr_stats); + + /* The above call should only return the 'actual' memory due to + * changes that disabled statistics gathering from query-balloon. + * If so, let's check if we have a path to the balloon device + * and then try to gather more stats from there if possible + */ + if ((ret == 0 || ret == 1) && priv->balloonpath) { + memset(&prop, 0, sizeof(qemuMonitorObjectProperty)); + prop.type = QEMU_MONITOR_OBJECT_PROPERTY_BALLOON_STATS; + prop.curelems = ret; + prop.maxelems = nr_stats; + prop.val.ptr = (void **)stats; + if (qemuMonitorGetObjectProperty(priv->mon, priv->balloonpath, + "guest-stats", &prop) == 0) + ret = prop.curelems; + }
If we hide all the QOM stuff inside the existing qemuMonitorGetMemoryStats() API, then this patch just goes away entirely. Regards, 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 :|
Extend the virDomainSetMemeoryFlags() to accept a 'VIR_DOMAIN_MEM_PERIOD' which will be used to dynamically set the collection period for the balloon driver via a 'virsh dommemstat <domain> --period <value>' command. Add the --current, --live, & --config options to dommemstat. --- docs/formatdomain.html.in | 14 +++++++++ include/libvirt/libvirt.h.in | 1 + src/libvirt.c | 8 ++++- src/qemu/qemu_driver.c | 44 +++++++++++++++++++++++++++- tools/virsh-domain-monitor.c | 70 ++++++++++++++++++++++++++++++++++++++++++-- 5 files changed, 132 insertions(+), 5 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index cc4c5ea..0ec256d 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -4612,6 +4612,7 @@ qemu-kvm -net nic,model=? /dev/null <devices> <memballoon model='virtio'> <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + <stats period='10'/> </memballoon> </devices> </domain></pre> @@ -4629,6 +4630,19 @@ qemu-kvm -net nic,model=? /dev/null <li>'xen' — default with Xen</li> </ul> </dd> + <dt><code>period</code></dt> + <dd> + <p> + The optional <code>period</code> allows the QEMU virtio memory + balloon driver to provide statistics through the <code>virsh + dommemstat [domain]</code> command. By default, collection is + not enabled. In order to enable, use the <code>virsh dommemstat + [domain] --period [number]</code> command or <code>virsh edit</code> + command to add the option. If the QEMU driver is not at the right + revision, the attempt to set the period will fail. + <span class='since'>Since 1.1.1, requires QEMU 1.5</span> + </p> + </dd> </dl> <h4><a name="elementsRng">Random number generator device</a></h4> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index b791125..1fc6e82 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -1844,6 +1844,7 @@ typedef enum { /* Additionally, these flags may be bitwise-OR'd in. */ VIR_DOMAIN_MEM_MAXIMUM = (1 << 2), /* affect Max rather than current */ + VIR_DOMAIN_MEM_PERIOD = (1 << 3), /* set dommemstats period */ } virDomainMemoryModFlags; diff --git a/src/libvirt.c b/src/libvirt.c index bc1694a..81745bb 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -3738,6 +3738,9 @@ error: * on whether just live or both live and persistent state is changed. * If VIR_DOMAIN_MEM_MAXIMUM is set, the change affects domain's maximum memory * size rather than current memory size. + * If VIR_DOMAIN_MEM_PERIOD is set, the domain memory statistics collection + * period for the balloon driver will be adjusted. Use 0 to disable and + * a positive value to enable. * Not all hypervisors can support all flag combinations. * * Returns 0 in case of success, -1 in case of failure. @@ -3763,7 +3766,10 @@ virDomainSetMemoryFlags(virDomainPtr domain, unsigned long memory, virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); goto error; } - virCheckNonZeroArgGoto(memory, error); + + /* This can be non zero only for setting the balloon collection period */ + if (!(flags & VIR_DOMAIN_MEM_PERIOD)) + virCheckNonZeroArgGoto(memory, error); conn = domain->conn; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c9a66ff..b76e634 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2166,7 +2166,8 @@ static int qemuDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG | - VIR_DOMAIN_MEM_MAXIMUM, -1); + VIR_DOMAIN_MEM_MAXIMUM | + VIR_DOMAIN_MEM_PERIOD, -1); if (!(vm = qemuDomObjFromDomain(dom))) goto cleanup; @@ -2205,6 +2206,47 @@ static int qemuDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, goto endjob; } + } else if (flags & VIR_DOMAIN_MEM_PERIOD) { + /* Set the balloon driver collection interval */ + priv = vm->privateData; + if (!priv->balloonpath) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("memory balloon driver not defined or " + "not virtio model")); + goto endjob; + } + + if (newmem != (int)newmem) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("period value=%lu does not match set value=%d"), + newmem, (int)newmem); + goto endjob; + } + + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + qemuMonitorObjectProperty prop; + + memset(&prop, 0, sizeof(qemuMonitorObjectProperty)); + prop.type = QEMU_MONITOR_OBJECT_PROPERTY_INT; + prop.val.i = newmem; + + qemuDomainObjEnterMonitor(driver, vm); + r = qemuMonitorSetObjectProperty(priv->mon, priv->balloonpath, + "guest-stats-polling-interval", + &prop); + qemuDomainObjExitMonitor(driver, vm); + if (r < 0) + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("unable to set balloon driver collection " + "period")); + } + + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + sa_assert(persistentDef); + persistentDef->memballoon->period = newmem; + ret = virDomainSaveConfig(cfg->configDir, persistentDef); + goto endjob; + } } else { /* resize the current memory */ diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index 3ba829c..f123247 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -306,6 +306,23 @@ static const vshCmdOptDef opts_dommemstat[] = { .flags = VSH_OFLAG_REQ, .help = N_("domain name, id or uuid") }, + {.name = "period", + .type = VSH_OT_DATA, + .flags = VSH_OFLAG_EMPTY_OK, + .help = N_("period in seconds to set collection") + }, + {.name = "config", + .type = VSH_OT_BOOL, + .help = N_("affect next boot") + }, + {.name = "live", + .type = VSH_OT_BOOL, + .help = N_("affect running domain") + }, + {.name = "current", + .type = VSH_OT_BOOL, + .help = N_("affect current domain") + }, {.name = NULL} }; @@ -316,15 +333,60 @@ cmdDomMemStat(vshControl *ctl, const vshCmd *cmd) const char *name; struct _virDomainMemoryStat stats[VIR_DOMAIN_MEMORY_STAT_NR]; unsigned int nr_stats, i; + int ret = false; + int rv = 0; + int period = -1; + bool config = vshCommandOptBool(cmd, "config"); + bool live = vshCommandOptBool(cmd, "live"); + bool current = vshCommandOptBool(cmd, "current"); + unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT; + + VSH_EXCLUSIVE_OPTIONS_VAR(current, live); + VSH_EXCLUSIVE_OPTIONS_VAR(current, config); + if (config) + flags |= VIR_DOMAIN_AFFECT_CONFIG; + if (live) + flags |= VIR_DOMAIN_AFFECT_LIVE; if (!(dom = vshCommandOptDomain(ctl, cmd, &name))) return false; + /* none of the options were specified - choose defaults based on state */ + if (!current && !live && !config) { + if (virDomainIsActive(dom) == 1) + flags |= VIR_DOMAIN_AFFECT_LIVE; + else + flags |= VIR_DOMAIN_AFFECT_CONFIG; + } + + /* Providing a period will adjust the balloon driver collection period. + * This is not really an unsigned long, but it + */ + if ((rv = vshCommandOptInt(cmd, "period", &period)) < 0) { + vshError(ctl, "%s", + _("Unable to parse integer parameter.")); + goto cleanup; + } + if (rv > 0) { + if (period < 0) { + vshError(ctl, _("Invalid collection period value '%d'"), period); + goto cleanup; + } + + flags |= VIR_DOMAIN_MEM_PERIOD; + if (virDomainSetMemoryFlags(dom, period, flags) < 0) { + vshError(ctl, "%s", + _("Unable to change balloon collection period.")); + } else { + ret = true; + } + goto cleanup; + } + nr_stats = virDomainMemoryStats(dom, stats, VIR_DOMAIN_MEMORY_STAT_NR, 0); if (nr_stats == -1) { vshError(ctl, _("Failed to get memory statistics for domain %s"), name); - virDomainFree(dom); - return false; + goto cleanup; } for (i = 0; i < nr_stats; i++) { @@ -346,8 +408,10 @@ cmdDomMemStat(vshControl *ctl, const vshCmd *cmd) vshPrint(ctl, "rss %llu\n", stats[i].val); } + ret = true; +cleanup: virDomainFree(dom); - return true; + return ret; } /* -- 1.8.1.4
On Tue, Jul 02, 2013 at 09:39:26AM -0400, John Ferlan wrote:
Extend the virDomainSetMemeoryFlags() to accept a 'VIR_DOMAIN_MEM_PERIOD' which will be used to dynamically set the collection period for the balloon driver via a 'virsh dommemstat <domain> --period <value>' command. Add the --current, --live, & --config options to dommemstat.
While what you've done here works from a technical POV, I'm not a fan of overloading the semantics of the API using a flag. Tuning memory stat collection period has no semantic overlap with tuning of memory limits. So I'm inclined to say that we should have a new dedicated API for tuning the memory stats period. Regards, 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 02.07.2013 15:39, John Ferlan wrote:
Extend the virDomainSetMemeoryFlags() to accept a 'VIR_DOMAIN_MEM_PERIOD' which will be used to dynamically set the collection period for the balloon driver via a 'virsh dommemstat <domain> --period <value>' command. Add the --current, --live, & --config options to dommemstat. --- docs/formatdomain.html.in | 14 +++++++++ include/libvirt/libvirt.h.in | 1 + src/libvirt.c | 8 ++++- src/qemu/qemu_driver.c | 44 +++++++++++++++++++++++++++- tools/virsh-domain-monitor.c | 70 ++++++++++++++++++++++++++++++++++++++++++-- 5 files changed, 132 insertions(+), 5 deletions(-)
diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index 3ba829c..f123247 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -306,6 +306,23 @@ static const vshCmdOptDef opts_dommemstat[] = { .flags = VSH_OFLAG_REQ, .help = N_("domain name, id or uuid") }, + {.name = "period", + .type = VSH_OT_DATA, + .flags = VSH_OFLAG_EMPTY_OK, + .help = N_("period in seconds to set collection") + }, + {.name = "config", + .type = VSH_OT_BOOL, + .help = N_("affect next boot") + }, + {.name = "live", + .type = VSH_OT_BOOL, + .help = N_("affect running domain") + }, + {.name = "current", + .type = VSH_OT_BOOL, + .help = N_("affect current domain") + }, {.name = NULL} };
@@ -316,15 +333,60 @@ cmdDomMemStat(vshControl *ctl, const vshCmd *cmd) const char *name; struct _virDomainMemoryStat stats[VIR_DOMAIN_MEMORY_STAT_NR]; unsigned int nr_stats, i; + int ret = false; + int rv = 0; + int period = -1; + bool config = vshCommandOptBool(cmd, "config"); + bool live = vshCommandOptBool(cmd, "live"); + bool current = vshCommandOptBool(cmd, "current"); + unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT; + + VSH_EXCLUSIVE_OPTIONS_VAR(current, live); + VSH_EXCLUSIVE_OPTIONS_VAR(current, config); + if (config) + flags |= VIR_DOMAIN_AFFECT_CONFIG; + if (live) + flags |= VIR_DOMAIN_AFFECT_LIVE;
if (!(dom = vshCommandOptDomain(ctl, cmd, &name))) return false;
+ /* none of the options were specified - choose defaults based on state */ + if (!current && !live && !config) { + if (virDomainIsActive(dom) == 1) + flags |= VIR_DOMAIN_AFFECT_LIVE; + else + flags |= VIR_DOMAIN_AFFECT_CONFIG;
This should not be needed as qemu driver will the proper option for VIR_DOMAIN_AFFECT_CURRENT. Client can't make such decision as domain is not locked and may translate into different state.
+ } + + /* Providing a period will adjust the balloon driver collection period. + * This is not really an unsigned long, but it + */ + if ((rv = vshCommandOptInt(cmd, "period", &period)) < 0) { + vshError(ctl, "%s", + _("Unable to parse integer parameter.")); + goto cleanup; + } + if (rv > 0) { + if (period < 0) { + vshError(ctl, _("Invalid collection period value '%d'"), period); + goto cleanup; + } + + flags |= VIR_DOMAIN_MEM_PERIOD; + if (virDomainSetMemoryFlags(dom, period, flags) < 0) { + vshError(ctl, "%s", + _("Unable to change balloon collection period.")); + } else { + ret = true; + } + goto cleanup; + } + nr_stats = virDomainMemoryStats(dom, stats, VIR_DOMAIN_MEMORY_STAT_NR, 0); if (nr_stats == -1) { vshError(ctl, _("Failed to get memory statistics for domain %s"), name); - virDomainFree(dom); - return false; + goto cleanup; }
for (i = 0; i < nr_stats; i++) { @@ -346,8 +408,10 @@ cmdDomMemStat(vshControl *ctl, const vshCmd *cmd) vshPrint(ctl, "rss %llu\n", stats[i].val); }
+ ret = true; +cleanup: virDomainFree(dom); - return true; + return ret; }
/*
Michal
On 07/03/2013 08:03 AM, Michal Privoznik wrote:
On 02.07.2013 15:39, John Ferlan wrote:
Extend the virDomainSetMemeoryFlags() to accept a 'VIR_DOMAIN_MEM_PERIOD' which will be used to dynamically set the collection period for the balloon driver via a 'virsh dommemstat <domain> --period <value>' command. Add the --current, --live, & --config options to dommemstat. --- docs/formatdomain.html.in | 14 +++++++++ include/libvirt/libvirt.h.in | 1 + src/libvirt.c | 8 ++++- src/qemu/qemu_driver.c | 44 +++++++++++++++++++++++++++- tools/virsh-domain-monitor.c | 70 ++++++++++++++++++++++++++++++++++++++++++-- 5 files changed, 132 insertions(+), 5 deletions(-)
diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
...snip...
if (!(dom = vshCommandOptDomain(ctl, cmd, &name))) return false;
+ /* none of the options were specified - choose defaults based on state */ + if (!current && !live && !config) { + if (virDomainIsActive(dom) == 1) + flags |= VIR_DOMAIN_AFFECT_LIVE; + else + flags |= VIR_DOMAIN_AFFECT_CONFIG;
This should not be needed as qemu driver will the proper option for VIR_DOMAIN_AFFECT_CURRENT. Client can't make such decision as domain is not locked and may translate into different state.
I removed the 'else' condition and merely was following other examples for the 'if' option. John
Michal
On 07/02/2013 09:39 AM, John Ferlan wrote: <...snip...>
John Ferlan (8): Add qemuMonitorGetObjectListPaths() method for QMP qom-list command Add qemuMonitorGetObjectProperty() method for QMP qom-get command Add qemuMonitorSetObjectProperty() method for QMP qom-set command Add 'period' for Memballoon statistics gathering capability Determine whether to start balloon memory stats gathering. Add capability to fetch balloon stats If available fetch the balloon driver memory stats Allow balloon driver collection to be adjusted dynamically
docs/formatdomain.html.in | 14 ++ docs/schemas/domaincommon.rng | 7 + include/libvirt/libvirt.h.in | 1 + src/conf/domain_conf.c | 27 +++- src/conf/domain_conf.h | 1 + src/libvirt.c | 8 +- src/qemu/qemu_domain.c | 1 + src/qemu/qemu_domain.h | 2 + src/qemu/qemu_driver.c | 61 +++++++- src/qemu/qemu_monitor.c | 79 +++++++++++ src/qemu/qemu_monitor.h | 57 ++++++++ src/qemu/qemu_monitor_json.c | 317 ++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 18 +++ src/qemu/qemu_process.c | 164 +++++++++++++++++++++- tests/qemumonitorjsontest.c | 184 ++++++++++++++++++++++++ tools/virsh-domain-monitor.c | 70 +++++++++- 16 files changed, 1000 insertions(+), 11 deletions(-)
FYI: I have rebased to the most recently pushed patches and have removed the virReportOOMError()'s from patch #1 and #5 John
participants (4)
-
Daniel P. Berrange -
John Ferlan -
Ján Tomko -
Michal Privoznik