Re: [libvirt] [PATCH 04/11] Convert Xen domain managed save driver methods to use virDomainDefPtr

From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; Introduce use of a virDomainDefPtr in the domain save APIs to simplify introduction of ACL security checks. The virDomainPtr cannot be safely used, since the app may have supplied mis-matching name/uuid/id fields. eg the name points to domain X, while the uuid points to domain Y. Resolving the virDomainPtr to a virDomainDefPtr ensures a consistent name/uuid/id set. Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com&gt; --- src/xen/xen_driver.c | 72 ++++++++++++++++++++++++++++++++++++------------- src/xen/xend_internal.c | 23 +++++++++------- src/xen/xend_internal.h | 7 +++-- src/xen/xm_internal.c | 25 ++++++++--------- src/xen/xm_internal.h | 3 ++- 5 files changed, 86 insertions(+), 44 deletions(-) diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index 68a86b7..89b038c 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -1038,14 +1038,25 @@ static int xenUnifiedDomainSaveFlags(virDomainPtr dom, const char *to, const char *dxml, unsigned int flags) { + int ret = -1; + virDomainDefPtr def; + virCheckFlags(0, -1); + if (dxml) { virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", _("xml modification unsupported")); return -1; } - return xenDaemonDomainSave(dom, to); + if (!(def = xenGetDomainDefForDom(dom))) + goto cleanup; + + ret = xenDaemonDomainSave(dom->conn, def, to); + +cleanup: + virDomainDefFree(def); + return ret; } static int @@ -1055,11 +1066,12 @@ xenUnifiedDomainSave(virDomainPtr dom, const char *to) } static char * -xenUnifiedDomainManagedSavePath(xenUnifiedPrivatePtr priv, virDomainPtr dom) +xenUnifiedDomainManagedSavePath(xenUnifiedPrivatePtr priv, + virDomainDefPtr def)

{ char *ret; - if (virAsprintf(&ret, "%s/%s.save", priv->saveDir, dom->name) < 0) { + if (virAsprintf(&ret, "%s/%s.save", priv->saveDir, def->name) < 0) { virReportOOMError(); return NULL; } @@ -1072,19 +1084,23 @@ static int xenUnifiedDomainManagedSave(virDomainPtr dom, unsigned int flags) { xenUnifiedPrivatePtr priv = dom->conn->privateData; - char *name; + char *name = NULL; + virDomainDefPtr def = NULL; int ret = -1; virCheckFlags(0, -1); - name = xenUnifiedDomainManagedSavePath(priv, dom); - if (!name) + if (!(def = xenGetDomainDefForDom(dom))) + goto cleanup; + + if (!(name = xenUnifiedDomainManagedSavePath(priv, def))) goto cleanup; - ret = xenDaemonDomainSave(dom, name); + ret = xenDaemonDomainSave(dom->conn, def, name); cleanup: VIR_FREE(name); + virDomainDefFree(def); return ret; } @@ -1092,17 +1108,23 @@ static int xenUnifiedDomainHasManagedSaveImage(virDomainPtr dom, unsigned int flags) { xenUnifiedPrivatePtr priv = dom->conn->privateData; - char *name; + char *name = NULL; + virDomainDefPtr def = NULL; int ret = -1; virCheckFlags(0, -1); - name = xenUnifiedDomainManagedSavePath(priv, dom); - if (!name) - return ret; + if (!(def = xenGetDomainDefForDom(dom))) + goto cleanup; + + if (!(name = xenUnifiedDomainManagedSavePath(priv, def))) + goto cleanup; ret = virFileExists(name); + +cleanup: VIR_FREE(name); + virDomainDefFree(def); return ret; } @@ -1110,16 +1132,21 @@ static int xenUnifiedDomainManagedSaveRemove(virDomainPtr dom, unsigned int flags) { xenUnifiedPrivatePtr priv = dom->conn->privateData; - char *name; + char *name = NULL; + virDomainDefPtr def = NULL; int ret = -1; virCheckFlags(0, -1); - name = xenUnifiedDomainManagedSavePath(priv, dom); - if (!name) - return ret; + if (!(def = xenGetDomainDefForDom(dom))) + goto cleanup; + + if (!(name = xenUnifiedDomainManagedSavePath(priv, def))) + goto cleanup; ret = unlink(name); + +cleanup: VIR_FREE(name); return ret; } @@ -1496,12 +1523,15 @@ xenUnifiedDomainCreateWithFlags(virDomainPtr dom, unsigned int flags) { xenUnifiedPrivatePtr priv = dom->conn->privateData; int ret = -1; + virDomainDefPtr def = NULL; char *name = NULL; virCheckFlags(0, -1); - name = xenUnifiedDomainManagedSavePath(priv, dom); - if (!name) + if (!(def = xenGetDomainDefForDom(dom))) + goto cleanup; + + if (!(name = xenUnifiedDomainManagedSavePath(priv, def))) goto cleanup; if (virFileExists(name)) { @@ -1512,11 +1542,15 @@ xenUnifiedDomainCreateWithFlags(virDomainPtr dom, unsigned int flags) } if (priv->xendConfigVersion < XEND_CONFIG_VERSION_3_0_4) - ret = xenXMDomainCreate(dom); + ret = xenXMDomainCreate(dom->conn, def); else - ret = xenDaemonDomainCreate(dom); + ret = xenDaemonDomainCreate(dom->conn, def); + + if (ret >= 0) + dom->id = def->id; cleanup: + virDomainDefFree(def); VIR_FREE(name); return ret; } diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index c10567c..9555b33 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -1413,22 +1413,24 @@ xenDaemonDomainGetOSType(virConnectPtr conn, * Returns 0 in case of success, -1 (with errno) in case of error. */ int -xenDaemonDomainSave(virDomainPtr domain, const char *filename) +xenDaemonDomainSave(virConnectPtr conn, + virDomainDefPtr def, + const char *filename) { - if (domain->id < 0) { + if (def->id < 0) { virReportError(VIR_ERR_OPERATION_INVALID, - _("Domain %s isn't running."), domain->name); + _("Domain %s isn't running."), def->name); return -1; } /* We can't save the state of Domain-0, that would mean stopping it too */ - if (domain->id == 0) { + if (def->id == 0) { virReportError(VIR_ERR_INVALID_ARG, "%s", _("Cannot save host domain")); return -1; } - return xend_op(domain->conn, domain->name, "op", "save", "file", filename, NULL); + return xend_op(conn, def->name, "op", "save", "file", filename, NULL); } /** @@ -2862,17 +2864,18 @@ xenDaemonDomainDefineXML(virConnectPtr conn, const char *xmlDesc) return NULL; } int -xenDaemonDomainCreate(virDomainPtr domain) +xenDaemonDomainCreate(virConnectPtr conn, + virDomainDefPtr def)

{ int ret; - ret = xend_op(domain->conn, domain->name, "op", "start", NULL); + ret = xend_op(conn, def->name, "op", "start", NULL); if (ret == 0) { - int id = xenDaemonDomainLookupByName_ids(domain->conn, domain->name, - domain->uuid); + int id = xenDaemonDomainLookupByName_ids(conn, def->name, + def->uuid);

if (id > 0) - domain->id = id; + def->id = id; } return ret; diff --git a/src/xen/xend_internal.h b/src/xen/xend_internal.h index 87e0a0f..01d321f 100644 --- a/src/xen/xend_internal.h +++ b/src/xen/xend_internal.h @@ -92,7 +92,9 @@ int xenDaemonDomainResume(virConnectPtr conn, virDomainDefPtr def); int xenDaemonDomainShutdown(virConnectPtr conn, virDomainDefPtr def); int xenDaemonDomainReboot(virConnectPtr conn, virDomainDefPtr def); int xenDaemonDomainDestroy(virConnectPtr conn, virDomainDefPtr def); -int xenDaemonDomainSave(virDomainPtr domain, const char *filename); +int xenDaemonDomainSave(virConnectPtr conn, + virDomainDefPtr def, + const char *filename); int xenDaemonDomainCoreDump(virDomainPtr domain, const char *filename, unsigned int flags); int xenDaemonDomainRestore(virConnectPtr conn, const char *filename); @@ -131,7 +133,8 @@ int xenDaemonDetachDeviceFlags(virDomainPtr domain, unsigned int flags); virDomainPtr xenDaemonDomainDefineXML(virConnectPtr xend, const char *sexpr); -int xenDaemonDomainCreate(virDomainPtr domain); +int xenDaemonDomainCreate(virConnectPtr conn, + virDomainDefPtr def);

int xenDaemonDomainUndefine(virDomainPtr domain); int xenDaemonDomainSetVcpus (virDomainPtr domain, diff --git a/src/xen/xm_internal.c b/src/xen/xm_internal.c index ce44e39..6c884d9 100644 --- a/src/xen/xm_internal.c +++ b/src/xen/xm_internal.c @@ -893,48 +893,49 @@ cleanup: * Start a domain from an existing defined config file */ int -xenXMDomainCreate(virDomainPtr domain) +xenXMDomainCreate(virConnectPtr conn, + virDomainDefPtr def)

{ char *sexpr; int ret = -1; - xenUnifiedPrivatePtr priv= domain->conn->privateData; + xenUnifiedPrivatePtr priv = conn->privateData; const char *filename; xenXMConfCachePtr entry = NULL; xenUnifiedLock(priv); - if (!(filename = virHashLookup(priv->nameConfigMap, domain->name))) + if (!(filename = virHashLookup(priv->nameConfigMap, def->name))) goto error; if (!(entry = virHashLookup(priv->configCache, filename))) goto error; - if (!(sexpr = xenFormatSxpr(domain->conn, entry->def, priv->xendConfigVersion))) + if (!(sexpr = xenFormatSxpr(conn, entry->def, priv->xendConfigVersion))) goto error; - ret = xenDaemonDomainCreateXML(domain->conn, sexpr); + ret = xenDaemonDomainCreateXML(conn, sexpr); VIR_FREE(sexpr); if (ret != 0) goto error; - if ((ret = xenDaemonDomainLookupByName_ids(domain->conn, domain->name, + if ((ret = xenDaemonDomainLookupByName_ids(conn, def->name, entry->def->uuid)) < 0) goto error; - domain->id = ret; + def->id = ret; - if (xend_wait_for_devices(domain->conn, domain->name) < 0) + if (xend_wait_for_devices(conn, def->name) < 0) goto error; - if (xenDaemonDomainResume(domain->conn, entry->def) < 0) + if (xenDaemonDomainResume(conn, entry->def) < 0) goto error; xenUnifiedUnlock(priv); return 0; error: - if (domain->id != -1 && entry) { - xenDaemonDomainDestroy(domain->conn, entry->def); - domain->id = -1; + if (def->id != -1 && entry) { + xenDaemonDomainDestroy(conn, entry->def); + def->id = -1; } xenUnifiedUnlock(priv); return -1; diff --git a/src/xen/xm_internal.h b/src/xen/xm_internal.h index 0ae32bc..0521625 100644 --- a/src/xen/xm_internal.h +++ b/src/xen/xm_internal.h @@ -65,7 +65,8 @@ virDomainDefPtr xenXMDomainLookupByUUID(virConnectPtr conn, const unsigned char int xenXMListDefinedDomains(virConnectPtr conn, char ** const names, int maxnames); int xenXMNumOfDefinedDomains(virConnectPtr conn); -int xenXMDomainCreate(virDomainPtr domain); +int xenXMDomainCreate(virConnectPtr conn, + virDomainDefPtr def);

virDomainPtr xenXMDomainDefineXML(virConnectPtr con, const char *xml); int xenXMDomainUndefine(virDomainPtr domain);