[libvirt] [PATCH] Add domainCoreDump to libxl driver

[Jim Fehlig]

Markus Groß wrote:
> For core dumping to work correctly the following patch
> for xen is needed:
> http://lists.xensource.com/archives/html/xen-devel/2011-05/msg01469.html
>
> This patch is in xen-unstable and is considered for backport to
> the xen stable branches. Without this patch the mapped
> memory pages of the pv guest are not unmapped after core-dump.
>
> ---
>  src/libxl/libxl_driver.c |   87 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 87 insertions(+), 0 deletions(-)
>
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index b2cc0e8..75008db 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -1627,6 +1627,92 @@ libxlDomainGetState(virDomainPtr dom,
>  }
>  
>  static int
> +libxlDomainCoreDump(virDomainPtr dom, const char *to, int flags)
> +{
> +    libxlDriverPrivatePtr driver = dom->conn->privateData;
> +    libxlDomainObjPrivatePtr priv;
> +    virDomainObjPtr vm;
> +    virDomainEventPtr event = NULL;
> +    int paused = 0;
> +    int ret = -1;
> +
> +    virCheckFlags(VIR_DUMP_LIVE | VIR_DUMP_CRASH, -1);
> +
> +    libxlDriverLock(driver);
> +    vm = virDomainFindByUUID(&driver->domains, dom->uuid);
> +
> +    if (!vm) {
> +        char uuidstr[VIR_UUID_STRING_BUFLEN];
> +        virUUIDFormat(dom->uuid, uuidstr);
> +        libxlError(VIR_ERR_NO_DOMAIN,
> +                   _("No domain with matching uuid '%s'"), uuidstr);
> +        goto cleanup;
> +    }
> +
> +    if (!virDomainObjIsActive(vm)) {
> +        libxlError(VIR_ERR_OPERATION_INVALID, "%s", _("Domain is not running"));
> +        goto cleanup;
> +    }
> +
> +    priv = vm->privateData;
> +
> +    if (!(flags & VIR_DUMP_LIVE) &&
> +        virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) {
> +        if (libxl_domain_pause(&priv->ctx, dom->id) != 0) {
> +            libxlError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Failed to suspend domain '%d' with libxenlight"),
> +                       dom->id);
>   

I think there could be a little more info in that error message, e.g.
"Before dumping core, failed to suspend ...".

> +            goto cleanup;
> +        }
> +        virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, VIR_DOMAIN_PAUSED_DUMP);
> +        paused = 1;
> +    }
> +
> +    if (libxl_domain_core_dump(&priv->ctx, dom->id, to) != 0) {
> +        libxlError(VIR_ERR_INTERNAL_ERROR,
> +                   _("Failed to dump core of domain '%d' with libxenlight"),
> +                   dom->id);
> +        goto cleanup;
> +    }
>   

If core dumping fails and the domain was paused, it won't be unpaused by
jumping to cleanup.

> +
> +    if (flags & VIR_DUMP_CRASH) {
> +        if (libxlVmReap(driver, vm, 1, VIR_DOMAIN_SHUTOFF_CRASHED) != 0) {
> +            libxlError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Failed to destroy domain '%d'"), dom->id);
> +            goto cleanup;
> +        }
> +
> +        event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED,
> +                                         VIR_DOMAIN_EVENT_STOPPED_CRASHED);
> +
> +    } else if (paused) {
> +        if (libxl_domain_unpause(&priv->ctx, dom->id) != 0) {
> +            libxlError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Failed to resume domain '%d' with libxenlight"),
> +                       dom->id);
>   

Here too more info in the error message, e.g. "After dumping core,
failed to resume ..."

> +            goto cleanup;
> +        }
> +        virDomainObjSetState(vm, VIR_DOMAIN_RUNNING,
> +                             VIR_DOMAIN_RUNNING_UNPAUSED);
> +    }
> +
> +    if ((flags & VIR_DUMP_CRASH) && !vm->persistent) {
> +        virDomainRemoveInactive(&driver->domains, vm);
> +        vm = NULL;
> +    }
> +
> +    ret = 0;
> +
> +cleanup:
> +    if (vm)
> +        virDomainObjUnlock(vm);
> +    if (event)
> +        libxlDomainEventQueue(driver, event);
> +    libxlDriverUnlock(driver);
>   

Do we need to have the driver locked during the core dump?  Most of the
driver functions use this pattern

    libxlDriverLock(driver);
    vm = virDomainFindByUUID(&driver->domains, dom->uuid);
    libxlDriverUnlock(driver);

Regards,
Jim

> +    return ret;
> +}
> +
> +static int
>  libxlDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus,
>                           unsigned int flags)
>  {
> @@ -2722,6 +2808,7 @@ static virDriver libxlDriver = {
>      .domainSetMemoryFlags = libxlDomainSetMemoryFlags, /* 0.9.0 */
>      .domainGetInfo = libxlDomainGetInfo, /* 0.9.0 */
>      .domainGetState = libxlDomainGetState, /* 0.9.2 */
> +    .domainCoreDump = libxlDomainCoreDump, /* 0.9.2 */
>      .domainSetVcpus = libxlDomainSetVcpus, /* 0.9.0 */
>      .domainSetVcpusFlags = libxlDomainSetVcpusFlags, /* 0.9.0 */
>      .domainGetVcpusFlags = libxlDomainGetVcpusFlags, /* 0.9.0 */
>