[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] implement managedsave in libvirt xen legacy driver



Bamvor Jian Zhang wrote:
> Implement the domainManagedSave, domainHasManagedSaveImage, and
> domainManagedSaveRemove functions in the libvirt legacy xen driver.
>   

Cool, thanks for the patch!

In case others are interested, one motivation for adding this
functionality in the legacy xen driver is to avoid xen-only hacks in the
nova libvirt driver.

> domainHasManagedSaveImage check the managedsave image from filesystem
> everytime. This is different from qemu and libxl driver. In qemu or
> libxl driver, there is a hasManagesSave flags in virDomainObjPtr which
> is not used in xen legacy driver. This flag could not add into xen
> driver ptr either, because the driver ptr will be release at the end of
> every libvirt api calls. Meanwhile, AFAIK, xen store all the flags in
> xen not in libvirt xen driver. There is no need to add this flags in xen.
>   

I think checking the filesystem is the only way to go since xend doesn't
have the notion of managedsave.  Do others see any issues with this
approach?

> ---
> i have test the following case for this patch:
> 1), "virsh managedsave" save domain to /var/lib/xen/save/domain_name.save.
> call xenUnifiedDomainManagedSave.
> 2), "virsh start": if managedsave image is exist, it should be boot from
> managed save image.
> call xenUnifiedDomainHasManagedSaveImage.
> 3), "virsh start --force-boot": fresh boot, delete the managed save image
> if exists.
> call xenUnifiedDomainHasManagedSaveImage,
> xenUnifiedDomainManagedSaveRemove.
> 4), "virsh managedsave-remove": remove managed save image.
> call xenUnifiedDomainManagedSaveRemove
> 5), "virsh undefine": undefine the domain, it will fail if mananed save
> image exist.
> call xenUnifiedDomainHasManagedSaveImage.
> 6), "virsh undefine --managed-save": undefine the domain, and remove
> mananed save image.
> call xenUnifiedDomainHasManagedSaveImage and
> xenUnifiedDomainManagedSaveRemove.
> 7), "virsh list --all --with-managed-save". list domain if managed save
> image exist. got nothing if non-exists.
> call xenUnifiedDomainHasManagedSaveImage.
> 8), "virsh list --all --without-managed-save". do not list the domain if
> managed save image exist.
> call xenUnifiedDomainHasManagedSaveImage.
>   

Thanks for including your test cases.  I've tried these on your patch as
well and looks good!

>  src/xen/xen_driver.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  src/xen/xen_driver.h |   2 +
>  2 files changed, 110 insertions(+), 1 deletion(-)
>
> diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c
> index 5a40757..0b2418d 100644
> --- a/src/xen/xen_driver.c
> +++ b/src/xen/xen_driver.c
> @@ -67,6 +67,7 @@
>  #include "nodeinfo.h"
>  
>  #define VIR_FROM_THIS VIR_FROM_XEN
> +#define XEN_SAVE_DIR LOCALSTATEDIR "/lib/libvirt/xen/save"
>   

#include "configmake.h" is needed for LOCALSTATEDIR

