11:40 a.m.
On 03/06/2012 04:02 AM, Alon Levy wrote:
RHBZ: 800338
Adds a new capability to qemu, QEMU_CAPS_SCREENDUMP_ASYNC, available if
the qmp command "screendump-async" exists.
If that cap exists qemuDomainScreenshot uses it. The implementation
consists of a hash from filename to struct holding the stream and
temporary fd. The fd is closed and the stream is written to (in reverse
order) by the completion callback, qemuProcessScreenshotComplete.
Note: in qemuDomainScreenshot I don't check for an existing entry in the
screenshots hash table because we the key is a temporary filename,
produced by mkstemp, and it's only unlinked at
qemuProcessScreenshotComplete.
For testing you need to apply the following patches (they are still
pending review on qemu-devel):
http://patchwork.ozlabs.org/patch/144706/
http://patchwork.ozlabs.org/patch/144705/
http://patchwork.ozlabs.org/patch/144704/
Assuming qemu doesn't make any last-minute changes to the naming of the
new command and format of the new event, then this patch looks
reasonable. I'm reluctant to push it upstream until we know for sure
that qemu is committed to the interface, though. And we need a v2 to
fix the bugs below.
+++ b/src/qemu/qemu_domain.c
@@ -181,6 +181,10 @@ qemuDomainObjFreeJob(qemuDomainObjPrivatePtr priv)
ignore_value(virCondDestroy(&priv->job.asyncCond));
}
+static void
+freeScreenshot(void *payload, const void *name ATTRIBUTE_UNUSED) {
+ VIR_FREE(payload);
+}
static void *qemuDomainObjPrivateAlloc(void)
{
@@ -196,6 +200,8 @@ static void *qemuDomainObjPrivateAlloc(void)
goto error;
priv->migMaxBandwidth = QEMU_DOMAIN_DEFAULT_MIG_BANDWIDTH_MAX;
+ priv->screenshots = virHashCreate(QEMU_DOMAIN_SCREENSHOTS_CONCURRENT_MAX,
+ freeScreenshot);
Missing a counterpart virHashFree(priv->screenshots) in
qemuDomainObjPrivateFree.
+++ b/src/qemu/qemu_driver.c
@@ -3135,6 +3135,7 @@ qemuDomainScreenshot(virDomainPtr dom,
int tmp_fd = -1;
char *ret = NULL;
bool unlink_tmp = false;
+ qemuScreenshotAsync *screenshot;
Assign this to NULL...
virCheckFlags(0, NULL);
@@ -3184,9 +3185,34 @@ qemuDomainScreenshot(virDomainPtr dom,
virSecurityManagerSetSavedStateLabel(qemu_driver->securityManager, vm->def,
tmp);
qemuDomainObjEnterMonitor(driver, vm);
- if (qemuMonitorScreendump(priv->mon, tmp) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
- goto endjob;
+ if (qemuCapsGet(priv->qemuCaps, QEMU_CAPS_SCREENDUMP_ASYNC)) {
+ if (virHashSize(priv->screenshots) >=
+ QEMU_DOMAIN_SCREENSHOTS_CONCURRENT_MAX) {
+ qemuReportError(VIR_ERR_INTERNAL_ERROR,
VIR_ERR_OPERATION_INVALID - the failure is transient and related to the
state of the rest of libvirt, and not an internal error.
+ "%s", _("too many ongoing
screenshots"));
+ goto endjob;
+ }
+ if (VIR_ALLOC(screenshot) < 0) {
+ qemuReportError(VIR_ERR_NO_MEMORY, "%s", _("out of
memory"));
virReportOOMError() (it's almost always wrong to directly report
VIR_ERR_NO_MEMORY; the helper function exists for a reason, since it can
bypass some of the malloc's used in direct reporting, for a higher
chance of success at actually reporting the error without tripping up on
additional OOM situations.)
+ goto endjob;
+ }
+ screenshot->fd = tmp_fd;
+ screenshot->filename = tmp;
+ screenshot->stream = st;
+ virHashAddEntry(priv->screenshots, tmp, screenshot);
+ if (qemuMonitorScreendumpAsync(priv->mon, tmp) < 0) {
+ qemuDomainObjExitMonitor(driver, vm);
+ goto endjob;
...and add VIR_FREE(screenshot) somewhere in the endjob label,
otherwise, this failure path will leak memory.
@@ -725,6 +727,16 @@ out:
qemuMonitorEmitBlockJob(mon, device, type, status);
}
+static void qemuMonitorJSONHandleScreenDumpComplete(qemuMonitorPtr mon,
+ virJSONValuePtr data)
+{
+ const char *filename;
+
+ if ((filename = virJSONValueObjectGetString(data, "filename")) == NULL) {
+ VIR_WARN("missing filename in screen dump complete event");
+ }
+ qemuMonitorEmitScreenDumpComplete(mon, filename);
Is it safe to call qemuMonitorEmitScreenDumpComplete with filename of
NULL, or should this be in an else clause?...
+++ b/src/qemu/qemu_process.c
@@ -47,6 +47,7 @@
#include "datatypes.h"
#include "logging.h"
+#include "fdstream.h"
#include "virterror_internal.h"
#include "memory.h"
#include "hooks.h"
@@ -918,6 +919,33 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
}
static int
+qemuProcessScreenshotComplete(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
+ virDomainObjPtr vm,
+ const char *filename)
+{
+ qemuDomainObjPrivatePtr priv = vm->privateData;
+ qemuScreenshotAsyncPtr screenshot;
+ int ret = 0;
+
+ if ((screenshot = virHashLookup(priv->screenshots, filename)) == NULL) {
...It's not safe to lookup NULL, so one of these two functions should
check for NULL filename.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org