On 2/6/24 10:34, Purna Pavan Chandra Aekkaladevi wrote:
> From: Purna Pavan Chandra Aekkaladevi <paekkaladevi@xxxxxxxxxxxxx>
>
> save, managedsave and restore is supported for domains without any
> network, hostdev config defined. The `path` input to the save command
> should be a directory path since cloud-hypervisor expects directory path.
>
> Signed-off-by: Purna Pavan Chandra Aekkaladevi <paekkaladevi@xxxxxxxxxxxxxxxxxxx>
> ---
> src/ch/ch_conf.c | 6 +
> src/ch/ch_conf.h | 12 ++
> src/ch/ch_driver.c | 511 +++++++++++++++++++++++++++++++++++++++++++-
> src/ch/ch_monitor.c | 97 ++++++++-
> src/ch/ch_monitor.h | 6 +-
> src/ch/ch_process.c | 101 +++++++--
> src/ch/ch_process.h | 4 +
> 7 files changed, 715 insertions(+), 22 deletions(-)
There are multiple APIs implemented here which makes it unnecessarily
hard to do a proper review. Can you please split this huge patch into
smaller ones?
>
> diff --git a/src/ch/ch_conf.c b/src/ch/ch_conf.c
> index f421af5121..c109721a83 100644
> --- a/src/ch/ch_conf.c
> +++ b/src/ch/ch_conf.c
> @@ -139,10 +139,12 @@ virCHDriverConfigNew(bool privileged)
> if (privileged) {
> cfg->logDir = g_strdup_printf("%s/log/libvirt/ch", LOCALSTATEDIR);
> cfg->stateDir = g_strdup_printf("%s/libvirt/ch", RUNSTATEDIR);
> + cfg->saveDir = g_strdup_printf("%s/lib/libvirt/ch/save", LOCALSTATEDIR);
>
> } else {
> g_autofree char *rundir = NULL;
> g_autofree char *cachedir = NULL;
> + g_autofree char *configbasedir = NULL;
>
> cachedir = virGetUserCacheDirectory();
>
> @@ -150,6 +152,9 @@ virCHDriverConfigNew(bool privileged)
>
> rundir = virGetUserRuntimeDirectory();
> cfg->stateDir = g_strdup_printf("%s/ch/run", rundir);
> +
> + configbasedir = virGetUserConfigDirectory();
> + cfg->saveDir = g_strdup_printf("%s/ch/save", configbasedir);
> }
>
> return cfg;
> @@ -166,6 +171,7 @@ virCHDriverConfigDispose(void *obj)
> {
> virCHDriverConfig *cfg = obj;
>
> + g_free(cfg->saveDir);
> g_free(cfg->stateDir);
> g_free(cfg->logDir);
> }
> diff --git a/src/ch/ch_conf.h b/src/ch/ch_conf.h
> index 579eca894e..a77cad7a2a 100644
> --- a/src/ch/ch_conf.h
> +++ b/src/ch/ch_conf.h
> @@ -37,6 +37,7 @@ struct _virCHDriverConfig {
>
> char *stateDir;
> char *logDir;
> + char *saveDir;
>
> int cgroupControllers;
>
> @@ -81,6 +82,17 @@ struct _virCHDriver
> ebtablesContext *ebtables;
> };
>
> +#define CH_SAVE_MAGIC "libvirt-xml\n \0 \r"
> +#define CH_SAVE_XML "libvirt-save.xml"
> +
> +typedef struct _CHSaveXMLHeader CHSaveXMLHeader;
> +struct _CHSaveXMLHeader {
> + char magic[sizeof(CH_SAVE_MAGIC)-1];
> + uint32_t xmlLen;
> + /* 20 bytes used, pad up to 64 bytes */
> + uint32_t unused[11];
> +};
> +
> virCaps *virCHDriverCapsInit(void);
> virCaps *virCHDriverGetCapabilities(virCHDriver *driver,
> bool refresh);
> diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c
> index 96de5044ac..4413abfa79 100644
> --- a/src/ch/ch_driver.c
> +++ b/src/ch/ch_driver.c
> @@ -20,6 +20,8 @@
>
> #include <config.h>
>
> +#include <fcntl.h>
> +
> #include "ch_capabilities.h"
> #include "ch_conf.h"
> #include "ch_domain.h"
> @@ -32,6 +34,7 @@
> #include "viraccessapicheck.h"
> #include "virchrdev.h"
> #include "virerror.h"
> +#include "virfile.h"
> #include "virlog.h"
> #include "virobject.h"
> #include "virtypedparam.h"
> @@ -176,6 +179,13 @@ static char *chConnectGetCapabilities(virConnectPtr conn)
> return xml;
> }
>
> +static char *
> +chDomainManagedSavePath(virCHDriver *driver, virDomainObj *vm)
> +{
> + g_autoptr(virCHDriverConfig) cfg = virCHDriverGetConfig(driver);
> + return g_strdup_printf("%s/%s.save", cfg->saveDir, vm->def->name);
> +}
> +
> /**
> * chDomainCreateXML:
> * @conn: pointer to connection
> @@ -196,6 +206,7 @@ chDomainCreateXML(virConnectPtr conn,
> virDomainObj *vm = NULL;
> virDomainPtr dom = NULL;
> unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE;
> + g_autofree char *managed_save_path = NULL;
>
> virCheckFlags(VIR_DOMAIN_START_VALIDATE, NULL);
>
> @@ -218,6 +229,15 @@ chDomainCreateXML(virConnectPtr conn,
> NULL)))
> goto cleanup;
>
> + /* cleanup if there's any stale managedsave dir */
> + managed_save_path = chDomainManagedSavePath(driver, vm);
> + if (virFileDeleteTree(managed_save_path) < 0) {
> + virReportSystemError(errno,
> + _("Failed to cleanup stale managed save dir '%1$s'"),
> + managed_save_path);
> + goto cleanup;
> + }
I don't think we should do this. Users can start a transient domain with
the same name and UUID as a defined domain, but a different XML. If we
did this then previously saved data is lost (and domain disks are
possibly left in an inconsistent state as the domain's memory would be
removed here).
> +
> if (virDomainObjBeginJob(vm, VIR_JOB_MODIFY) < 0)
> goto cleanup;
>
> @@ -242,6 +262,8 @@ chDomainCreateWithFlags(virDomainPtr dom, unsigned int flags)
> {
> virCHDriver *driver = dom->conn->privateData;
> virDomainObj *vm;
> + virCHDomainObjPrivate *priv;
> + g_autofree char *managed_save_path = NULL;
> int ret = -1;
>
> virCheckFlags(0, -1);
> @@ -255,8 +277,33 @@ chDomainCreateWithFlags(virDomainPtr dom, unsigned int flags)
> if (virDomainObjBeginJob(vm, VIR_JOB_MODIFY) < 0)
> goto cleanup;
>
> - ret = virCHProcessStart(driver, vm, VIR_DOMAIN_RUNNING_BOOTED);
> + if (vm->hasManagedSave) {
> + priv = vm->privateData;
> + managed_save_path = chDomainManagedSavePath(driver, vm);
> + if (virCHProcessStartRestore(driver, vm, managed_save_path) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("failed to restore domain from managed save"));
> + goto endjob;
> + }
> + if (virCHMonitorResumeVM(priv->monitor) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("failed to resume domain after restore from managed save"));
> + goto endjob;
> + }
> + virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, VIR_DOMAIN_RUNNING_RESTORED);
> + if (virFileDeleteTree(managed_save_path) < 0) {
> + virReportSystemError(errno,
> + _("Failed to remove managed save path '%1$s'"),
> + managed_save_path);
> + goto endjob;
> + }
> + vm->hasManagedSave = false;
> + ret = 0;
> + } else {
> + ret = virCHProcessStart(driver, vm, VIR_DOMAIN_RUNNING_BOOTED);
> + }
>
> + endjob:
> virDomainObjEndJob(vm);
>
> cleanup:
> @@ -277,6 +324,7 @@ chDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags)
> g_autoptr(virDomainDef) vmdef = NULL;
> virDomainObj *vm = NULL;
> virDomainPtr dom = NULL;
> + g_autofree char *managed_save_path = NULL;
> unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE;
>
> virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL);
> @@ -299,6 +347,15 @@ chDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags)
> 0, NULL)))
> goto cleanup;
>
> + /* cleanup if there's any stale managedsave dir */
> + managed_save_path = chDomainManagedSavePath(driver, vm);
> + if (virFileDeleteTree(managed_save_path) < 0) {
> + virReportSystemError(errno,
> + _("Failed to cleanup stale managed save dir '%1$s'"),
> + managed_save_path);
> + goto cleanup;
> + }
> +
> vm->persistent = 1;
>
> dom = virGetDomain(conn, vm->def->name, vm->def->uuid, vm->def->id);
> @@ -621,6 +678,449 @@ chDomainDestroy(virDomainPtr dom)
> return chDomainDestroyFlags(dom, 0);
> }
>
> +static int
> +chDomainSaveAdditionalValidation(virDomainDef *vmdef)
> +{
> + /*
> + SAVE and RESTORE are functional only without any networking and
> + device passthrough configuration
> + */
> + if (vmdef->nnets > 0) {
> + virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> + _("cannot save domain with network interfaces"));
> + return -1;
> + }
> + if (vmdef->nhostdevs > 0) {
> + virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> + _("cannot save domain with host devices"));
> + return -1;
> + }
> + return 0;
> +}
> +
> +/**
> + * chDoDomainSave:
> + * @driver: pointer to driver structure
> + * @vm: pointer to virtual machine structure. Must be locked before invocation.
> + * @to_dir: directory path (CH needs directory input) to save the domain
Ah, so it stores multiple files inside of the directory? Well, this
calls for update of virDomainSave() documentation then. I don't think
it's worth going through the hassle of packing everything under that dir
into a single file (e.g. via 'tar').
This is where I stop my review. Sorry for letting this slip.
Michal
_______________________________________________
Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