>  
>  static int
>  xenUnifiedNodeGetInfo(virConnectPtr conn, virNodeInfoPtr info);
> @@ -267,6 +268,7 @@ xenUnifiedOpen(virConnectPtr conn, virConnectAuthPtr auth, unsigned int flags)
>  {
>      int i, ret = VIR_DRV_OPEN_DECLINED;
>      xenUnifiedPrivatePtr priv;
> +    char ebuf[1024];
>  
>  #ifdef __sun
>      /*
> @@ -406,6 +408,17 @@ xenUnifiedOpen(virConnectPtr conn, virConnectAuthPtr auth, unsigned int flags)
>      }
>  #endif
>  
> +    if (virAsprintf(&priv->saveDir, "%s", XEN_SAVE_DIR) == -1) {
> +        virReportOOMError();
> +        goto fail;
> +    }
> +
> +    if (virFileMakePath(priv->saveDir) < 0) {
> +        VIR_ERROR(_("Failed to create save dir '%s': %s"), priv->saveDir,
> +                  virStrerror(errno, ebuf, sizeof(ebuf)));
> +        goto fail;
> +    }
> +
>      return VIR_DRV_OPEN_SUCCESS;
>  
>  fail:
> @@ -437,6 +450,7 @@ xenUnifiedClose(virConnectPtr conn)
>          if (priv->opened[i])
>              drivers[i]->xenClose(conn);
>  
> +    VIR_FREE(priv->saveDir);
>      virMutexDestroy(&priv->lock);
>      VIR_FREE(conn->privateData);
>  
> @@ -1080,6 +1094,79 @@ xenUnifiedDomainSave(virDomainPtr dom, const char *to)
>      return xenUnifiedDomainSaveFlags(dom, to, NULL, 0);
>  }
>  
> +static char *
> +xenUnifiedDomainManagedSavePath(xenUnifiedPrivatePtr priv, virDomainPtr dom)
> +{
> +    char *ret;
> +
> +    if (virAsprintf(&ret, "%s/%s.save", priv->saveDir, dom->name) < 0) {
> +        virReportOOMError();
> +        return NULL;
> +    }
> +
> +    VIR_DEBUG("managed save image: %s", ret);
> +    return ret;
> +}
> +
> +static int
> +xenUnifiedDomainManagedSave(virDomainPtr dom, unsigned int flags)
> +{
> +    GET_PRIVATE(dom->conn);
> +    char *name;
> +    int ret = -1;
> +
> +    virCheckFlags(0, -1);
> +
> +    name = xenUnifiedDomainManagedSavePath(priv, dom);
> +    if (!name)
> +        goto cleanup;
> +
> +    if (priv->opened[XEN_UNIFIED_XEND_OFFSET])
> +        ret = xenDaemonDomainSave(dom, name);
> +
> +cleanup:
> +    VIR_FREE(name);
> +    return ret;
> +}
> +
> +static int
> +xenUnifiedDomainHasManagedSaveImage(virDomainPtr dom, unsigned int flags)
> +{
> +    GET_PRIVATE(dom->conn);
> +    char *name;
> +    int ret = -1;
> +
> +    virCheckFlags(0, -1);
> +
> +    name = xenUnifiedDomainManagedSavePath(priv, dom);
> +    if (!name)
> +        return ret;
> +
> +    ret = virFileExists(name);
> +    VIR_FREE(name);
> +    return ret;
> +}
> +
> +static int
> +xenUnifiedDomainManagedSaveRemove(virDomainPtr dom, unsigned int flags)
> +{
> +    GET_PRIVATE(dom->conn);
> +    char *name;
> +    int ret = -1;
> +
> +    virCheckFlags(0, -1);
> +
> +    name = xenUnifiedDomainManagedSavePath(priv, dom);
> +    if (!name)
> +        goto cleanup;
>   

I think we can do away with the cleanup label and simply return ret here.

> +
> +    ret = unlink(name);
> +    VIR_FREE(name);
> +
> +cleanup:
> +    return ret;
> +}
> +
>  static int
>  xenUnifiedDomainRestoreFlags(virConnectPtr conn, const char *from,
>                               const char *dxml, unsigned int flags)
> @@ -1504,16 +1591,33 @@ static int
>  xenUnifiedDomainCreateWithFlags(virDomainPtr dom, unsigned int flags)
>  {
>      GET_PRIVATE(dom->conn);
> +    char *name;
>      int i;
> +    int ret = -1;
>  
>      virCheckFlags(0, -1);
>  
> +    name = xenUnifiedDomainManagedSavePath(priv, dom);
> +    if (!name)
> +        goto cleanup;
> +
> +    if (virFileExists(name)) {
> +        if (priv->opened[XEN_UNIFIED_XEND_OFFSET]) {
> +            ret = xenDaemonDomainRestore(dom->conn, name);
> +            if (ret == 0)
> +                unlink(name);
> +        }
> +        VIR_FREE(name);
> +        goto cleanup;
> +    }
>   

name is leaked here if virFileExists() fails.  You can initialize name
to NULL and then unconditionally free it in cleanup.

> +
>      for (i = 0; i < XEN_UNIFIED_NR_DRIVERS; ++i)
>          if (priv->opened[i] && drivers[i]->xenDomainCreate &&
>              drivers[i]->xenDomainCreate(dom) == 0)
>              return 0;
>  
> -    return -1;
> +cleanup:
> +    return ret;
>  }
>  
>  static int
> @@ -2218,6 +2322,9 @@ static virDriver xenUnifiedDriver = {
>      .domainGetState = xenUnifiedDomainGetState, /* 0.9.2 */
>      .domainSave = xenUnifiedDomainSave, /* 0.0.3 */
>      .domainSaveFlags = xenUnifiedDomainSaveFlags, /* 0.9.4 */
> +    .domainManagedSave = xenUnifiedDomainManagedSave, /* 1.0.1 */
> +    .domainHasManagedSaveImage = xenUnifiedDomainHasManagedSaveImage, /* 1.0.1 */
> +    .domainManagedSaveRemove = xenUnifiedDomainManagedSaveRemove, /* 1.0.1 */
>      .domainRestore = xenUnifiedDomainRestore, /* 0.0.3 */
>      .domainRestoreFlags = xenUnifiedDomainRestoreFlags, /* 0.9.4 */
>      .domainCoreDump = xenUnifiedDomainCoreDump, /* 0.1.9 */
> diff --git a/src/xen/xen_driver.h b/src/xen/xen_driver.h
> index b3fbcff..078980e 100644
> --- a/src/xen/xen_driver.h
> +++ b/src/xen/xen_driver.h
> @@ -200,6 +200,8 @@ struct _xenUnifiedPrivate {
>      /* Location of config files, either /etc
>       * or /var/lib/xen */
>      const char *configDir;
> +    /* Location of managed save dir, default /var/lib/libvirt/xen/save */
> +    char *saveDir;
>  
>  # if WITH_XEN_INOTIFY
>      /* The inotify fd */
>   

With exception of the few nits, looks good!  I would like to hear what
others think about checking existence of the managed save image on each
invocation of domainHasManagedSaveImage().

Thanks,
Jim


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]