Re: [libvirt] [PATCH 05/11] Convert Xen domain start/migration APIs to use virDomainDefPtr

From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; Introduce use of a virDomainDefPtr in the domain migrate & start 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 | 127 ++++++++++++++++++++++++++++++++---------------- src/xen/xend_internal.c | 71 +++++++++------------------ src/xen/xend_internal.h | 22 ++++++--- src/xen/xm_internal.c | 49 ++++++++----------- src/xen/xm_internal.h | 7 +-- 5 files changed, 148 insertions(+), 128 deletions(-) diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index 89b038c..8b7dec9 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -1294,18 +1294,31 @@ static char * xenUnifiedDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) { xenUnifiedPrivatePtr priv = dom->conn->privateData; + virDomainDefPtr minidef = NULL; + virDomainDefPtr def = NULL; + char *ret = NULL; + + if (!(minidef = xenGetDomainDefForDom(dom))) + goto cleanup; if (dom->id < 0 && priv->xendConfigVersion < XEND_CONFIG_VERSION_3_0_4) { - return xenXMDomainGetXMLDesc(dom, flags); + def = xenXMDomainGetXMLDesc(dom->conn, minidef); } else { - char *cpus, *res; + char *cpus; xenUnifiedLock(priv); cpus = xenDomainUsedCpus(dom); xenUnifiedUnlock(priv); - res = xenDaemonDomainGetXMLDesc(dom, flags, cpus); + def = xenDaemonDomainGetXMLDesc(dom->conn, minidef, cpus); VIR_FREE(cpus); - return res; } + + if (def) + ret = virDomainDefFormat(def, flags); + +cleanup: + virDomainDefFree(def); + virDomainDefFree(minidef); + return ret; } @@ -1438,10 +1451,21 @@ xenUnifiedDomainMigratePerform(virDomainPtr dom, const char *dname, unsigned long resource) { + virDomainDefPtr def = NULL; + int ret = -1; + virCheckFlags(XEN_MIGRATION_FLAGS, -1); - return xenDaemonDomainMigratePerform(dom, cookie, cookielen, uri, - flags, dname, resource); + if (!(def = xenGetDomainDefForDom(dom))) + goto cleanup; + + ret = xenDaemonDomainMigratePerform(dom->conn, def, + cookie, cookielen, uri, + flags, dname, resource); + +cleanup: + virDomainDefFree(def); + return ret; } static virDomainPtr @@ -1452,45 +1476,37 @@ xenUnifiedDomainMigrateFinish(virConnectPtr dconn, const char *uri ATTRIBUTE_UNUSED, unsigned long flags) { - virDomainPtr dom = NULL; - char *domain_xml = NULL; - virDomainPtr dom_new = NULL; + xenUnifiedPrivatePtr priv = dconn->privateData; + virDomainPtr ret = NULL; + virDomainDefPtr minidef = NULL; + virDomainDefPtr def = NULL; virCheckFlags(XEN_MIGRATION_FLAGS, NULL); - if (!(dom = xenUnifiedDomainLookupByName(dconn, dname))) - return NULL; + if (!(minidef = xenGetDomainDefForName(dconn, dname))) + goto cleanup; if (flags & VIR_MIGRATE_PERSIST_DEST) { - domain_xml = xenDaemonDomainGetXMLDesc(dom, 0, NULL); - if (! domain_xml) { - virReportError(VIR_ERR_MIGRATE_PERSIST_FAILED, - "%s", _("failed to get XML representation of migrated domain")); - goto error; - } + if (!(def = xenDaemonDomainGetXMLDesc(dconn, minidef, NULL))) + goto cleanup; - dom_new = xenDaemonDomainDefineXML(dconn, domain_xml); - if (! dom_new) { - virReportError(VIR_ERR_MIGRATE_PERSIST_FAILED, - "%s", _("failed to define domain on destination host")); - goto error; + if (priv->xendConfigVersion < XEND_CONFIG_VERSION_3_0_4) { + if (xenXMDomainDefineXML(dconn, def) < 0) + goto cleanup; + } else { + if (xenDaemonDomainDefineXML(dconn, def) < 0) + goto cleanup; } - - /* Free additional reference added by Define */ - virDomainFree(dom_new); } - VIR_FREE(domain_xml); - - return dom; - + ret = virGetDomain(dconn, minidef->name, minidef->uuid); + if (ret) + ret->id = minidef->id; -error: - virDomainFree(dom); - - VIR_FREE(domain_xml); - - return NULL; +cleanup: + virDomainDefFree(def); + virDomainDefFree(minidef); + return ret; } static int @@ -1565,23 +1581,52 @@ static virDomainPtr xenUnifiedDomainDefineXML(virConnectPtr conn, const char *xml) { xenUnifiedPrivatePtr priv = conn->privateData; + virDomainDefPtr def = NULL; + virDomainPtr ret = NULL; - if (priv->xendConfigVersion < XEND_CONFIG_VERSION_3_0_4) - return xenXMDomainDefineXML(conn, xml); - else - return xenDaemonDomainDefineXML(conn, xml); + if (!(def = virDomainDefParseString(xml, priv->caps, priv->xmlopt, + 1 << VIR_DOMAIN_VIRT_XEN, + VIR_DOMAIN_XML_INACTIVE))) + goto cleanup; + + if (priv->xendConfigVersion < XEND_CONFIG_VERSION_3_0_4) { + if (xenXMDomainDefineXML(conn, def) < 0) + goto cleanup; + def = NULL; /* XM driver owns it now */ + } else { + if (xenDaemonDomainDefineXML(conn, def) < 0) + goto cleanup; + } + + ret = virGetDomain(conn, def->name, def->uuid); + if (ret) + ret->id = -1; + +cleanup: + virDomainDefFree(def); + return ret; } static int xenUnifiedDomainUndefineFlags(virDomainPtr dom, unsigned int flags) { xenUnifiedPrivatePtr priv = dom->conn->privateData; + virDomainDefPtr def = NULL; + int ret = -1; virCheckFlags(0, -1); + + if (!(def = xenGetDomainDefForDom(dom))) + goto cleanup; + if (priv->xendConfigVersion < XEND_CONFIG_VERSION_3_0_4) - return xenXMDomainUndefine(dom); + ret = xenXMDomainUndefine(dom->conn, def); else - return xenDaemonDomainUndefine(dom); + ret = xenDaemonDomainUndefine(dom->conn, def); + +cleanup: + virDomainDefFree(def); + return ret; } static int diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index 9555b33..77c4dec 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -1604,7 +1604,6 @@ cleanup: /** * xenDaemonDomainGetXMLDesc: * @domain: a domain object - * @flags: potential dump flags * @cpus: list of cpu the domain is pinned to. * * Provide an XML description of the domain. @@ -1612,27 +1611,15 @@ cleanup: * Returns a 0 terminated UTF-8 encoded XML instance, or NULL in case of error. * the caller must free() the returned value. */

-char * -xenDaemonDomainGetXMLDesc(virDomainPtr domain, - unsigned int flags, +virDomainDefPtr +xenDaemonDomainGetXMLDesc(virConnectPtr conn, + virDomainDefPtr minidef, const char *cpus) { - virDomainDefPtr def; - char *xml; - - /* Flags checked by virDomainDefFormat */ - - if (!(def = xenDaemonDomainFetch(domain->conn, - domain->id, - domain->name, - cpus))) - return NULL; - - xml = virDomainDefFormat(def, flags); - - virDomainDefFree(def); - - return xml; + return xenDaemonDomainFetch(conn, + minidef->id, + minidef->name, + cpus); } @@ -2657,7 +2644,8 @@ xenDaemonDomainMigratePrepare(virConnectPtr dconn ATTRIBUTE_UNUSED, } int -xenDaemonDomainMigratePerform(virDomainPtr domain, +xenDaemonDomainMigratePerform(virConnectPtr conn, + virDomainDefPtr def, const char *cookie ATTRIBUTE_UNUSED, int cookielen ATTRIBUTE_UNUSED, const char *uri, @@ -2801,7 +2789,7 @@ xenDaemonDomainMigratePerform(virDomainPtr domain, * to our advantage since all parameters supported and required * by current xend can be included without breaking older xend. */ - ret = xend_op(domain->conn, domain->name, + ret = xend_op(conn, def->name, "op", "migrate", "destination", hostname, "live", live, @@ -2814,34 +2802,24 @@ xenDaemonDomainMigratePerform(virDomainPtr domain, VIR_FREE(hostname); if (ret == 0 && undefined_source) - xenDaemonDomainUndefine(domain); + xenDaemonDomainUndefine(conn, def); VIR_DEBUG("migration done"); return ret; } -virDomainPtr -xenDaemonDomainDefineXML(virConnectPtr conn, const char *xmlDesc) +int +xenDaemonDomainDefineXML(virConnectPtr conn, virDomainDefPtr def)

{ - int ret; + int ret = -1; char *sexpr; - virDomainPtr dom; xenUnifiedPrivatePtr priv = conn->privateData; - virDomainDefPtr def; - - if (!(def = virDomainDefParseString(xmlDesc, priv->caps, priv->xmlopt, - 1 << VIR_DOMAIN_VIRT_XEN, - VIR_DOMAIN_XML_INACTIVE))) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("failed to parse domain description")); - return NULL; - } if (!(sexpr = xenFormatSxpr(conn, def, priv->xendConfigVersion))) { virReportError(VIR_ERR_XML_ERROR, "%s", _("failed to build sexpr")); - goto error; + goto cleanup; } ret = xend_op(conn, "", "op", "new", "config", sexpr, NULL); @@ -2849,20 +2827,15 @@ xenDaemonDomainDefineXML(virConnectPtr conn, const char *xmlDesc) if (ret != 0) { virReportError(VIR_ERR_XEN_CALL, _("Failed to create inactive domain %s"), def->name); - goto error; + goto cleanup; } - dom = virDomainLookupByName(conn, def->name); - if (dom == NULL) { - goto error; - } - virDomainDefFree(def); - return dom; + ret = 0; - error: - virDomainDefFree(def); - return NULL; +cleanup: + return ret; } + int xenDaemonDomainCreate(virConnectPtr conn, virDomainDefPtr def) @@ -2882,9 +2855,9 @@ xenDaemonDomainCreate(virConnectPtr conn, } int -xenDaemonDomainUndefine(virDomainPtr domain) +xenDaemonDomainUndefine(virConnectPtr conn, virDomainDefPtr def) { - return xend_op(domain->conn, domain->name, "op", "delete", NULL); + return xend_op(conn, def->name, "op", "delete", NULL); } /** diff --git a/src/xen/xend_internal.h b/src/xen/xend_internal.h index 01d321f..1284db3 100644 --- a/src/xen/xend_internal.h +++ b/src/xen/xend_internal.h @@ -111,8 +111,9 @@ int xenDaemonDomainGetState(virConnectPtr conn, virDomainDefPtr def, int *state, int *reason); -char *xenDaemonDomainGetXMLDesc(virDomainPtr domain, unsigned int flags, - const char *cpus); +virDomainDefPtr xenDaemonDomainGetXMLDesc(virConnectPtr conn, + virDomainDefPtr def, + const char *cpus); unsigned long long xenDaemonDomainGetMaxMemory(virConnectPtr conn, virDomainDefPtr def); char **xenDaemonListDomainsOld(virConnectPtr xend); @@ -132,10 +133,12 @@ int xenDaemonDetachDeviceFlags(virDomainPtr domain, const char *xml, unsigned int flags); -virDomainPtr xenDaemonDomainDefineXML(virConnectPtr xend, const char *sexpr); +int xenDaemonDomainDefineXML(virConnectPtr conn, + virDomainDefPtr def); int xenDaemonDomainCreate(virConnectPtr conn, virDomainDefPtr def); -int xenDaemonDomainUndefine(virDomainPtr domain); +int xenDaemonDomainUndefine(virConnectPtr conn, + virDomainDefPtr def);

int xenDaemonDomainSetVcpus (virDomainPtr domain, unsigned int vcpus); @@ -163,8 +166,15 @@ int xenDaemonDomainSetAutostart (virDomainPtr domain, virDomainPtr xenDaemonCreateXML(virConnectPtr conn, const char *xmlDesc); virDomainDefPtr xenDaemonLookupByUUID(virConnectPtr conn, const unsigned char *uuid); virDomainDefPtr xenDaemonLookupByName(virConnectPtr conn, const char *domname); -int xenDaemonDomainMigratePrepare (virConnectPtr dconn, char **cookie, int *cookielen, const char *uri_in, char **uri_out, unsigned long flags, const char *dname, unsigned long resource); -int xenDaemonDomainMigratePerform (virDomainPtr domain, const char *cookie, int cookielen, const char *uri, unsigned long flags, const char *dname, unsigned long resource); +int xenDaemonDomainMigratePrepare (virConnectPtr dconn, + char **cookie, int *cookielen, + const char *uri_in, char **uri_out, + unsigned long flags, const char *dname, unsigned long resource); +int xenDaemonDomainMigratePerform (virConnectPtr conn, + virDomainDefPtr def, + const char *cookie, int cookielen, + const char *uri, unsigned long flags, + const char *dname, unsigned long resource); int xenDaemonDomainBlockPeek (virDomainPtr domain, const char *path, unsigned long long offset, size_t size, void *buffer); diff --git a/src/xen/xm_internal.c b/src/xen/xm_internal.c index 6c884d9..79f773d 100644 --- a/src/xen/xm_internal.c +++ b/src/xen/xm_internal.c @@ -500,28 +500,29 @@ error: * Turn a config record into a lump of XML describing the * domain, suitable for later feeding for virDomainCreateXML */ -char * -xenXMDomainGetXMLDesc(virDomainPtr domain, unsigned int flags) +virDomainDefPtr +xenXMDomainGetXMLDesc(virConnectPtr conn, + virDomainDefPtr def)

{ - xenUnifiedPrivatePtr priv = domain->conn->privateData; + xenUnifiedPrivatePtr priv = conn->privateData; const char *filename; xenXMConfCachePtr entry; - char *ret = NULL; + virDomainDefPtr ret = NULL; /* Flags checked by virDomainDefFormat */ - if (domain->id != -1) - return NULL; - xenUnifiedLock(priv); - if (!(filename = virHashLookup(priv->nameConfigMap, domain->name))) + if (!(filename = virHashLookup(priv->nameConfigMap, def->name))) goto cleanup; if (!(entry = virHashLookup(priv->configCache, filename))) goto cleanup; - ret = virDomainDefFormat(entry->def, flags); + ret = virDomainDefCopy(entry->def, + priv->caps, + priv->xmlopt, + false); cleanup: xenUnifiedUnlock(priv); @@ -945,13 +946,11 @@ xenXMDomainCreate(virConnectPtr conn, * Create a config file for a domain, based on an XML * document describing its config */ -virDomainPtr -xenXMDomainDefineXML(virConnectPtr conn, const char *xml) +int +xenXMDomainDefineXML(virConnectPtr conn, virDomainDefPtr def) { - virDomainPtr ret; char *filename = NULL; const char *oldfilename; - virDomainDefPtr def = NULL; virConfPtr conf = NULL; xenXMConfCachePtr entry = NULL; xenUnifiedPrivatePtr priv = conn->privateData; @@ -960,14 +959,7 @@ xenXMDomainDefineXML(virConnectPtr conn, const char *xml) if (!xenInotifyActive(conn) && xenXMConfigCacheRefresh(conn) < 0) { xenUnifiedUnlock(priv); - return NULL; - } - - if (!(def = virDomainDefParseString(xml, priv->caps, priv->xmlopt, - 1 << VIR_DOMAIN_VIRT_XEN, - VIR_DOMAIN_XML_INACTIVE))) { - xenUnifiedUnlock(priv); - return NULL; + return -1; } if (!(conf = xenFormatXM(conn, def, priv->xendConfigVersion))) @@ -1061,10 +1053,9 @@ xenXMDomainDefineXML(virConnectPtr conn, const char *xml) goto error; } - ret = virGetDomain(conn, def->name, def->uuid); xenUnifiedUnlock(priv); VIR_FREE(filename); - return ret; + return 0; error: VIR_FREE(filename); @@ -1072,25 +1063,25 @@ xenXMDomainDefineXML(virConnectPtr conn, const char *xml) VIR_FREE(entry->filename); VIR_FREE(entry); virConfFree(conf); - virDomainDefFree(def); xenUnifiedUnlock(priv); - return NULL; + return -1; } /* * Delete a domain from disk */ int -xenXMDomainUndefine(virDomainPtr domain) +xenXMDomainUndefine(virConnectPtr conn, + virDomainDefPtr def)

{ - xenUnifiedPrivatePtr priv = domain->conn->privateData; + xenUnifiedPrivatePtr priv = conn->privateData; const char *filename; xenXMConfCachePtr entry; int ret = -1; xenUnifiedLock(priv); - if (!(filename = virHashLookup(priv->nameConfigMap, domain->name))) + if (!(filename = virHashLookup(priv->nameConfigMap, def->name))) goto cleanup; if (!(entry = virHashLookup(priv->configCache, filename))) @@ -1100,7 +1091,7 @@ xenXMDomainUndefine(virDomainPtr domain) goto cleanup; /* Remove the name -> filename mapping */ - if (virHashRemoveEntry(priv->nameConfigMap, domain->name) < 0) + if (virHashRemoveEntry(priv->nameConfigMap, def->name) < 0) goto cleanup; /* Remove the config record itself */ diff --git a/src/xen/xm_internal.h b/src/xen/xm_internal.h index 0521625..5a434b9 100644 --- a/src/xen/xm_internal.h +++ b/src/xen/xm_internal.h @@ -44,7 +44,8 @@ int xenXMDomainGetState(virConnectPtr conn, virDomainDefPtr def, int *state, int *reason); -char *xenXMDomainGetXMLDesc(virDomainPtr domain, unsigned int flags); +virDomainDefPtr xenXMDomainGetXMLDesc(virConnectPtr conn, + virDomainDefPtr def);

int xenXMDomainSetMemory(virConnectPtr conn, virDomainDefPtr def, unsigned long memory); @@ -67,8 +68,8 @@ int xenXMNumOfDefinedDomains(virConnectPtr conn); int xenXMDomainCreate(virConnectPtr conn, virDomainDefPtr def); -virDomainPtr xenXMDomainDefineXML(virConnectPtr con, const char *xml); -int xenXMDomainUndefine(virDomainPtr domain); +int xenXMDomainDefineXML(virConnectPtr con, virDomainDefPtr def); +int xenXMDomainUndefine(virConnectPtr conn, virDomainDefPtr def); int xenXMDomainBlockPeek (virDomainPtr dom, const char *path, unsigned long long offset, size_t size, void *buffer);