On 12/11/2014 10:42 AM, Pavel Hrdina wrote:

The search is done recursively only through QOM object that has a type prefixed with "child<" as this indicate that the QOM is a parent for other QOM objects.

The usage is that you give known device name with starting path where to search.

Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_monitor.c | 173 ++++++++++++++++++++++++++++-------------------- 1 file changed, 102 insertions(+), 71 deletions(-)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index c9c84f9..ee4460f 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1008,22 +1008,90 @@ qemuMonitorSetOptions(qemuMonitorPtr mon, virJSONValuePtr options) mon->options = options; }

-/* Search the qom objects for the balloon driver object by it's known name - * of "virtio-balloon-pci". The entry for the driver will be found in the - * returned 'type' field using the syntax "child<virtio-balloon-pci>". - * - * Once found, check the entry to ensure it has the correct property listed. - * If it does not, then obtaining statistics from qemu will not be possible. - * This feature was added to qemu 1.5. + +/** + * Search the qom objects by it's known name. The name is compared against + * filed 'type' formatted as 'link<%name>'. * * This procedure will be call recursively until found or the qom-list is * exhausted. * * Returns: * - * 1 - Found - * 0 - Not found still looking + * 0 - Found * -1 - Error bail out + * -2 - Not found + * + * NOTE: This assumes we have already called qemuDomainObjEnterMonitor() + */ +static int +qemuMonitorFindObjectPath(qemuMonitorPtr mon, + const char *curpath, + const char *name, + char **path) +{ + ssize_t i, npaths = 0; + int ret = -2; + char *nextpath = NULL; + char *type = NULL; + qemuMonitorJSONListPathPtr *paths = NULL; + + if (virAsprintf(&type, "link<%s>", name) < 0) + return -1; + + VIR_DEBUG("Searching for '%s' Object Path starting at '%s'", type, curpath); + + npaths = qemuMonitorJSONGetObjectListPaths(mon, curpath, &paths); + if (npaths < 0) + goto cleanup; + + for (i = 0; i < npaths && ret == -2; i++) { + + if (STREQ_NULLABLE(paths[i]->type, type)) { + VIR_DEBUG("Path to '%s' is '%s/%s'", type, curpath, paths[i]->name); + ret = 0; + if (virAsprintf(path, "%s/%s", curpath, paths[i]->name) < 0) { + *path = NULL; + ret = -1; + } + goto cleanup; + } + + /* Type entries that begin with "child<" are a branch that can be + * traversed looking for more entries + */ + if (paths[i]->type && STRPREFIX(paths[i]->type, "child<")) { + if (virAsprintf(&nextpath, "%s/%s", curpath, paths[i]->name) < 0) { + ret = -1; + goto cleanup; + } + + ret = qemuMonitorFindObjectPath(mon, nextpath, name, path); + } + } + + cleanup: + for (i = 0; i < npaths; i++) + qemuMonitorJSONListPathFree(paths[i]); + VIR_FREE(paths); + VIR_FREE(nextpath); + VIR_FREE(type); + return ret; +} + +

