I finally have some time to continue reviewing this series...
Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
>
> Introduce use of a virDomainDefPtr in the domain migrate &
> start APIs to simplify introduction of ACL security checks.
>
Not really the 'start' API, but GetXMLDesc, Define, and Undefine. Should
those be part of the lifecycle changes made in patch 2?
> 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@xxxxxxxxxx>
> ---
> 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.
> */
>
Comments for this function need updated, e.g. the domain parameter no
longer exists and it now returns a virtDomainDefPtr.
> -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)
>
xenDaemonDomainGetXMLDesc and xenDaemonDomainDefineXML are poorly named
now that they don't return or accept XML, but that's a super nit :).
> {
> - 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);
>
These still fit on one line, but this whole files has inconsistent use
of whitespace.
>
> 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)
>
Still fits on one line.
> {
> - 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)
>
Still fits on one line.
> {
> - 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);
>
This still squeezes into one line too.
Regards,
Jim
> 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);
>
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list