Re: [libvirt] [PATCH 02/11] Convert Xen domain lifecycle driver methods to use virDomainDefPtr

From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; Introduce use of a virDomainDefPtr in the domain lifecycle 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 | 67 +++++++++++++++++++++++++++++++++++++++++++++---- src/xen/xend_internal.c | 60 ++++++++++++++++++++++--------------------- src/xen/xend_internal.h | 10 ++++---- src/xen/xm_internal.c | 8 +++--- 4 files changed, 103 insertions(+), 42 deletions(-)

diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index d9420d8..37107ff 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -136,6 +136,13 @@ static virDomainDefPtr xenGetDomainDefForUUID(virConnectPtr conn, const unsigned } +static virDomainDefPtr xenGetDomainDefForDom(virDomainPtr dom) +{ + /* UUID lookup is more efficient than name lookup */ + return xenGetDomainDefForUUID(dom->conn, dom->uuid); +} + + /** * xenNumaInit: * @conn: pointer to the hypervisor connection @@ -781,22 +788,52 @@ xenUnifiedDomainIsUpdated(virDomainPtr dom ATTRIBUTE_UNUSED) static int xenUnifiedDomainSuspend(virDomainPtr dom) { - return xenDaemonDomainSuspend(dom); + int ret = -1; + virDomainDefPtr def; + + if (!(def = xenGetDomainDefForDom(dom))) + goto cleanup; + + ret = xenDaemonDomainSuspend(dom->conn, def); + +cleanup: + virDomainDefFree(def); + return ret; } static int xenUnifiedDomainResume(virDomainPtr dom) { - return xenDaemonDomainResume(dom); + int ret = -1; + virDomainDefPtr def; + + if (!(def = xenGetDomainDefForDom(dom))) + goto cleanup; + + ret = xenDaemonDomainResume(dom->conn, def); + +cleanup: + virDomainDefFree(def); + return ret; } static int xenUnifiedDomainShutdownFlags(virDomainPtr dom, unsigned int flags) { + int ret = -1; + virDomainDefPtr def; + virCheckFlags(0, -1); - return xenDaemonDomainShutdown(dom); + if (!(def = xenGetDomainDefForDom(dom))) + goto cleanup; + + ret = xenDaemonDomainShutdown(dom->conn, def); + +cleanup: + virDomainDefFree(def); + return ret; } static int @@ -808,18 +845,38 @@ xenUnifiedDomainShutdown(virDomainPtr dom) static int xenUnifiedDomainReboot(virDomainPtr dom, unsigned int flags) { + int ret = -1; + virDomainDefPtr def; + virCheckFlags(0, -1); - return xenDaemonDomainReboot(dom); + if (!(def = xenGetDomainDefForDom(dom))) + goto cleanup; + + ret = xenDaemonDomainReboot(dom->conn, def); + +cleanup: + virDomainDefFree(def); + return ret; } static int xenUnifiedDomainDestroyFlags(virDomainPtr dom, unsigned int flags) { + int ret = -1; + virDomainDefPtr def; + virCheckFlags(0, -1); - return xenDaemonDomainDestroy(dom); + if (!(def = xenGetDomainDefForDom(dom))) + goto cleanup; + + ret = xenDaemonDomainDestroy(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 5ea1627..f8bd72b 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -1251,7 +1251,8 @@ xenDaemonClose(virConnectPtr conn ATTRIBUTE_UNUSED) /** * xenDaemonDomainSuspend: - * @domain: pointer to the Domain block + * @conn: the connection object + * @def: the domain to suspend * * Pause the domain, the domain is not scheduled anymore though its resources * are preserved. Use xenDaemonDomainResume() to resume execution. @@ -1259,41 +1260,42 @@ xenDaemonClose(virConnectPtr conn ATTRIBUTE_UNUSED) * Returns 0 in case of success, -1 (with errno) in case of error. */ int -xenDaemonDomainSuspend(virDomainPtr domain) +xenDaemonDomainSuspend(virConnectPtr conn, virDomainDefPtr def) { - 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; } - return xend_op(domain->conn, domain->name, "op", "pause", NULL); + return xend_op(conn, def->name, "op", "pause", NULL); } /** * xenDaemonDomainResume: - * @xend: pointer to the Xen Daemon block - * @name: name for the domain + * @conn: the connection object + * @def: the domain to resume * * Resume the domain after xenDaemonDomainSuspend() has been called * * Returns 0 in case of success, -1 (with errno) in case of error. */ int -xenDaemonDomainResume(virDomainPtr domain) +xenDaemonDomainResume(virConnectPtr conn, virDomainDefPtr def) { - 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; } - return xend_op(domain->conn, domain->name, "op", "unpause", NULL); + return xend_op(conn, def->name, "op", "unpause", NULL); } /** * xenDaemonDomainShutdown: - * @domain: pointer to the Domain block + * @conn: the connection object + * @def: the domain to shutdown * * Shutdown the domain, the OS is requested to properly shutdown * and the domain may ignore it. It will return immediately @@ -1302,20 +1304,21 @@ xenDaemonDomainResume(virDomainPtr domain) * Returns 0 in case of success, -1 (with errno) in case of error. */ int -xenDaemonDomainShutdown(virDomainPtr domain) +xenDaemonDomainShutdown(virConnectPtr conn, virDomainDefPtr def) { - 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; } - return xend_op(domain->conn, domain->name, "op", "shutdown", "reason", "poweroff", NULL); + return xend_op(conn, def->name, "op", "shutdown", "reason", "poweroff", NULL); } /** * xenDaemonDomainReboot: - * @domain: pointer to the Domain block + * @conn: the connection object + * @def: the domain to reboot * * Reboot the domain, the OS is requested to properly shutdown * and restart but the domain may ignore it. It will return immediately @@ -1324,20 +1327,21 @@ xenDaemonDomainShutdown(virDomainPtr domain) * Returns 0 in case of success, -1 (with errno) in case of error. */ int -xenDaemonDomainReboot(virDomainPtr domain) +xenDaemonDomainReboot(virConnectPtr conn, virDomainDefPtr def) { - 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; } - return xend_op(domain->conn, domain->name, "op", "shutdown", "reason", "reboot", NULL); + return xend_op(conn, def->name, "op", "shutdown", "reason", "reboot", NULL); } /** * xenDaemonDomainDestroy: - * @domain: pointer to the Domain block + * @conn: the connection object + * @def: the domain to destroy * * Abruptly halt the domain, the OS is not properly shutdown and the * resources allocated for the domain are immediately freed, mounted @@ -1349,15 +1353,15 @@ xenDaemonDomainReboot(virDomainPtr domain) * Returns 0 in case of success, -1 (with errno) in case of error. */ int -xenDaemonDomainDestroy(virDomainPtr domain) +xenDaemonDomainDestroy(virConnectPtr conn, virDomainDefPtr def) { - 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; } - return xend_op(domain->conn, domain->name, "op", "destroy", NULL); + return xend_op(conn, def->name, "op", "destroy", NULL); } /** @@ -2160,7 +2164,7 @@ xenDaemonCreateXML(virConnectPtr conn, const char *xmlDesc) if (xend_wait_for_devices(conn, def->name) < 0) goto error; - if (xenDaemonDomainResume(dom) < 0) + if (xenDaemonDomainResume(conn, def) < 0) goto error; virDomainDefFree(def); @@ -2169,7 +2173,7 @@ xenDaemonCreateXML(virConnectPtr conn, const char *xmlDesc) error: /* Make sure we don't leave a still-born domain around */ if (dom != NULL) { - xenDaemonDomainDestroy(dom); + xenDaemonDomainDestroy(conn, def); virObjectUnref(dom); } virDomainDefFree(def); diff --git a/src/xen/xend_internal.h b/src/xen/xend_internal.h index a2d05f3..4a6b406 100644 --- a/src/xen/xend_internal.h +++ b/src/xen/xend_internal.h @@ -87,11 +87,11 @@ int xenDaemonOpen(virConnectPtr conn, virConnectAuthPtr auth, int xenDaemonClose(virConnectPtr conn); int xenDaemonNodeGetInfo(virConnectPtr conn, virNodeInfoPtr info); int xenDaemonNodeGetTopology(virConnectPtr conn, virCapsPtr caps); -int xenDaemonDomainSuspend(virDomainPtr domain); -int xenDaemonDomainResume(virDomainPtr domain); -int xenDaemonDomainShutdown(virDomainPtr domain); -int xenDaemonDomainReboot(virDomainPtr domain); -int xenDaemonDomainDestroy(virDomainPtr domain); +int xenDaemonDomainSuspend(virConnectPtr conn, virDomainDefPtr def); +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 xenDaemonDomainCoreDump(virDomainPtr domain, const char *filename, unsigned int flags); diff --git a/src/xen/xm_internal.c b/src/xen/xm_internal.c index 9b8d742..f4e8ea0 100644 --- a/src/xen/xm_internal.c +++ b/src/xen/xm_internal.c @@ -894,7 +894,7 @@ xenXMDomainCreate(virDomainPtr domain) int ret = -1; xenUnifiedPrivatePtr priv= domain->conn->privateData; const char *filename; - xenXMConfCachePtr entry; + xenXMConfCachePtr entry = NULL; xenUnifiedLock(priv); @@ -920,15 +920,15 @@ xenXMDomainCreate(virDomainPtr domain) if (xend_wait_for_devices(domain->conn, domain->name) < 0) goto error; - if (xenDaemonDomainResume(domain) < 0) + if (xenDaemonDomainResume(domain->conn, entry->def) < 0) goto error; xenUnifiedUnlock(priv); return 0; error: - if (domain->id != -1) { - xenDaemonDomainDestroy(domain); + if (domain->id != -1 && entry) { + xenDaemonDomainDestroy(domain->conn, entry->def); domain->id = -1; } xenUnifiedUnlock(priv);