NIT: You added a /** as the comment opener previously, but not here.

+/* Search the qom objects for the balloon driver object by it's known name + * of "virtio-balloon-pci". The entry for the driver will be found by using + * function "qemuMonitorFindObjectPath". + * + * Once found, check the entry to ensure it has the correct property listed. + * If it does not, then obtaining statistics from QEMU will not be possible. + * This feature was added to QEMU 1.5. + * + * Returns: + * + * 0 - Found + * -1 - Not found or error * * NOTE: This assumes we have already called qemuDomainObjEnterMonitor() */ @@ -1032,14 +1100,13 @@ qemuMonitorFindBalloonObjectPath(qemuMonitorPtr mon, virDomainObjPtr vm, const char *curpath) { - ssize_t i, j, npaths = 0, nprops = 0; - int ret = 0; - char *nextpath = NULL; - qemuMonitorJSONListPathPtr *paths = NULL; + ssize_t i, nprops = 0; + int ret = -1; + char *path = NULL; qemuMonitorJSONListPathPtr *bprops = NULL;

if (mon->balloonpath) { - return 1; + return 0; } else if (mon->ballooninit) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Cannot determine balloon device path")); @@ -1055,70 +1122,34 @@ qemuMonitorFindBalloonObjectPath(qemuMonitorPtr mon, return -1; }

- VIR_DEBUG("Searching for Balloon Object Path starting at %s", curpath); - - npaths = qemuMonitorJSONGetObjectListPaths(mon, curpath, &paths); - if (npaths < 0) + if (qemuMonitorFindObjectPath(mon, curpath, "virtio-balloon-pci", &path) < 0) return -1;

- for (i = 0; i < npaths && ret == 0; i++) { - - if (STREQ_NULLABLE(paths[i]->type, "link<virtio-balloon-pci>")) { - VIR_DEBUG("Path to <virtio-balloon-pci> is '%s/%s'", - curpath, paths[i]->name); - if (virAsprintf(&nextpath, "%s/%s", curpath, paths[i]->name) < 0) { - ret = -1; - goto cleanup; - } - - /* Now look at the each of the property entries to determine - * whether "guest-stats-polling-interval" exists. If not, - * then this version of qemu/kvm does not support the feature. - */ - nprops = qemuMonitorJSONGetObjectListPaths(mon, nextpath, &bprops); - if (nprops < 0) { - ret = -1; - goto cleanup; - } - - for (j = 0; j < nprops; j++) { - if (STREQ(bprops[j]->name, "guest-stats-polling-interval")) { - VIR_DEBUG("Found Balloon Object Path %s", nextpath); - mon->balloonpath = nextpath; - nextpath = NULL; - ret = 1; - goto cleanup; - } - } - - /* If we get here, we found the path, but not the property */ - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Property 'guest-stats-polling-interval' " - "not found on memory balloon driver.")); - ret = -1; + nprops = qemuMonitorJSONGetObjectListPaths(mon, path, &bprops); + if (nprops < 0) + goto cleanup; + + for (i = 0; i < nprops; i++) { + if (STREQ(bprops[i]->name, "guest-stats-polling-interval")) { + VIR_DEBUG("Found Balloon Object Path %s", path); + mon->balloonpath = path; + path = NULL; + ret = 0; goto cleanup; } - - /* Type entries that begin with "child<" are a branch that can be - * traversed looking for more entries - */ - if (paths[i]->type && STRPREFIX(paths[i]->type, "child<")) { - if (virAsprintf(&nextpath, "%s/%s", curpath, paths[i]->name) < 0) { - ret = -1; - goto cleanup; - } - ret = qemuMonitorFindBalloonObjectPath(mon, vm, nextpath); - } }

+ + /* If we get here, we found the path, but not the property */ + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Property 'guest-stats-polling-interval' " + "not found on memory balloon driver.")); + cleanup: - for (i = 0; i < npaths; i++) - qemuMonitorJSONListPathFree(paths[i]); - VIR_FREE(paths); - for (j = 0; j < nprops; j++) - qemuMonitorJSONListPathFree(bprops[j]); + for (i = 0; i < nprops; i++) + qemuMonitorJSONListPathFree(bprops[i]); VIR_FREE(bprops); - VIR_FREE(nextpath); + VIR_FREE(path); return ret; }

@@ -1660,7 +1691,7 @@ int qemuMonitorSetMemoryStatsPeriod(qemuMonitorPtr mon, return -1; }

- if (qemuMonitorFindBalloonObjectPath(mon, mon->vm, "/") == 1) { + if (qemuMonitorFindBalloonObjectPath(mon, mon->vm, "/") == 0) {

A bit of a bikeshed, but the "/" could be moved into the called function - I say this only because in patch 3 you call qemuMonitorFindObjectPath with "/". Could probably also just pass 'mon' and deref 'mon->vm' in the called routine. ACK, John

ret = qemuMonitorJSONSetMemoryStatsPeriod(mon, mon->balloonpath, period); }

On 12/11/2014 04:42 PM, Pavel Hrdina wrote:

Moving the struct and friends to qemu_monitor.h because the 'qemuMonitorJSONGetObjectProperty' has usage also for internal libvirt code not only for tests.

These properties are not really needed outside qemu_monitor_json.c if you keep FindObjectPath in _json.c as well and create a wrapper like: qemuMonitorGetVideoRam(*device, *alias, *vram, *ram, *vgamem) Jan

Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_monitor.h | 28 ++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.c | 12 ++++++------ src/qemu/qemu_monitor_json.h | 34 ++-------------------------------- tests/qemumonitorjsontest.c | 10 +++++----- 4 files changed, 41 insertions(+), 43 deletions(-)

diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 21533a4..4918588 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -211,6 +211,34 @@ struct _qemuMonitorCallbacks { qemuMonitorDomainSerialChangeCallback domainSerialChange; };

+/* 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 iv; + long long l; + unsigned int ui; + unsigned long long ul; + double d; + char *str; + } val; +}; + char *qemuMonitorEscapeArg(const char *in); char *qemuMonitorUnescapeArg(const char *in);

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 162579b..7854b9c 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1489,10 +1489,10 @@ qemuMonitorJSONSetMemoryStatsPeriod(qemuMonitorPtr mon, char *balloonpath, int period) { - qemuMonitorJSONObjectProperty prop; + qemuMonitorObjectProperty prop;

/* Set to the value in memballoon (could enable or disable) */ - memset(&prop, 0, sizeof(qemuMonitorJSONObjectProperty)); + memset(&prop, 0, sizeof(qemuMonitorObjectProperty)); prop.type = QEMU_MONITOR_OBJECT_PROPERTY_INT; prop.val.iv = period; if (qemuMonitorJSONSetObjectProperty(mon, balloonpath, @@ -5287,7 +5287,7 @@ void qemuMonitorJSONListPathFree(qemuMonitorJSONListPathPtr paths) int qemuMonitorJSONGetObjectProperty(qemuMonitorPtr mon, const char *path, const char *property, - qemuMonitorJSONObjectPropertyPtr prop) + qemuMonitorObjectPropertyPtr prop) { int ret; virJSONValuePtr cmd; @@ -5317,7 +5317,7 @@ int qemuMonitorJSONGetObjectProperty(qemuMonitorPtr mon, goto cleanup; }

- switch ((qemuMonitorJSONObjectPropertyType) prop->type) { + switch ((qemuMonitorObjectPropertyType) prop->type) { /* Simple cases of boolean, int, long, uint, ulong, double, and string * will receive return value as part of {"return": xxx} statement */ @@ -5378,13 +5378,13 @@ int qemuMonitorJSONGetObjectProperty(qemuMonitorPtr mon, int qemuMonitorJSONSetObjectProperty(qemuMonitorPtr mon, const char *path, const char *property, - qemuMonitorJSONObjectPropertyPtr prop) + qemuMonitorObjectPropertyPtr prop) { int ret = -1; virJSONValuePtr cmd = NULL; virJSONValuePtr reply = NULL;

- switch ((qemuMonitorJSONObjectPropertyType) prop->type) { + switch ((qemuMonitorObjectPropertyType) prop->type) { /* Simple cases of boolean, int, long, uint, ulong, double, and string * will receive return value as part of {"return": xxx} statement */ diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index ae20fb1..b8f14bc 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -388,47 +388,17 @@ int qemuMonitorJSONGetObjectListPaths(qemuMonitorPtr mon,

void qemuMonitorJSONListPathFree(qemuMonitorJSONListPathPtr paths);

-/* ObjectProperty structures and Get/Set API's are public only - * for qemumonitorjsontest - */ -/* Flags for the 'type' field in _qemuMonitorJSONObjectProperty */ -typedef enum { - QEMU_MONITOR_OBJECT_PROPERTY_BOOLEAN=1, - QEMU_MONITOR_OBJECT_PROPERTY_INT, - QEMU_MONITOR_OBJECT_PROPERTY_LONG, - QEMU_MONITOR_OBJECT_PROPERTY_UINT, - QEMU_MONITOR_OBJECT_PROPERTY_ULONG, - QEMU_MONITOR_OBJECT_PROPERTY_DOUBLE, - QEMU_MONITOR_OBJECT_PROPERTY_STRING, - - QEMU_MONITOR_OBJECT_PROPERTY_LAST -} qemuMonitorJSONObjectPropertyType; - -typedef struct _qemuMonitorJSONObjectProperty qemuMonitorJSONObjectProperty; -typedef qemuMonitorJSONObjectProperty *qemuMonitorJSONObjectPropertyPtr; -struct _qemuMonitorJSONObjectProperty { - int type; /* qemuMonitorJSONObjectPropertyType */ - union { - bool b; - int iv; - long long l; - unsigned int ui; - unsigned long long ul; - double d; - char *str; - } val; -};

int qemuMonitorJSONGetObjectProperty(qemuMonitorPtr mon, const char *path, const char *property, - qemuMonitorJSONObjectPropertyPtr prop) + qemuMonitorObjectPropertyPtr prop) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4);

int qemuMonitorJSONSetObjectProperty(qemuMonitorPtr mon, const char *path, const char *property, - qemuMonitorJSONObjectPropertyPtr prop) + qemuMonitorObjectPropertyPtr prop) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4);

int qemuMonitorJSONGetObjectProps(qemuMonitorPtr mon, diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index bd92e63..14cde7c 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -895,7 +895,7 @@ testQemuMonitorJSONGetObjectProperty(const void *data) virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data; qemuMonitorTestPtr test = qemuMonitorTestNewSimple(true, xmlopt); int ret = -1; - qemuMonitorJSONObjectProperty prop; + qemuMonitorObjectProperty prop;

if (!test) return -1; @@ -905,7 +905,7 @@ testQemuMonitorJSONGetObjectProperty(const void *data) goto cleanup;

/* Present with path and property */ - memset(&prop, 0, sizeof(qemuMonitorJSONObjectProperty)); + memset(&prop, 0, sizeof(qemuMonitorObjectProperty)); prop.type = QEMU_MONITOR_OBJECT_PROPERTY_BOOLEAN; if (qemuMonitorJSONGetObjectProperty(qemuMonitorTestGetMonitor(test), "/machine/i440fx", @@ -938,7 +938,7 @@ testQemuMonitorJSONSetObjectProperty(const void *data) virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data; qemuMonitorTestPtr test = qemuMonitorTestNewSimple(true, xmlopt); int ret = -1; - qemuMonitorJSONObjectProperty prop; + qemuMonitorObjectProperty prop;

if (!test) return -1; @@ -951,7 +951,7 @@ testQemuMonitorJSONSetObjectProperty(const void *data) goto cleanup;

/* Let's attempt the setting */ - memset(&prop, 0, sizeof(qemuMonitorJSONObjectProperty)); + memset(&prop, 0, sizeof(qemuMonitorObjectProperty)); prop.type = QEMU_MONITOR_OBJECT_PROPERTY_BOOLEAN; prop.val.b = true; if (qemuMonitorJSONSetObjectProperty(qemuMonitorTestGetMonitor(test), @@ -963,7 +963,7 @@ testQemuMonitorJSONSetObjectProperty(const void *data) /* To make sure it worked, fetch the property - if this succeeds then * we didn't hose things */ - memset(&prop, 0, sizeof(qemuMonitorJSONObjectProperty)); + memset(&prop, 0, sizeof(qemuMonitorObjectProperty)); prop.type = QEMU_MONITOR_OBJECT_PROPERTY_BOOLEAN; if (qemuMonitorJSONGetObjectProperty(qemuMonitorTestGetMonitor(test), "/machine/i440fx",

On 12/11/2014 10:42 AM, Pavel Hrdina wrote:

QEMU internally updates the size of video memory if you specify too low

s/you specify/the domain XML had provided a/ ?Or something like that...

memory size or there are some dependencies like for QXL device and its

s/like for/for s/QXL device and its/a QXL devices'

'vgamem' and 'ram' size. We need to know about the changes and store them into the status XML to not break migration or managedsave trough

s/trough/through

different libvirt versions.

Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_monitor.c | 34 +++++++++++++++ src/qemu/qemu_monitor.h | 10 +++++ src/qemu/qemu_process.c | 108 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 152 insertions(+)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index ee4460f..e061e02 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1153,6 +1153,40 @@ qemuMonitorFindBalloonObjectPath(qemuMonitorPtr mon, return ret; }

+ +/** + * For device finds its QOM Object path and after that reads and return + * specified property. If the path cannot be found or the QOM Object doesn't + * have that property it will fail and report appropriate error message. + * + * Returns 0 on success, otherwise -1. + */ +int +qemuMonitorGetDeviceParam(qemuMonitorPtr mon, + const char *device, + const char *name, + qemuMonitorObjectPropertyPtr prop) +{ + char *path = NULL; + + if (qemuMonitorFindObjectPath(mon, "/", device, &path) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to find QOM Object path for device '%s'"), + device); + return -1; + } + + if (qemuMonitorJSONGetObjectProperty(mon, path, name, prop)) {

prop) < 0) right?

+ virReportError(VIR_ERR_INTERNAL_ERROR, + _("QOM Object '%s' has no property '%s'"), device, name); + VIR_FREE(path); + return -1; + }

[1] See below - searches through qemuMonitorFindObjectPath are somewhat expensive (how long is the chain if we have to start at "/" each time), but I assume that the 'prop' returned could be an array and then for each prop<input> you'd be able to fill in the prop<output> rather than make 'n' sequential calls.

+

VIR_FREE(path); (strange my Coverity run didn't pick up on it, but my eyes did). Then I thought - hey why not create two functions - the first returns path and the second uses it as many times as there are properties to look up instead of a prop array...

+ return 0; +} + + int qemuMonitorHMPCommandWithFd(qemuMonitorPtr mon, const char *cmd, int scm_fd, diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 4918588..dc74964 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -271,6 +271,12 @@ virJSONValuePtr qemuMonitorGetOptions(qemuMonitorPtr mon) ATTRIBUTE_NONNULL(1); void qemuMonitorSetOptions(qemuMonitorPtr mon, virJSONValuePtr options) ATTRIBUTE_NONNULL(1); +int qemuMonitorGetDeviceParam(qemuMonitorPtr mon, + const char *device, + const char *name, + qemuMonitorObjectPropertyPtr prop) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) + ATTRIBUTE_NONNULL(4); int qemuMonitorHMPCommandWithFd(qemuMonitorPtr mon, const char *cmd, int scm_fd, @@ -367,6 +373,10 @@ int qemuMonitorGetCPUInfo(qemuMonitorPtr mon, int **pids); int qemuMonitorGetVirtType(qemuMonitorPtr mon, int *virtType); +int qemuMonitorGetVideoDeviceParam(qemuMonitorPtr mon, + const char *device, + const char *name, + qemuMonitorObjectPropertyPtr prop); int qemuMonitorGetBalloonInfo(qemuMonitorPtr mon, unsigned long long *currmem); int qemuMonitorGetMemoryStats(qemuMonitorPtr mon, diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index ab4df9b..551d48c 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3037,6 +3037,110 @@ qemuProcessCleanupChardevDevice(virDomainDefPtr def ATTRIBUTE_UNUSED, }

+/** + * Loads and update video memory size for video devices according to QEMU + * process as the QEMU will silently update the values that we pass to QEMU + * through command line. We need to load these updated values and store them + * into the status XML. + *

So in essence, we are searching the object tree's for link<VGA>, link<qxl-vga>/link<qxl>, or link<vmware-svga> for the specific properties listed. Just something to add to this entry so it's easier for the next person to understand what's going on if they choose to read the comments...

+ * We will fail if for some reason the values cannot be loaded from QEMU because + * its mandatory to get the correct video memory size to status XML to not break + * migration. + */ +static int +qemuProcessUpdateVideoRamSize(virQEMUDriverPtr driver, + virDomainObjPtr vm, + int asyncJob) +{ + ssize_t i; + qemuDomainObjPrivatePtr priv = vm->privateData; + virDomainVideoDefPtr video = NULL; + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + qemuMonitorObjectProperty prop = { + QEMU_MONITOR_OBJECT_PROPERTY_ULONG, + {0} + }; + + if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) + return -1; + + for (i = 0; i < vm->def->nvideos; i++) { + video = vm->def->videos[i]; + switch (video->type) { + case VIR_DOMAIN_VIDEO_TYPE_VGA: + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VGA_VGAMEM)) { + if (qemuMonitorGetDeviceParam(priv->mon, + "VGA", + "vgamem_mb", + &prop) < 0) { + goto error; + } + + video->vram = prop.val.ul * 1024; + } + break; + case VIR_DOMAIN_VIDEO_TYPE_QXL: + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_QXL_VGA_VGAMEM)) { + if (qemuMonitorGetDeviceParam(priv->mon, + video->primary ? "qxl-vga" : "qxl", + "vram_size", + &prop) < 0) { + goto error; + } + + video->vram = prop.val.ul / 1024; + if (qemuMonitorGetDeviceParam(priv->mon, + video->primary ? "qxl-vga" : "qxl", + "ram_size", + &prop) < 0) { + goto error; + } + + video->ram = prop.val.ul / 1024; + if (qemuMonitorGetDeviceParam(priv->mon, + video->primary ? "qxl-vga" : "qxl", + "vgamem_mb", + &prop) < 0) { + goto error; + } + + video->vgamem = prop.val.ul * 1024;

Three trips through qemuMonitorFindObjectPath is "expensive"... This is where it might be worthwhile to generate an array of prop's: prop[0] = "vram_size" prop[1] = "ram_size" prop[2] = "vgamem_mb" Which could then be used in the qemuMonitorGetDeviceParam*s*() to return each data point desired. or return path from the first call and make 'n' calls using that path to get properties. Although I like the idea of an array better.

+ } + break; + case VIR_DOMAIN_VIDEO_TYPE_VMVGA: + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VMWARE_SVGA_VGAMEM)) { + + if (qemuMonitorGetDeviceParam(priv->mon, + "vmware-svga", + "vgamem_mb", + &prop) < 0) { + goto error; + } + + video->vram = prop.val.ul * 1024; + } + break; + case VIR_DOMAIN_VIDEO_TYPE_CIRRUS: + case VIR_DOMAIN_VIDEO_TYPE_XEN: + case VIR_DOMAIN_VIDEO_TYPE_VBOX: + case VIR_DOMAIN_VIDEO_TYPE_LAST: + break; + } + }

We could get here without changing anything.... Either we don't have the specific CAP available, the value is the same as we already have stored, or we fall into the last group of case's. In any of those cases the subsequent save is only ripe for unnecessary errors... Maybe it's best to have some sort of boolean that indicates a value was changed.

+ + qemuDomainObjExitMonitor(driver, vm); + + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0)

There's a few places in qemu_driver which do a VIR_WARN("Failed to save status on vm %s", vm->def->name); if this fails... Not sure if you want to follow that here too.

+ return -1; + + return 0; + + error: + qemuDomainObjExitMonitor(driver, vm); + return -1; +} + + struct qemuProcessHookData { virConnectPtr conn; virDomainObjPtr vm; @@ -4787,6 +4891,10 @@ int qemuProcessStart(virConnectPtr conn, } qemuDomainObjExitMonitor(driver, vm);

+ VIR_DEBUG("Detecting actual memory size for video device"); + if (qemuProcessUpdateVideoRamSize(driver, vm, asyncJob) < 0) + goto cleanup; +

This only works if mon->json, correct? So if we don't have that, then we're dead in the water? Currently we're not dead in the water, incorrect values perhaps displayed/used, but we're not failing to start up. Is that what we is desired? I think overall it looks ok, there's a couple of adjustments though and things to understand better before an ACK though... John

if (!(flags & VIR_QEMU_PROCESS_START_PAUSED)) { VIR_DEBUG("Starting domain CPUs"); /* Allow the CPUS to start executing */