On 2/6/24 10:34, Purna Pavan Chandra Aekkaladevi wrote:
From: Purna Pavan Chandra Aekkaladevi
<paekkaladevi(a)microsoft.com>
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(a)linux.microsoft.com>
---
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