[libvirt] [PATCH] add screendump async to qemu

[Eric Blake]

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 at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120306/af3ecd94/attachment-0001.sig>