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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list