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@xxxxxxxxxx +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list