Re: [libvirt] [PATCH 30/40] Convert Xen domain lookup driver methods to use virDomainDefPtr

From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; Introduce use of a virDomainDefPtr in the domain lookup 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/conf/domain_conf.c | 23 +++++++++ src/conf/domain_conf.h | 4 ++ src/libvirt_private.syms | 1 + src/xen/xen_driver.c | 132 +++++++++++++++++++++++++++++++++-------------- src/xen/xen_hypervisor.c | 17 +++--- src/xen/xen_hypervisor.h | 8 +-- src/xen/xen_inotify.c | 14 ++--- src/xen/xend_internal.c | 28 ++++------ src/xen/xend_internal.h | 4 +- src/xen/xm_internal.c | 30 ++++------- src/xen/xm_internal.h | 5 +- 11 files changed, 165 insertions(+), 101 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d945b74..1fef6db 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2048,6 +2048,29 @@ error: return NULL; } +

+virDomainDefPtr virDomainDefNew(const char *name, + const unsigned char *uuid, + int id) +{ + virDomainDefPtr def; + + if (VIR_ALLOC(def) < 0) { + virReportOOMError(); + return NULL; + } + + if (!(def->name = strdup(name))) { + VIR_FREE(def); + return NULL; + } + + memcpy(def->uuid, uuid, VIR_UUID_BUFLEN); + def->id = id; + + return def; +} + void virDomainObjAssignDef(virDomainObjPtr domain, const virDomainDefPtr def, bool live, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 3a0f23a..d5d05f7 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2148,6 +2148,10 @@ void virDomainDefFree(virDomainDefPtr vm); virDomainChrDefPtr virDomainChrDefNew(void); +virDomainDefPtr virDomainDefNew(const char *name, + const unsigned char *uuid, + int id); + enum { VIR_DOMAIN_OBJ_LIST_ADD_LIVE = (1 << 0), VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE = (1 << 1), diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 89b65b5..3b625a8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -117,6 +117,7 @@ virDomainDefGenSecurityLabelDef; virDomainDefGetDefaultEmulator; virDomainDefGetSecurityLabelDef; virDomainDefMaybeAddController; +virDomainDefNew; virDomainDefParseFile; virDomainDefParseNode; virDomainDefParseString; diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index d6817eb..86df4b6 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -82,6 +82,58 @@ xenUnifiedDomainGetVcpus(virDomainPtr dom, static bool inside_daemon = false; +static virDomainDefPtr xenGetDomainDefForID(virConnectPtr conn, int id) +{ + virDomainDefPtr ret; + + ret = xenHypervisorLookupDomainByID(conn, id); + + if (!ret && virGetLastError() == NULL) + virReportError(VIR_ERR_NO_DOMAIN, __FUNCTION__); + + return ret; +} + +static virDomainDefPtr xenGetDomainDefForName(virConnectPtr conn, const char *name) +{ + xenUnifiedPrivatePtr priv = conn->privateData; + virDomainDefPtr ret; + + ret = xenDaemonLookupByName(conn, name); + + /* Try XM for inactive domains. */ + if (!ret && + priv->xendConfigVersion <= XEND_CONFIG_VERSION_3_0_3) + ret = xenXMDomainLookupByName(conn, name); + + if (!ret && virGetLastError() == NULL) + virReportError(VIR_ERR_NO_DOMAIN, __FUNCTION__); + + return ret; +} + +static virDomainDefPtr xenGetDomainDefForUUID(virConnectPtr conn, const unsigned char *uuid) +{ + xenUnifiedPrivatePtr priv = conn->privateData; + virDomainDefPtr ret; + + ret = xenHypervisorLookupDomainByUUID(conn, uuid); + + /* Try XM for inactive domains. */ + if (!ret) { + if (priv->xendConfigVersion <= XEND_CONFIG_VERSION_3_0_3) + ret = xenXMDomainLookupByUUID(conn, uuid); + else + ret = xenDaemonLookupByUUID(conn, uuid); + } + + if (!ret && virGetLastError() == NULL) + virReportError(VIR_ERR_NO_DOMAIN, __FUNCTION__); + + return ret; +} + +

/** * xenNumaInit: * @conn: pointer to the hypervisor connection @@ -597,12 +649,18 @@ static virDomainPtr xenUnifiedDomainLookupByID(virConnectPtr conn, int id) { virDomainPtr ret = NULL; + virDomainDefPtr def = NULL; - ret = xenHypervisorLookupDomainByID(conn, id); + if (!(def = xenGetDomainDefForID(conn, id))) + goto cleanup; - if (!ret && virGetLastError() == NULL) - virReportError(VIR_ERR_NO_DOMAIN, __FUNCTION__); + if (!(ret = virGetDomain(conn, def->name, def->uuid))) + goto cleanup; + + ret->id = def->id; +cleanup: + virDomainDefFree(def); return ret; } @@ -610,22 +668,19 @@ static virDomainPtr xenUnifiedDomainLookupByUUID(virConnectPtr conn, const unsigned char *uuid) { - xenUnifiedPrivatePtr priv = conn->privateData; - virDomainPtr ret; + virDomainPtr ret = NULL; + virDomainDefPtr def = NULL; - ret = xenHypervisorLookupDomainByUUID(conn, uuid); + if (!(def = xenGetDomainDefForUUID(conn, uuid))) + goto cleanup; - /* Try XM for inactive domains. */ - if (!ret) { - if (priv->xendConfigVersion <= XEND_CONFIG_VERSION_3_0_3) - ret = xenXMDomainLookupByUUID(conn, uuid); - else - return xenDaemonLookupByUUID(conn, uuid); - } + if (!(ret = virGetDomain(conn, def->name, def->uuid))) + goto cleanup; - if (!ret && virGetLastError() == NULL) - virReportError(VIR_ERR_NO_DOMAIN, __FUNCTION__); + ret->id = def->id; +cleanup: + virDomainDefFree(def); return ret; } @@ -633,18 +688,19 @@ static virDomainPtr xenUnifiedDomainLookupByName(virConnectPtr conn, const char *name) { - xenUnifiedPrivatePtr priv = conn->privateData; - virDomainPtr ret; + virDomainPtr ret = NULL; + virDomainDefPtr def = NULL; - ret = xenDaemonLookupByName(conn, name); + if (!(def = xenGetDomainDefForName(conn, name))) + goto cleanup; - /* Try XM for inactive domains. */ - if (priv->xendConfigVersion <= XEND_CONFIG_VERSION_3_0_3) - ret = xenXMDomainLookupByName(conn, name); + if (!(ret = virGetDomain(conn, def->name, def->uuid))) + goto cleanup; - if (!ret && virGetLastError() == NULL) - virReportError(VIR_ERR_NO_DOMAIN, __FUNCTION__); + ret->id = def->id; +cleanup: + virDomainDefFree(def); return ret; } @@ -652,15 +708,14 @@ xenUnifiedDomainLookupByName(virConnectPtr conn, static int xenUnifiedDomainIsActive(virDomainPtr dom) { - virDomainPtr currdom; + virDomainDefPtr def; int ret = -1; - /* ID field in dom may be outdated, so re-lookup */ - currdom = xenHypervisorLookupDomainByUUID(dom->conn, dom->uuid); + def = xenHypervisorLookupDomainByUUID(dom->conn, dom->uuid); - if (currdom) { - ret = currdom->id == -1 ? 0 : 1; - virDomainFree(currdom); + if (def) { + ret = def->id == -1 ? 0 : 1; + virDomainDefFree(def);

} return ret; @@ -670,22 +725,22 @@ static int xenUnifiedDomainIsPersistent(virDomainPtr dom) { xenUnifiedPrivatePtr priv = dom->conn->privateData; - virDomainPtr currdom = NULL; + virDomainDefPtr def = NULL; int ret = -1; if (priv->opened[XEN_UNIFIED_XM_OFFSET]) { /* Old Xen, pre-inactive domain management. * If the XM driver can see the guest, it is definitely persistent */ - currdom = xenXMDomainLookupByUUID(dom->conn, dom->uuid); - if (currdom) + def = xenXMDomainLookupByUUID(dom->conn, dom->uuid); + if (def) ret = 1; else ret = 0; } else { /* New Xen with inactive domain management */ - currdom = xenDaemonLookupByUUID(dom->conn, dom->uuid); - if (currdom) { - if (currdom->id == -1) { + def = xenDaemonLookupByUUID(dom->conn, dom->uuid); + if (def) { + if (def->id == -1) { /* If its inactive, then trivially, it must be persistent */ ret = 1; } else { @@ -697,7 +752,7 @@ xenUnifiedDomainIsPersistent(virDomainPtr dom) virUUIDFormat(dom->uuid, uuidstr); if (virAsprintf(&path, "%s/%s", XEND_DOMAINS_DIR, uuidstr) < 0) { virReportOOMError(); - goto done; + goto cleanup; } if (access(path, R_OK) == 0) ret = 1; @@ -707,9 +762,8 @@ xenUnifiedDomainIsPersistent(virDomainPtr dom) } } -done: - if (currdom) - virDomainFree(currdom); +cleanup: + virDomainDefFree(def); return ret; } diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c index d7b7cfc..1643091 100644 --- a/src/xen/xen_hypervisor.c +++ b/src/xen/xen_hypervisor.c @@ -2562,12 +2562,13 @@ xenHypervisorHasDomain(virConnectPtr conn, int id) return 1; } -virDomainPtr +

+virDomainDefPtr xenHypervisorLookupDomainByID(virConnectPtr conn, int id) { xenUnifiedPrivatePtr priv = conn->privateData; xen_getdomaininfo dominfo; - virDomainPtr ret; + virDomainDefPtr ret; char *name; XEN_GETDOMAININFO_CLEAR(dominfo); @@ -2584,20 +2585,20 @@ xenHypervisorLookupDomainByID(virConnectPtr conn, int id) if (!name) return NULL; - ret = virGetDomain(conn, name, XEN_GETDOMAININFO_UUID(dominfo)); - if (ret) - ret->id = id; + ret = virDomainDefNew(name, + XEN_GETDOMAININFO_UUID(dominfo), + id); VIR_FREE(name); return ret; } -virDomainPtr +virDomainDefPtr xenHypervisorLookupDomainByUUID(virConnectPtr conn, const unsigned char *uuid) { xen_getdomaininfolist dominfos; xenUnifiedPrivatePtr priv = conn->privateData; - virDomainPtr ret; + virDomainDefPtr ret; char *name; int maxids = 100, nids, i, id; @@ -2648,7 +2649,7 @@ xenHypervisorLookupDomainByUUID(virConnectPtr conn, const unsigned char *uuid) if (!name) return NULL; - ret = virGetDomain(conn, name, uuid); + ret = virDomainDefNew(name, uuid, id); if (ret) ret->id = id; VIR_FREE(name); diff --git a/src/xen/xen_hypervisor.h b/src/xen/xen_hypervisor.h index 8507bd0..1d44a92 100644 --- a/src/xen/xen_hypervisor.h +++ b/src/xen/xen_hypervisor.h @@ -27,6 +27,7 @@ # include "capabilities.h" # include "driver.h" # include "viruri.h" +# include "domain_conf.h" /* See xenHypervisorInit() for details. */ struct xenHypervisorVersions { @@ -43,10 +44,9 @@ virCapsPtr xenHypervisorMakeCapabilities (virConnectPtr conn); int xenHypervisorHasDomain(virConnectPtr conn, int id); -virDomainPtr - xenHypervisorLookupDomainByID (virConnectPtr conn, - int id); -virDomainPtr +virDomainDefPtr + xenHypervisorLookupDomainByID (virConnectPtr conn, int id); +virDomainDefPtr xenHypervisorLookupDomainByUUID (virConnectPtr conn, const unsigned char *uuid); char * diff --git a/src/xen/xen_inotify.c b/src/xen/xen_inotify.c index b032bba..576eb10 100644 --- a/src/xen/xen_inotify.c +++ b/src/xen/xen_inotify.c @@ -76,7 +76,7 @@ xenInotifyXendDomainsDirLookup(virConnectPtr conn, unsigned char *uuid) { int i; - virDomainPtr dom; + virDomainDefPtr def; const char *uuid_str; unsigned char rawuuid[VIR_UUID_BUFLEN]; xenUnifiedPrivatePtr priv = conn->privateData; @@ -96,8 +96,8 @@ xenInotifyXendDomainsDirLookup(virConnectPtr conn, be set during open while we are building our initial list of domains */ VIR_DEBUG("Looking for dom with uuid: %s", uuid_str); - /* XXX Should not have to go via a virDomainPtr obj instance */ - if (!(dom = xenDaemonLookupByUUID(conn, rawuuid))) { + + if (!(def = xenDaemonLookupByUUID(conn, rawuuid))) { /* If we are here, the domain has gone away. search for, and create a domain from the stored list info */ @@ -118,13 +118,13 @@ xenInotifyXendDomainsDirLookup(virConnectPtr conn, return -1; } - if (!(*name = strdup(dom->name))) { + if (!(*name = strdup(def->name))) { virReportOOMError(); - virDomainFree(dom); + virDomainDefFree(def); return -1; } - memcpy(uuid, dom->uuid, VIR_UUID_BUFLEN); - virDomainFree(dom); + memcpy(uuid, def->uuid, VIR_UUID_BUFLEN); + virDomainDefFree(def); /* succeeded too find domain by uuid */ return 0; } diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index ba4a018..1d5ebbb 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -1113,13 +1113,14 @@ sexpr_to_xend_topology(const struct sexpr *root, virCapsPtr caps) * * Returns the domain pointer or NULL in case of error.

*/ -static virDomainPtr +static virDomainDefPtr sexpr_to_domain(virConnectPtr conn, const struct sexpr *root) { - virDomainPtr ret = NULL; + virDomainDefPtr ret = NULL; unsigned char uuid[VIR_UUID_BUFLEN]; const char *name; const char *tmp; + int id = -1; xenUnifiedPrivatePtr priv = conn->privateData; if (sexpr_uuid(uuid, root, "domain/uuid") < 0) @@ -1128,9 +1129,6 @@ sexpr_to_domain(virConnectPtr conn, const struct sexpr *root) if (name == NULL) goto error; - ret = virGetDomain(conn, name, uuid); - if (ret == NULL) return NULL; - tmp = sexpr_node(root, "domain/domid"); /* New 3.0.4 XenD will not report a domid for inactive domains, * so only error out for old XenD @@ -1139,11 +1137,9 @@ sexpr_to_domain(virConnectPtr conn, const struct sexpr *root) goto error; if (tmp) - ret->id = sexpr_int(root, "domain/domid"); - else - ret->id = -1; /* An inactive domain */ + id = sexpr_int(root, "domain/domid"); - return ret; + return virDomainDefNew(name, uuid, id); error: virReportError(VIR_ERR_INTERNAL_ERROR, @@ -1696,11 +1692,11 @@ xenDaemonDomainGetState(virDomainPtr domain, * * Returns domain info on success; NULL (with errno) on error

*/ -virDomainPtr +virDomainDefPtr xenDaemonLookupByName(virConnectPtr conn, const char *domname) { struct sexpr *root; - virDomainPtr ret = NULL; + virDomainDefPtr ret = NULL; root = sexpr_get(conn, "/xend/domain/%s?detail=1", domname); if (root == NULL) @@ -2051,10 +2047,10 @@ xenDaemonDomainGetVcpus(virDomainPtr domain, * * Returns a new domain object or NULL in case of failure

*/ -virDomainPtr +virDomainDefPtr xenDaemonLookupByUUID(virConnectPtr conn, const unsigned char *uuid) { - virDomainPtr ret; + virDomainDefPtr ret; char *name = NULL; int id = -1; xenUnifiedPrivatePtr priv = conn->privateData; @@ -2114,12 +2110,8 @@ xenDaemonLookupByUUID(virConnectPtr conn, const unsigned char *uuid) if (name == NULL) return NULL; - ret = virGetDomain(conn, name, uuid); - if (ret == NULL) goto cleanup; - - ret->id = id; + ret = virDomainDefNew(name, uuid, id); - cleanup: VIR_FREE(name); return ret; } diff --git a/src/xen/xend_internal.h b/src/xen/xend_internal.h index 7332303..a2d05f3 100644 --- a/src/xen/xend_internal.h +++ b/src/xen/xend_internal.h @@ -149,8 +149,8 @@ int xenDaemonDomainSetAutostart (virDomainPtr domain, int autostart); virDomainPtr xenDaemonCreateXML(virConnectPtr conn, const char *xmlDesc); -virDomainPtr xenDaemonLookupByUUID(virConnectPtr conn, const unsigned char *uuid); -virDomainPtr xenDaemonLookupByName(virConnectPtr conn, const char *domname); +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); diff --git a/src/xen/xm_internal.c b/src/xen/xm_internal.c index fa0392b..880cdeb 100644 --- a/src/xen/xm_internal.c +++ b/src/xen/xm_internal.c @@ -819,13 +819,13 @@ xenXMDomainPinVcpu(virDomainPtr domain, /* * Find an inactive domain based on its name */ -virDomainPtr +virDomainDefPtr xenXMDomainLookupByName(virConnectPtr conn, const char *domname) { xenUnifiedPrivatePtr priv = conn->privateData; const char *filename; xenXMConfCachePtr entry; - virDomainPtr ret = NULL; + virDomainDefPtr ret = NULL; xenUnifiedLock(priv); @@ -838,12 +838,7 @@ xenXMDomainLookupByName(virConnectPtr conn, const char *domname) if (!(entry = virHashLookup(priv->configCache, filename))) goto cleanup; - if (!(ret = virGetDomain(conn, domname, entry->def->uuid))) - goto cleanup; - - /* Ensure its marked inactive, because may be cached - handle to a previously active domain */ - ret->id = -1; + ret = virDomainDefNew(domname, entry->def->uuid, -1); cleanup: xenUnifiedUnlock(priv); @@ -871,12 +866,12 @@ xenXMDomainSearchForUUID(const void *payload, /* * Find an inactive domain based on its UUID */ -virDomainPtr +virDomainDefPtr xenXMDomainLookupByUUID(virConnectPtr conn, const unsigned char *uuid) { xenUnifiedPrivatePtr priv = conn->privateData; xenXMConfCachePtr entry; - virDomainPtr ret = NULL; + virDomainDefPtr ret = NULL; xenUnifiedLock(priv); @@ -886,12 +881,7 @@ xenXMDomainLookupByUUID(virConnectPtr conn, const unsigned char *uuid) if (!(entry = virHashSearch(priv->configCache, xenXMDomainSearchForUUID, (const void *)uuid))) goto cleanup; - if (!(ret = virGetDomain(conn, entry->def->name, uuid))) - goto cleanup; - - /* Ensure its marked inactive, because may be cached - handle to a previously active domain */ - ret->id = -1; + ret = virDomainDefNew(entry->def->name, uuid, -1); cleanup: xenUnifiedUnlock(priv); @@ -1134,7 +1124,7 @@ struct xenXMListIteratorContext { static void xenXMListIterator(void *payload ATTRIBUTE_UNUSED, const void *name, void *data) { struct xenXMListIteratorContext *ctx = data; - virDomainPtr dom = NULL; + virDomainDefPtr def = NULL; if (ctx->oom) return; @@ -1142,14 +1132,14 @@ xenXMListIterator(void *payload ATTRIBUTE_UNUSED, const void *name, void *data) if (ctx->count == ctx->max) return; - dom = xenDaemonLookupByName(ctx->conn, name); - if (!dom) { + def = xenDaemonLookupByName(ctx->conn, name); + if (!def) { if (!(ctx->names[ctx->count] = strdup(name))) ctx->oom = 1; else ctx->count++; } else { - virDomainFree(dom); + virDomainDefFree(def); } } diff --git a/src/xen/xm_internal.h b/src/xen/xm_internal.h index 731a126..95f8283 100644 --- a/src/xen/xm_internal.h +++ b/src/xen/xm_internal.h @@ -52,9 +52,8 @@ int xenXMDomainSetVcpusFlags(virDomainPtr domain, unsigned int vcpus, int xenXMDomainGetVcpusFlags(virDomainPtr domain, unsigned int flags); int xenXMDomainPinVcpu(virDomainPtr domain, unsigned int vcpu, unsigned char *cpumap, int maplen); -virDomainPtr xenXMDomainLookupByName(virConnectPtr conn, const char *domname); -virDomainPtr xenXMDomainLookupByUUID(virConnectPtr conn, - const unsigned char *uuid); +virDomainDefPtr xenXMDomainLookupByName(virConnectPtr conn, const char *domname); +virDomainDefPtr xenXMDomainLookupByUUID(virConnectPtr conn, const unsigned char *uuid);

int xenXMListDefinedDomains(virConnectPtr conn, char ** const names, int maxnames); int xenXMNumOfDefinedDomains(virConnectPtr conn);