On Thu, May 09, 2013 at 03:01:42PM +0200, Michal Privoznik wrote:

On 09.05.2013 14:59, Daniel P. Berrange wrote:

...

From: "Daniel P. Berrange" <berrange@redhat.com>

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@redhat.com> --- src/conf/domain_conf.c | 24 ++++++++ src/conf/domain_conf.h | 4 ++ src/libvirt_private.syms | 1 + src/xen/xen_driver.c | 147 +++++++++++++++++++++++++++++++---------------- src/xen/xen_hypervisor.c | 17 +++--- src/xen/xen_hypervisor.h | 8 +-- src/xen/xen_inotify.c | 14 ++--- src/xen/xend_internal.c | 34 +++++------ src/xen/xend_internal.h | 4 +- src/xen/xm_internal.c | 30 ++++------ src/xen/xm_internal.h | 5 +- 11 files changed, 173 insertions(+), 115 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d55ce6b..61995cd 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2048,6 +2048,30 @@ 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; + }

Can you switch to VIR_STRDUP instead?

Ok, consider this to be squashed in once acked @@ -2060,7 +2060,7 @@ virDomainDefPtr virDomainDefNew(const char *name, return NULL; } - if (!(def->name = strdup(name))) { + if (VIR_STRDUP(def->name, name) < 0) { VIR_FREE(def); return NULL; } Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On Thu, May 09, 2013 at 11:52:39AM -0600, Jim Fehlig wrote:

Daniel P. Berrange wrote:

...

From: "Daniel P. Berrange" <berrange@redhat.com>

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@redhat.com> --- src/conf/domain_conf.c | 24 ++++++++ src/conf/domain_conf.h | 4 ++ src/libvirt_private.syms | 1 + src/xen/xen_driver.c | 147 +++++++++++++++++++++++++++++++---------------- src/xen/xen_hypervisor.c | 17 +++--- src/xen/xen_hypervisor.h | 8 +-- src/xen/xen_inotify.c | 14 ++--- src/xen/xend_internal.c | 34 +++++------ src/xen/xend_internal.h | 4 +- src/xen/xm_internal.c | 30 ++++------ src/xen/xm_internal.h | 5 +- 11 files changed, 173 insertions(+), 115 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d55ce6b..61995cd 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2048,6 +2048,30 @@ error: return NULL; }

+

Extra newline? I've noticed inconsistencies throughout most of the files wrt 1 or 2 newlines between function definitions, so difficult to say which is preferred.

Yep, my preference is to use 2 newlines of whitespace between functions, so you'll see that throughout these patches, though as you say we don't enforce that strongly. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

Daniel P. Berrange wrote:

From: "Daniel P. Berrange" <berrange@redhat.com>

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@redhat.com> --- 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(-)

Looks good. ACK. Regards, Jim

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);

Daniel P. Berrange wrote:

From: "Daniel P. Berrange" <berrange@redhat.com>

Introduce use of a virDomainDefPtr in the domain property 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@redhat.com> --- src/xen/xen_driver.c | 98 +++++++++++++++++++++++++++++++++++++----------- src/xen/xen_hypervisor.c | 42 +++++++++++---------- src/xen/xen_hypervisor.h | 18 +++++---- src/xen/xend_internal.c | 44 +++++++++++++--------- src/xen/xend_internal.h | 21 ++++++++--- src/xen/xm_internal.c | 41 +++++++++++--------- src/xen/xm_internal.h | 18 ++++++--- 7 files changed, 187 insertions(+), 95 deletions(-)

diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index 37107ff..68a86b7 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -889,18 +889,27 @@ static char * xenUnifiedDomainGetOSType(virDomainPtr dom) { xenUnifiedPrivatePtr priv = dom->conn->privateData; + char *ret = NULL; + virDomainDefPtr def;

- if (dom->id < 0) { + if (!(def = xenGetDomainDefForDom(dom))) + goto cleanup; + + if (def->id < 0) { if (priv->xendConfigVersion < XEND_CONFIG_VERSION_3_0_4) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unable to query OS type for inactive domain")); return NULL; } else { - return xenDaemonDomainGetOSType(dom); + ret = xenHypervisorDomainGetOSType(dom->conn, def);

Should still call xenDaemonDomainGetOSType here since id < 0 right?

} } else { - return xenHypervisorDomainGetOSType(dom); + ret = xenDaemonDomainGetOSType(dom->conn, def);

And call the hypervisor one here when the domain is active.

} + +cleanup: + virDomainDefFree(def); + return ret; }

@@ -908,56 +917,92 @@ static unsigned long long xenUnifiedDomainGetMaxMemory(virDomainPtr dom) { xenUnifiedPrivatePtr priv = dom->conn->privateData; + unsigned long long ret = 0; + virDomainDefPtr def;

- if (dom->id < 0) { + if (!(def = xenGetDomainDefForDom(dom))) + goto cleanup; + + if (def->id < 0) { if (priv->xendConfigVersion < XEND_CONFIG_VERSION_3_0_4) - return xenXMDomainGetMaxMemory(dom); + ret = xenXMDomainGetMaxMemory(dom->conn, def); else - return xenDaemonDomainGetMaxMemory(dom); + ret = xenDaemonDomainGetMaxMemory(dom->conn, def); } else { - return xenHypervisorGetMaxMemory(dom); + ret = xenHypervisorGetMaxMemory(dom->conn, def); } + +cleanup: + virDomainDefFree(def); + return ret; }

static int xenUnifiedDomainSetMaxMemory(virDomainPtr dom, unsigned long memory) { xenUnifiedPrivatePtr priv = dom->conn->privateData; + int ret = -1; + virDomainDefPtr def;

- if (dom->id < 0) { + if (!(def = xenGetDomainDefForDom(dom))) + goto cleanup; + + if (def->id < 0) { if (priv->xendConfigVersion < XEND_CONFIG_VERSION_3_0_4) - return xenXMDomainSetMaxMemory(dom, memory); + ret = xenXMDomainSetMaxMemory(dom->conn, def, memory); else - return xenDaemonDomainSetMaxMemory(dom, memory); + ret = xenDaemonDomainSetMaxMemory(dom->conn, def, memory); } else { - return xenHypervisorSetMaxMemory(dom, memory); + ret = xenHypervisorSetMaxMemory(dom->conn, def, memory); } + +cleanup: + virDomainDefFree(def); + return ret; }

static int xenUnifiedDomainSetMemory(virDomainPtr dom, unsigned long memory) { xenUnifiedPrivatePtr priv = dom->conn->privateData; + int ret = -1; + virDomainDefPtr def;

- if (dom->id < 0 && priv->xendConfigVersion < XEND_CONFIG_VERSION_3_0_4) - return xenXMDomainSetMemory(dom, memory); + if (!(def = xenGetDomainDefForDom(dom))) + goto cleanup; + + if (def->id < 0 && priv->xendConfigVersion < XEND_CONFIG_VERSION_3_0_4) + ret = xenXMDomainSetMemory(dom->conn, def, memory); else - return xenDaemonDomainSetMemory(dom, memory); + ret = xenDaemonDomainSetMemory(dom->conn, def, memory); + +cleanup: + virDomainDefFree(def); + return ret; }

static int xenUnifiedDomainGetInfo(virDomainPtr dom, virDomainInfoPtr info) { xenUnifiedPrivatePtr priv = dom->conn->privateData; + int ret = -1; + virDomainDefPtr def;

- if (dom->id < 0) { + if (!(def = xenGetDomainDefForDom(dom))) + goto cleanup; + + if (def->id < 0) { if (priv->xendConfigVersion < XEND_CONFIG_VERSION_3_0_4) - return xenXMDomainGetInfo(dom, info); + ret = xenXMDomainGetInfo(dom->conn, def, info); else - return xenDaemonDomainGetInfo(dom, info); + ret = xenDaemonDomainGetInfo(dom->conn, def, info); } else { - return xenHypervisorGetDomainInfo(dom, info); + ret = xenHypervisorGetDomainInfo(dom->conn, def, info); } + +cleanup: + virDomainDefFree(def); + return ret; }

static int @@ -967,17 +1012,26 @@ xenUnifiedDomainGetState(virDomainPtr dom, unsigned int flags) { xenUnifiedPrivatePtr priv = dom->conn->privateData; + int ret = -1; + virDomainDefPtr def;

virCheckFlags(0, -1);

- if (dom->id < 0) { + if (!(def = xenGetDomainDefForDom(dom))) + goto cleanup; + + if (def->id < 0) { if (priv->xendConfigVersion < XEND_CONFIG_VERSION_3_0_4) - return xenXMDomainGetState(dom, state, reason); + ret = xenXMDomainGetState(dom->conn, def, state, reason); else - return xenDaemonDomainGetState(dom, state, reason); + ret = xenDaemonDomainGetState(dom->conn, def, state, reason); } else { - return xenHypervisorGetDomainState(dom, state, reason); + ret = xenHypervisorGetDomainState(dom->conn, def, state, reason); } + +cleanup: + virDomainDefFree(def); + return ret; }

static int diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c index 55b0930..423ce85 100644 --- a/src/xen/xen_hypervisor.c +++ b/src/xen/xen_hypervisor.c @@ -2508,27 +2508,28 @@ xenHypervisorGetCapabilities(virConnectPtr conn)

char * -xenHypervisorDomainGetOSType(virDomainPtr dom) +xenHypervisorDomainGetOSType(virConnectPtr conn, + virDomainDefPtr def)

Fits on one line.

{ - xenUnifiedPrivatePtr priv = dom->conn->privateData; + xenUnifiedPrivatePtr priv = conn->privateData; xen_getdomaininfo dominfo; char *ostype = NULL;

/* HV's earlier than 3.1.0 don't include the HVM flags in guests status*/ if (hv_versions.hypervisor < 2 || hv_versions.dom_interface < 4) { - return xenDaemonDomainGetOSType(dom); + return xenDaemonDomainGetOSType(conn, def); }

XEN_GETDOMAININFO_CLEAR(dominfo);

- if (virXen_getdomaininfo(priv->handle, dom->id, &dominfo) < 0) { + if (virXen_getdomaininfo(priv->handle, def->id, &dominfo) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot get domain details")); return NULL; }

- if (XEN_GETDOMAININFO_DOMAIN(dominfo) != dom->id) { + if (XEN_GETDOMAININFO_DOMAIN(dominfo) != def->id) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot get domain details")); return NULL; @@ -2678,9 +2679,10 @@ xenHypervisorGetMaxVcpus(virConnectPtr conn ATTRIBUTE_UNUSED, * Returns the memory size in kilobytes or 0 in case of error. */ unsigned long -xenHypervisorGetMaxMemory(virDomainPtr dom) +xenHypervisorGetMaxMemory(virConnectPtr conn, + virDomainDefPtr def)

Same here.

{ - xenUnifiedPrivatePtr priv = dom->conn->privateData; + xenUnifiedPrivatePtr priv = conn->privateData; xen_getdomaininfo dominfo; int ret;

@@ -2692,9 +2694,9 @@ xenHypervisorGetMaxMemory(virDomainPtr dom)

XEN_GETDOMAININFO_CLEAR(dominfo);

- ret = virXen_getdomaininfo(priv->handle, dom->id, &dominfo); + ret = virXen_getdomaininfo(priv->handle, def->id, &dominfo);

- if ((ret < 0) || (XEN_GETDOMAININFO_DOMAIN(dominfo) != dom->id)) + if ((ret < 0) || (XEN_GETDOMAININFO_DOMAIN(dominfo) != def->id)) return 0;

return (unsigned long) XEN_GETDOMAININFO_MAX_PAGES(dominfo) * kb_per_pages; @@ -2788,9 +2790,11 @@ xenHypervisorGetDomInfo(virConnectPtr conn, int id, virDomainInfoPtr info) * Returns 0 in case of success, -1 in case of error. */ int -xenHypervisorGetDomainInfo(virDomainPtr domain, virDomainInfoPtr info) +xenHypervisorGetDomainInfo(virConnectPtr conn, + virDomainDefPtr def, + virDomainInfoPtr info) { - return xenHypervisorGetDomInfo(domain->conn, domain->id, info); + return xenHypervisorGetDomInfo(conn, def->id, info); }

/** @@ -2804,13 +2808,14 @@ xenHypervisorGetDomainInfo(virDomainPtr domain, virDomainInfoPtr info) * Returns 0 in case of success, -1 in case of error. */ int -xenHypervisorGetDomainState(virDomainPtr domain, +xenHypervisorGetDomainState(virConnectPtr conn, + virDomainDefPtr def, int *state, int *reason) { virDomainInfo info;

- if (xenHypervisorGetDomInfo(domain->conn, domain->id, &info) < 0) + if (xenHypervisorGetDomInfo(conn, def->id, &info) < 0) return -1;

*state = info.state; @@ -2899,15 +2904,14 @@ xenHypervisorNodeGetCellsFreeMemory(virConnectPtr conn, * Returns 0 in case of success, -1 in case of error. */ int -xenHypervisorSetMaxMemory(virDomainPtr domain, unsigned long memory) +xenHypervisorSetMaxMemory(virConnectPtr conn, + virDomainDefPtr def, + unsigned long memory) { int ret; - xenUnifiedPrivatePtr priv = domain->conn->privateData; - - if (domain->id < 0) - return -1; + xenUnifiedPrivatePtr priv = conn->privateData;

- ret = virXen_setmaxmem(priv->handle, domain->id, memory); + ret = virXen_setmaxmem(priv->handle, def->id, memory); if (ret < 0) return -1; return 0; diff --git a/src/xen/xen_hypervisor.h b/src/xen/xen_hypervisor.h index 1d44a92..9ee1f13 100644 --- a/src/xen/xen_hypervisor.h +++ b/src/xen/xen_hypervisor.h @@ -50,7 +50,8 @@ virDomainDefPtr xenHypervisorLookupDomainByUUID (virConnectPtr conn, const unsigned char *uuid); char * - xenHypervisorDomainGetOSType (virDomainPtr dom); + xenHypervisorDomainGetOSType (virConnectPtr conn, + virDomainDefPtr def);

int xenHypervisorOpen (virConnectPtr conn, @@ -64,23 +65,26 @@ virCapsPtr virArch hostarch, FILE *cpuinfo, FILE *capabilities); -char * - xenHypervisorGetCapabilities (virConnectPtr conn); +char * xenHypervisorGetCapabilities (virConnectPtr conn); unsigned long - xenHypervisorGetMaxMemory(virDomainPtr dom); + xenHypervisorGetMaxMemory(virConnectPtr conn, + virDomainDefPtr def); int xenHypervisorGetMaxVcpus (virConnectPtr conn, const char *type); -int xenHypervisorGetDomainInfo (virDomainPtr domain, +int xenHypervisorGetDomainInfo (virConnectPtr conn, + virDomainDefPtr def, virDomainInfoPtr info) ATTRIBUTE_NONNULL (1); -int xenHypervisorGetDomainState (virDomainPtr domain, +int xenHypervisorGetDomainState (virConnectPtr conn, + virDomainDefPtr def, int *state, int *reason) ATTRIBUTE_NONNULL (1); int xenHypervisorGetDomInfo (virConnectPtr conn, int id, virDomainInfoPtr info); -int xenHypervisorSetMaxMemory (virDomainPtr domain, +int xenHypervisorSetMaxMemory (virConnectPtr conn, + virDomainDefPtr def, unsigned long memory) ATTRIBUTE_NONNULL (1); int xenHypervisorCheckID (virConnectPtr conn, diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index f8bd72b..c10567c 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -890,7 +890,7 @@ xend_detect_config_version(virConnectPtr conn) */ static int ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) -sexpr_to_xend_domain_state(virDomainPtr domain, const struct sexpr *root) +sexpr_to_xend_domain_state(virDomainDefPtr def, const struct sexpr *root) { const char *flags; int state = VIR_DOMAIN_NOSTATE; @@ -908,7 +908,7 @@ sexpr_to_xend_domain_state(virDomainPtr domain, const struct sexpr *root) state = VIR_DOMAIN_BLOCKED; else if (strchr(flags, 'r')) state = VIR_DOMAIN_RUNNING; - } else if (domain->id < 0 || sexpr_int(root, "domain/status") == 0) { + } else if (def->id < 0 || sexpr_int(root, "domain/status") == 0) { /* As far as I can see the domain->id is a bad sign for checking * inactive domains as this is inaccurate after the domain has * been running once. However domain/status from xend seems to @@ -933,13 +933,13 @@ sexpr_to_xend_domain_state(virDomainPtr domain, const struct sexpr *root) * Returns 0 in case of success, -1 in case of error */ static int -sexpr_to_xend_domain_info(virDomainPtr domain, +sexpr_to_xend_domain_info(virDomainDefPtr def, const struct sexpr *root, virDomainInfoPtr info) { int vcpus;

- info->state = sexpr_to_xend_domain_state(domain, root); + info->state = sexpr_to_xend_domain_state(def, root); info->memory = sexpr_u64(root, "domain/memory") << 10; info->maxMem = sexpr_u64(root, "domain/maxmem") << 10; info->cpuTime = sexpr_float(root, "domain/cpu_time") * 1000000000; @@ -1374,13 +1374,14 @@ xenDaemonDomainDestroy(virConnectPtr conn, virDomainDefPtr def) * freed by the caller. */ char * -xenDaemonDomainGetOSType(virDomainPtr domain) +xenDaemonDomainGetOSType(virConnectPtr conn, + virDomainDefPtr def)

Fits on one line.

{ char *type; struct sexpr *root;

/* can we ask for a subset ? worth it ? */ - root = sexpr_get(domain->conn, "/xend/domain/%s?detail=1", domain->name); + root = sexpr_get(conn, "/xend/domain/%s?detail=1", def->name); if (root == NULL) return NULL;

@@ -1489,13 +1490,13 @@ xenDaemonDomainRestore(virConnectPtr conn, const char *filename) * Returns the memory size in kilobytes or 0 in case of error. */ unsigned long long -xenDaemonDomainGetMaxMemory(virDomainPtr domain) +xenDaemonDomainGetMaxMemory(virConnectPtr conn, virDomainDefPtr def) { unsigned long long ret = 0; struct sexpr *root;

/* can we ask for a subset ? worth it ? */ - root = sexpr_get(domain->conn, "/xend/domain/%s?detail=1", domain->name); + root = sexpr_get(conn, "/xend/domain/%s?detail=1", def->name); if (root == NULL) return 0;

@@ -1518,12 +1519,14 @@ xenDaemonDomainGetMaxMemory(virDomainPtr domain) * Returns 0 for success; -1 (with errno) on error */ int -xenDaemonDomainSetMaxMemory(virDomainPtr domain, unsigned long memory) +xenDaemonDomainSetMaxMemory(virConnectPtr conn, + virDomainDefPtr def, + unsigned long memory) { char buf[1024];

snprintf(buf, sizeof(buf), "%lu", VIR_DIV_UP(memory, 1024)); - return xend_op(domain->conn, domain->name, "op", "maxmem_set", "memory", + return xend_op(conn, def->name, "op", "maxmem_set", "memory", buf, NULL);

Can be condensed to one line here.

}

@@ -1544,12 +1547,14 @@ xenDaemonDomainSetMaxMemory(virDomainPtr domain, unsigned long memory) * Returns 0 for success; -1 (with errno) on error */ int -xenDaemonDomainSetMemory(virDomainPtr domain, unsigned long memory) +xenDaemonDomainSetMemory(virConnectPtr conn, + virDomainDefPtr def, + unsigned long memory) { char buf[1024];

snprintf(buf, sizeof(buf), "%lu", VIR_DIV_UP(memory, 1024)); - return xend_op(domain->conn, domain->name, "op", "mem_target_set", + return xend_op(conn, def->name, "op", "mem_target_set", "target", buf, NULL);

And here at exactly 80 columns :).

}

@@ -1640,16 +1645,18 @@ xenDaemonDomainGetXMLDesc(virDomainPtr domain, * Returns 0 in case of success, -1 in case of error */ int -xenDaemonDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info) +xenDaemonDomainGetInfo(virConnectPtr conn, + virDomainDefPtr def, + virDomainInfoPtr info) { struct sexpr *root; int ret;

- root = sexpr_get(domain->conn, "/xend/domain/%s?detail=1", domain->name); + root = sexpr_get(conn, "/xend/domain/%s?detail=1", def->name); if (root == NULL) return -1;

- ret = sexpr_to_xend_domain_info(domain, root, info); + ret = sexpr_to_xend_domain_info(def, root, info); sexpr_free(root); return ret; } @@ -1666,17 +1673,18 @@ xenDaemonDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info) * Returns 0 in case of success, -1 in case of error */ int -xenDaemonDomainGetState(virDomainPtr domain, +xenDaemonDomainGetState(virConnectPtr conn, + virDomainDefPtr def, int *state, int *reason) { struct sexpr *root;

- root = sexpr_get(domain->conn, "/xend/domain/%s?detail=1", domain->name); + root = sexpr_get(conn, "/xend/domain/%s?detail=1", def->name); if (!root) return -1;

- *state = sexpr_to_xend_domain_state(domain, root); + *state = sexpr_to_xend_domain_state(def, root); if (reason) *reason = 0;

diff --git a/src/xen/xend_internal.h b/src/xen/xend_internal.h index 4a6b406..87e0a0f 100644 --- a/src/xen/xend_internal.h +++ b/src/xen/xend_internal.h @@ -96,18 +96,27 @@ int xenDaemonDomainSave(virDomainPtr domain, const char *filename); int xenDaemonDomainCoreDump(virDomainPtr domain, const char *filename, unsigned int flags); int xenDaemonDomainRestore(virConnectPtr conn, const char *filename); -int xenDaemonDomainSetMemory(virDomainPtr domain, unsigned long memory); -int xenDaemonDomainSetMaxMemory(virDomainPtr domain, unsigned long memory); -int xenDaemonDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info); -int xenDaemonDomainGetState(virDomainPtr domain, +int xenDaemonDomainSetMemory(virConnectPtr conn, + virDomainDefPtr def, + unsigned long memory); +int xenDaemonDomainSetMaxMemory(virConnectPtr conn, + virDomainDefPtr def, + unsigned long memory); +int xenDaemonDomainGetInfo(virConnectPtr conn, + virDomainDefPtr def, + virDomainInfoPtr info); +int xenDaemonDomainGetState(virConnectPtr conn, + virDomainDefPtr def, int *state, int *reason); char *xenDaemonDomainGetXMLDesc(virDomainPtr domain, unsigned int flags, const char *cpus); -unsigned long long xenDaemonDomainGetMaxMemory(virDomainPtr domain); +unsigned long long xenDaemonDomainGetMaxMemory(virConnectPtr conn, + virDomainDefPtr def); char **xenDaemonListDomainsOld(virConnectPtr xend);

-char *xenDaemonDomainGetOSType(virDomainPtr domain); +char *xenDaemonDomainGetOSType(virConnectPtr conn, + virDomainDefPtr def);

Still fits on one line.

int xenDaemonNumOfDefinedDomains(virConnectPtr conn); int xenDaemonListDefinedDomains(virConnectPtr conn, diff --git a/src/xen/xm_internal.c b/src/xen/xm_internal.c index f4e8ea0..ce44e39 100644 --- a/src/xen/xm_internal.c +++ b/src/xen/xm_internal.c @@ -446,7 +446,8 @@ xenXMClose(virConnectPtr conn) * Since these are all offline domains, the state is always SHUTOFF. */ int -xenXMDomainGetState(virDomainPtr domain ATTRIBUTE_UNUSED, +xenXMDomainGetState(virConnectPtr conn ATTRIBUTE_UNUSED, + virDomainDefPtr def ATTRIBUTE_UNUSED, int *state, int *reason) { @@ -463,15 +464,17 @@ xenXMDomainGetState(virDomainPtr domain ATTRIBUTE_UNUSED, * VCPUs and memory. */ int -xenXMDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info) +xenXMDomainGetInfo(virConnectPtr conn, + virDomainDefPtr def, + virDomainInfoPtr info) { - xenUnifiedPrivatePtr priv = domain->conn->privateData; + xenUnifiedPrivatePtr priv = conn->privateData; const char *filename; xenXMConfCachePtr entry;

xenUnifiedLock(priv);

- if (!(filename = virHashLookup(priv->nameConfigMap, domain->name))) + if (!(filename = virHashLookup(priv->nameConfigMap, def->name))) goto error;

if (!(entry = virHashLookup(priv->configCache, filename))) @@ -530,9 +533,11 @@ cleanup: * Update amount of memory in the config file */ int -xenXMDomainSetMemory(virDomainPtr domain, unsigned long memory) +xenXMDomainSetMemory(virConnectPtr conn, + virDomainDefPtr def, + unsigned long memory) { - xenUnifiedPrivatePtr priv = domain->conn->privateData; + xenUnifiedPrivatePtr priv = conn->privateData; const char *filename; xenXMConfCachePtr entry; int ret = -1; @@ -546,7 +551,7 @@ xenXMDomainSetMemory(virDomainPtr domain, unsigned long memory)

xenUnifiedLock(priv);

- if (!(filename = virHashLookup(priv->nameConfigMap, domain->name))) + if (!(filename = virHashLookup(priv->nameConfigMap, def->name))) goto cleanup;

if (!(entry = virHashLookup(priv->configCache, filename))) @@ -559,7 +564,7 @@ xenXMDomainSetMemory(virDomainPtr domain, unsigned long memory) /* If this fails, should we try to undo our changes to the * in-memory representation of the config file. I say not! */ - if (xenXMConfigSaveFile(domain->conn, entry->filename, entry->def) < 0) + if (xenXMConfigSaveFile(conn, entry->filename, entry->def) < 0) goto cleanup; ret = 0;

@@ -572,9 +577,11 @@ cleanup: * Update maximum memory limit in config */ int -xenXMDomainSetMaxMemory(virDomainPtr domain, unsigned long memory) +xenXMDomainSetMaxMemory(virConnectPtr conn, + virDomainDefPtr def, + unsigned long memory) { - xenUnifiedPrivatePtr priv = domain->conn->privateData; + xenUnifiedPrivatePtr priv = conn->privateData; const char *filename; xenXMConfCachePtr entry; int ret = -1; @@ -588,7 +595,7 @@ xenXMDomainSetMaxMemory(virDomainPtr domain, unsigned long memory)

xenUnifiedLock(priv);

- if (!(filename = virHashLookup(priv->nameConfigMap, domain->name))) + if (!(filename = virHashLookup(priv->nameConfigMap, def->name))) goto cleanup;

if (!(entry = virHashLookup(priv->configCache, filename))) @@ -601,7 +608,7 @@ xenXMDomainSetMaxMemory(virDomainPtr domain, unsigned long memory) /* If this fails, should we try to undo our changes to the * in-memory representation of the config file. I say not! */ - if (xenXMConfigSaveFile(domain->conn, entry->filename, entry->def) < 0) + if (xenXMConfigSaveFile(conn, entry->filename, entry->def) < 0) goto cleanup; ret = 0;

@@ -614,19 +621,17 @@ cleanup: * Get max memory limit from config */ unsigned long long -xenXMDomainGetMaxMemory(virDomainPtr domain) +xenXMDomainGetMaxMemory(virConnectPtr conn, + virDomainDefPtr def)

Still fits on one line. ACK, pending answer to question above about xenDaemonDomainGetOSType vs xenHypervisorDomainGetOSType. Regards, Jim

{ - xenUnifiedPrivatePtr priv = domain->conn->privateData; + xenUnifiedPrivatePtr priv = conn->privateData; const char *filename; xenXMConfCachePtr entry; unsigned long long ret = 0;

- if (domain->id != -1) - return 0; - xenUnifiedLock(priv);

- if (!(filename = virHashLookup(priv->nameConfigMap, domain->name))) + if (!(filename = virHashLookup(priv->nameConfigMap, def->name))) goto cleanup;

if (!(entry = virHashLookup(priv->configCache, filename))) diff --git a/src/xen/xm_internal.h b/src/xen/xm_internal.h index 0e55897..0ae32bc 100644 --- a/src/xen/xm_internal.h +++ b/src/xen/xm_internal.h @@ -37,14 +37,22 @@ int xenXMConfigCacheRemoveFile(virConnectPtr conn, const char *filename); int xenXMOpen(virConnectPtr conn, virConnectAuthPtr auth, unsigned int flags); int xenXMClose(virConnectPtr conn); const char *xenXMGetType(virConnectPtr conn); -int xenXMDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info); -int xenXMDomainGetState(virDomainPtr domain, +int xenXMDomainGetInfo(virConnectPtr conn, + virDomainDefPtr def, + virDomainInfoPtr info); +int xenXMDomainGetState(virConnectPtr conn, + virDomainDefPtr def, int *state, int *reason); char *xenXMDomainGetXMLDesc(virDomainPtr domain, unsigned int flags); -int xenXMDomainSetMemory(virDomainPtr domain, unsigned long memory); -int xenXMDomainSetMaxMemory(virDomainPtr domain, unsigned long memory); -unsigned long long xenXMDomainGetMaxMemory(virDomainPtr domain); +int xenXMDomainSetMemory(virConnectPtr conn, + virDomainDefPtr def, + unsigned long memory); +int xenXMDomainSetMaxMemory(virConnectPtr conn, + virDomainDefPtr def, + unsigned long memory); +unsigned long long xenXMDomainGetMaxMemory(virConnectPtr conn, + virDomainDefPtr def); int xenXMDomainSetVcpus(virDomainPtr domain, unsigned int vcpus); int xenXMDomainSetVcpusFlags(virDomainPtr domain, unsigned int vcpus, unsigned int flags);

On Mon, May 20, 2013 at 11:40:34AM -0600, Jim Fehlig wrote:

I finally have some time to continue reviewing this series...

Daniel P. Berrange wrote:

...

From: "Daniel P. Berrange" <berrange@redhat.com>

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?

Opps bad description. I tried to make each patch as small as possible. So where I grouped several methods together, I only did it because they were mutually dependent wrt the refactoring. Will fix the description of this commit.

...

@@ -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.

As you've probably seen by now, I've preferred to split the lines when there are > 1 parameter in the method, even if it would technically fit on one line. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On Mon, May 20, 2013 at 12:14:04PM -0600, Jim Fehlig wrote:

Daniel P. Berrange wrote:

...

From: "Daniel P. Berrange" <berrange@redhat.com>

Introduce use of a virDomainDefPtr in the domain VCPU 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@redhat.com> --- src/xen/xen_driver.c | 84 ++++++++++++++++++++++++++++++++++++++++-------- src/xen/xen_hypervisor.c | 42 ++++++++++++------------ src/xen/xen_hypervisor.h | 9 ++++-- src/xen/xend_internal.c | 81 ++++++++++++++++++++++++++-------------------- src/xen/xend_internal.h | 17 ++++++---- src/xen/xm_internal.c | 30 +++++++++-------- src/xen/xm_internal.h | 19 ++++++++--- 7 files changed, 187 insertions(+), 95 deletions(-)

diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index 8b7dec9..04cb69d 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -647,11 +647,30 @@ xenUnifiedConnectNumOfDomains(virConnectPtr conn)

static virDomainPtr xenUnifiedDomainCreateXML(virConnectPtr conn, - const char *xmlDesc, unsigned int flags) + const char *xml, + unsigned int flags) { + xenUnifiedPrivatePtr priv = conn->privateData; + virDomainDefPtr def = NULL; + virDomainPtr ret = NULL; + virCheckFlags(0, NULL);

- return xenDaemonCreateXML(conn, xmlDesc); + if (!(def = virDomainDefParseString(xml, priv->caps, priv->xmlopt, + 1 << VIR_DOMAIN_VIRT_XEN, + VIR_DOMAIN_XML_INACTIVE))) + goto cleanup; + + if (xenDaemonCreateXML(conn, def) < 0) + goto cleanup; + + ret = virGetDomain(conn, def->name, def->uuid); + if (ret) + ret->id = def->id; + +cleanup: + virDomainDefFree(def); + return ret; }

Should this hunk be in patch 2? Or perhaps it was meant for patch 5?

Hmm, actually, yes, it should be in the previous patch. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

Daniel P. Berrange wrote:

From: "Daniel P. Berrange" <berrange@redhat.com>

Introduce use of a virDomainDefPtr in the domain hotplug 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@redhat.com> --- src/xen/xen_driver.c | 64 +++++++++++++++++++++++++++++++------ src/xen/xend_internal.c | 85 ++++++++++++++++++++++++++----------------------- src/xen/xend_internal.h | 10 ++++-- src/xen/xm_internal.c | 22 +++++++------ src/xen/xm_internal.h | 6 ++-- 5 files changed, 122 insertions(+), 65 deletions(-)

diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index 04cb69d..f5f6407 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -1695,6 +1695,8 @@ xenUnifiedDomainAttachDevice(virDomainPtr dom, const char *xml) { xenUnifiedPrivatePtr priv = dom->conn->privateData; unsigned int flags = VIR_DOMAIN_DEVICE_MODIFY_LIVE; + virDomainDefPtr def = NULL; + int ret = -1;

/* * HACK: xend with xendConfigVersion >= 3 does not support changing live @@ -1704,12 +1706,17 @@ xenUnifiedDomainAttachDevice(virDomainPtr dom, const char *xml) if (priv->xendConfigVersion >= XEND_CONFIG_VERSION_3_0_4) flags |= VIR_DOMAIN_DEVICE_MODIFY_CONFIG;

+ if (!(def = xenGetDomainDefForDom(dom))) + goto cleanup; + if (dom->id < 0 && priv->xendConfigVersion < XEND_CONFIG_VERSION_3_0_4) - return xenXMDomainAttachDeviceFlags(dom, xml, flags); + ret = xenXMDomainAttachDeviceFlags(dom->conn, def, xml, flags); else - return xenDaemonAttachDeviceFlags(dom, xml, flags); + ret = xenDaemonAttachDeviceFlags(dom->conn, def, xml, flags);

- return -1; +cleanup: + virDomainDefFree(def); + return ret; }

static int @@ -1717,11 +1724,20 @@ xenUnifiedDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, unsigned int flags) { xenUnifiedPrivatePtr priv = dom->conn->privateData; + virDomainDefPtr def = NULL; + int ret = -1; + + if (!(def = xenGetDomainDefForDom(dom))) + goto cleanup;

if (dom->id < 0 && priv->xendConfigVersion < XEND_CONFIG_VERSION_3_0_4) - return xenXMDomainAttachDeviceFlags(dom, xml, flags); + ret = xenXMDomainAttachDeviceFlags(dom->conn, def, xml, flags); else - return xenDaemonAttachDeviceFlags(dom, xml, flags); + ret = xenDaemonAttachDeviceFlags(dom->conn, def, xml, flags); + +cleanup: + virDomainDefFree(def); + return ret; }

static int @@ -1729,6 +1745,8 @@ xenUnifiedDomainDetachDevice(virDomainPtr dom, const char *xml) { xenUnifiedPrivatePtr priv = dom->conn->privateData; unsigned int flags = VIR_DOMAIN_DEVICE_MODIFY_LIVE; + virDomainDefPtr def = NULL; + int ret = -1;

/* * HACK: xend with xendConfigVersion >= 3 does not support changing live @@ -1738,10 +1756,17 @@ xenUnifiedDomainDetachDevice(virDomainPtr dom, const char *xml) if (priv->xendConfigVersion >= XEND_CONFIG_VERSION_3_0_4) flags |= VIR_DOMAIN_DEVICE_MODIFY_CONFIG;

+ if (!(def = xenGetDomainDefForDom(dom))) + goto cleanup; + if (dom->id < 0 && priv->xendConfigVersion < XEND_CONFIG_VERSION_3_0_4) - return xenXMDomainDetachDeviceFlags(dom, xml, flags); + ret = xenXMDomainDetachDeviceFlags(dom->conn, def, xml, flags); else - return xenDaemonDetachDeviceFlags(dom, xml, flags); + ret = xenDaemonDetachDeviceFlags(dom->conn, def, xml, flags); + +cleanup: + virDomainDefFree(def); + return ret; }

static int @@ -1749,18 +1774,37 @@ xenUnifiedDomainDetachDeviceFlags(virDomainPtr dom, const char *xml, unsigned int flags) { xenUnifiedPrivatePtr priv = dom->conn->privateData; + virDomainDefPtr def = NULL; + int ret = -1; + + if (!(def = xenGetDomainDefForDom(dom))) + goto cleanup;

if (dom->id < 0 && priv->xendConfigVersion < XEND_CONFIG_VERSION_3_0_4) - return xenXMDomainDetachDeviceFlags(dom, xml, flags); + ret = xenXMDomainDetachDeviceFlags(dom->conn, def, xml, flags); else - return xenDaemonDetachDeviceFlags(dom, xml, flags); + ret = xenDaemonDetachDeviceFlags(dom->conn, def, xml, flags); + +cleanup: + virDomainDefFree(def); + return ret; }

static int xenUnifiedDomainUpdateDeviceFlags(virDomainPtr dom, const char *xml, unsigned int flags) { - return xenDaemonUpdateDeviceFlags(dom, xml, flags); + virDomainDefPtr def = NULL; + int ret = -1; + + if (!(def = xenGetDomainDefForDom(dom))) + goto cleanup; + + ret = xenDaemonUpdateDeviceFlags(dom->conn, def, xml, flags); + +cleanup: + virDomainDefFree(def); + return ret; }

static int diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index 75c980c..2715a3e 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -60,7 +60,8 @@ #define XEND_RCV_BUF_MAX_LEN (256 * 1024)

static int -virDomainXMLDevID(virDomainPtr domain, virDomainDeviceDefPtr dev, char *class, +virDomainXMLDevID(virConnectPtr conn, virDomainDefPtr domain, + virDomainDeviceDefPtr dev, char *class, char *ref, int ref_len);

/** @@ -2202,11 +2203,12 @@ xenDaemonCreateXML(virConnectPtr conn, virDomainDefPtr def) * Returns 0 in case of success, -1 in case of failure. */ int -xenDaemonAttachDeviceFlags(virDomainPtr domain, +xenDaemonAttachDeviceFlags(virConnectPtr conn, + virDomainDefPtr minidef,

My usual comment about the function comments needing updated :). Here and below where the signatures change. ACK. Regards, Jim

const char *xml, unsigned int flags) { - xenUnifiedPrivatePtr priv = domain->conn->privateData; + xenUnifiedPrivatePtr priv = conn->privateData; char *sexpr = NULL; int ret = -1; virDomainDeviceDefPtr dev = NULL; @@ -2217,7 +2219,7 @@ xenDaemonAttachDeviceFlags(virDomainPtr domain,

virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1);

- if (domain->id < 0) { + if (minidef->id < 0) { /* Cannot modify live config if domain is inactive */ if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", @@ -2247,9 +2249,9 @@ xenDaemonAttachDeviceFlags(virDomainPtr domain, } }

- if (!(def = xenDaemonDomainFetch(domain->conn, - domain->id, - domain->name, + if (!(def = xenDaemonDomainFetch(conn, + minidef->id, + minidef->name, NULL))) goto cleanup;

@@ -2275,7 +2277,7 @@ xenDaemonAttachDeviceFlags(virDomainPtr domain, break;

case VIR_DOMAIN_DEVICE_NET: - if (xenFormatSxprNet(domain->conn, + if (xenFormatSxprNet(conn, dev->data.net, &buf, STREQ(def->os.type, "hvm") ? 1 : 0, @@ -2320,9 +2322,9 @@ xenDaemonAttachDeviceFlags(virDomainPtr domain,

sexpr = virBufferContentAndReset(&buf);

- if (virDomainXMLDevID(domain, dev, class, ref, sizeof(ref))) { + if (virDomainXMLDevID(conn, minidef, dev, class, ref, sizeof(ref))) { /* device doesn't exist, define it */ - ret = xend_op(domain->conn, domain->name, "op", "device_create", + ret = xend_op(conn, def->name, "op", "device_create", "config", sexpr, NULL); } else { if (dev->data.disk->device != VIR_DOMAIN_DISK_DEVICE_CDROM) { @@ -2330,7 +2332,7 @@ xenDaemonAttachDeviceFlags(virDomainPtr domain, _("target '%s' already exists"), target); } else { /* device exists, attempt to modify it */ - ret = xend_op(domain->conn, domain->name, "op", "device_configure", + ret = xend_op(conn, minidef->name, "op", "device_configure", "config", sexpr, "dev", ref, NULL); } } @@ -2355,11 +2357,12 @@ cleanup: * Returns 0 in case of success, -1 in case of failure. */ int -xenDaemonUpdateDeviceFlags(virDomainPtr domain, +xenDaemonUpdateDeviceFlags(virConnectPtr conn, + virDomainDefPtr minidef, const char *xml, unsigned int flags) { - xenUnifiedPrivatePtr priv = domain->conn->privateData; + xenUnifiedPrivatePtr priv = conn->privateData; char *sexpr = NULL; int ret = -1; virDomainDeviceDefPtr dev = NULL; @@ -2370,7 +2373,7 @@ xenDaemonUpdateDeviceFlags(virDomainPtr domain, virCheckFlags(VIR_DOMAIN_DEVICE_MODIFY_LIVE | VIR_DOMAIN_DEVICE_MODIFY_CONFIG, -1);

- if (domain->id < 0) { + if (minidef->id < 0) { /* Cannot modify live config if domain is inactive */ if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", @@ -2400,9 +2403,9 @@ xenDaemonUpdateDeviceFlags(virDomainPtr domain, } }

- if (!(def = xenDaemonDomainFetch(domain->conn, - domain->id, - domain->name, + if (!(def = xenDaemonDomainFetch(conn, + minidef->id, + minidef->name, NULL))) goto cleanup;

@@ -2428,13 +2431,13 @@ xenDaemonUpdateDeviceFlags(virDomainPtr domain,

sexpr = virBufferContentAndReset(&buf);

- if (virDomainXMLDevID(domain, dev, class, ref, sizeof(ref))) { + if (virDomainXMLDevID(conn, minidef, dev, class, ref, sizeof(ref))) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("requested device does not exist")); goto cleanup; } else { /* device exists, attempt to modify it */ - ret = xend_op(domain->conn, domain->name, "op", "device_configure", + ret = xend_op(conn, minidef->name, "op", "device_configure", "config", sexpr, "dev", ref, NULL); }

@@ -2456,11 +2459,12 @@ cleanup: * Returns 0 in case of success, -1 in case of failure. */ int -xenDaemonDetachDeviceFlags(virDomainPtr domain, +xenDaemonDetachDeviceFlags(virConnectPtr conn, + virDomainDefPtr minidef, const char *xml, unsigned int flags) { - xenUnifiedPrivatePtr priv = domain->conn->privateData; + xenUnifiedPrivatePtr priv = conn->privateData; char class[8], ref[80]; virDomainDeviceDefPtr dev = NULL; virDomainDefPtr def = NULL; @@ -2470,7 +2474,7 @@ xenDaemonDetachDeviceFlags(virDomainPtr domain,

virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1);

- if (domain->id < 0) { + if (minidef->id < 0) { /* Cannot modify live config if domain is inactive */ if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", @@ -2500,9 +2504,9 @@ xenDaemonDetachDeviceFlags(virDomainPtr domain, } }

- if (!(def = xenDaemonDomainFetch(domain->conn, - domain->id, - domain->name, + if (!(def = xenDaemonDomainFetch(conn, + minidef->id, + minidef->name, NULL))) goto cleanup;

@@ -2510,7 +2514,7 @@ xenDaemonDetachDeviceFlags(virDomainPtr domain, VIR_DOMAIN_XML_INACTIVE))) goto cleanup;

- if (virDomainXMLDevID(domain, dev, class, ref, sizeof(ref))) + if (virDomainXMLDevID(conn, minidef, dev, class, ref, sizeof(ref))) goto cleanup;

if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV) { @@ -2524,12 +2528,12 @@ xenDaemonDetachDeviceFlags(virDomainPtr domain, goto cleanup; } xendev = virBufferContentAndReset(&buf); - ret = xend_op(domain->conn, domain->name, "op", "device_configure", + ret = xend_op(conn, minidef->name, "op", "device_configure", "config", xendev, "dev", ref, NULL); VIR_FREE(xendev); } else { - ret = xend_op(domain->conn, domain->name, "op", "device_destroy", + ret = xend_op(conn, minidef->name, "op", "device_destroy", "type", class, "dev", ref, "force", "0", "rm_cfg", "1", NULL); } @@ -3334,13 +3338,14 @@ xenDaemonDomainBlockPeek(virDomainPtr domain, * Returns 0 in case of success, -1 in case of failure. */ static int -virDomainXMLDevID(virDomainPtr domain, +virDomainXMLDevID(virConnectPtr conn, + virDomainDefPtr def, virDomainDeviceDefPtr dev, char *class, char *ref, int ref_len) { - xenUnifiedPrivatePtr priv = domain->conn->privateData; + xenUnifiedPrivatePtr priv = conn->privateData; char *xref; char *tmp;

@@ -3357,7 +3362,7 @@ virDomainXMLDevID(virDomainPtr domain, if (dev->data.disk->dst == NULL) return -1; xenUnifiedLock(priv); - xref = xenStoreDomainGetDiskID(domain->conn, domain->id, + xref = xenStoreDomainGetDiskID(conn, def->id, dev->data.disk->dst); xenUnifiedUnlock(priv); if (xref == NULL) @@ -3369,13 +3374,13 @@ virDomainXMLDevID(virDomainPtr domain, return -1; } else if (dev->type == VIR_DOMAIN_DEVICE_NET) { char mac[VIR_MAC_STRING_BUFLEN]; - virDomainNetDefPtr def = dev->data.net; - virMacAddrFormat(&def->mac, mac); + virDomainNetDefPtr netdef = dev->data.net; + virMacAddrFormat(&netdef->mac, mac);

strcpy(class, "vif");

xenUnifiedLock(priv); - xref = xenStoreDomainGetNetworkID(domain->conn, domain->id, mac); + xref = xenStoreDomainGetNetworkID(conn, def->id, mac); xenUnifiedUnlock(priv); if (xref == NULL) return -1; @@ -3388,13 +3393,13 @@ virDomainXMLDevID(virDomainPtr domain, dev->data.hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && dev->data.hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) { char *bdf; - virDomainHostdevDefPtr def = dev->data.hostdev; + virDomainHostdevDefPtr hostdef = dev->data.hostdev;

if (virAsprintf(&bdf, "%04x:%02x:%02x.%0x", - def->source.subsys.u.pci.addr.domain, - def->source.subsys.u.pci.addr.bus, - def->source.subsys.u.pci.addr.slot, - def->source.subsys.u.pci.addr.function) < 0) { + hostdef->source.subsys.u.pci.addr.domain, + hostdef->source.subsys.u.pci.addr.bus, + hostdef->source.subsys.u.pci.addr.slot, + hostdef->source.subsys.u.pci.addr.function) < 0) { virReportOOMError(); return -1; } @@ -3402,7 +3407,7 @@ virDomainXMLDevID(virDomainPtr domain, strcpy(class, "pci");

xenUnifiedLock(priv); - xref = xenStoreDomainGetPCIID(domain->conn, domain->id, bdf); + xref = xenStoreDomainGetPCIID(conn, def->id, bdf); xenUnifiedUnlock(priv); VIR_FREE(bdf); if (xref == NULL) diff --git a/src/xen/xend_internal.h b/src/xen/xend_internal.h index b78145c..62b85ef 100644 --- a/src/xen/xend_internal.h +++ b/src/xen/xend_internal.h @@ -126,10 +126,12 @@ int xenDaemonListDefinedDomains(virConnectPtr conn, char **const names, int maxnames);

-int xenDaemonAttachDeviceFlags(virDomainPtr domain, +int xenDaemonAttachDeviceFlags(virConnectPtr conn, + virDomainDefPtr def, const char *xml, unsigned int flags); -int xenDaemonDetachDeviceFlags(virDomainPtr domain, +int xenDaemonDetachDeviceFlags(virConnectPtr conn, + virDomainDefPtr def, const char *xml, unsigned int flags);

@@ -161,7 +163,9 @@ int xenDaemonDomainGetVcpus (virConnectPtr conn, int maxinfo, unsigned char *cpumaps, int maplen); -int xenDaemonUpdateDeviceFlags(virDomainPtr domain, const char *xml, +int xenDaemonUpdateDeviceFlags(virConnectPtr conn, + virDomainDefPtr def, + const char *xml, unsigned int flags); int xenDaemonDomainGetAutostart (virDomainPtr dom, int *autostart); diff --git a/src/xen/xm_internal.c b/src/xen/xm_internal.c index 76425dd..c2d9915 100644 --- a/src/xen/xm_internal.c +++ b/src/xen/xm_internal.c @@ -1219,7 +1219,8 @@ cleanup: * Returns 0 in case of success, -1 in case of failure. */ int -xenXMDomainAttachDeviceFlags(virDomainPtr domain, +xenXMDomainAttachDeviceFlags(virConnectPtr conn, + virDomainDefPtr minidef, const char *xml, unsigned int flags) { @@ -1228,12 +1229,12 @@ xenXMDomainAttachDeviceFlags(virDomainPtr domain, int ret = -1; virDomainDeviceDefPtr dev = NULL; virDomainDefPtr def; - xenUnifiedPrivatePtr priv = domain->conn->privateData; + xenUnifiedPrivatePtr priv = conn->privateData;

virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1);

if ((flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) || - (domain->id != -1 && flags == VIR_DOMAIN_DEVICE_MODIFY_CURRENT)) { + (minidef->id != -1 && flags == VIR_DOMAIN_DEVICE_MODIFY_CURRENT)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("Xm driver only supports modifying persistent config")); return -1; @@ -1241,7 +1242,7 @@ xenXMDomainAttachDeviceFlags(virDomainPtr domain,

xenUnifiedLock(priv);

- if (!(filename = virHashLookup(priv->nameConfigMap, domain->name))) + if (!(filename = virHashLookup(priv->nameConfigMap, minidef->name))) goto cleanup; if (!(entry = virHashLookup(priv->configCache, filename))) goto cleanup; @@ -1284,7 +1285,7 @@ xenXMDomainAttachDeviceFlags(virDomainPtr domain, /* If this fails, should we try to undo our changes to the * in-memory representation of the config file. I say not! */ - if (xenXMConfigSaveFile(domain->conn, entry->filename, entry->def) < 0) + if (xenXMConfigSaveFile(conn, entry->filename, entry->def) < 0) goto cleanup;

ret = 0; @@ -1309,7 +1310,8 @@ xenXMDomainAttachDeviceFlags(virDomainPtr domain, * Returns 0 in case of success, -1 in case of failure. */ int -xenXMDomainDetachDeviceFlags(virDomainPtr domain, +xenXMDomainDetachDeviceFlags(virConnectPtr conn, + virDomainDefPtr minidef, const char *xml, unsigned int flags) { @@ -1319,12 +1321,12 @@ xenXMDomainDetachDeviceFlags(virDomainPtr domain, virDomainDefPtr def; int ret = -1; int i; - xenUnifiedPrivatePtr priv = domain->conn->privateData; + xenUnifiedPrivatePtr priv = conn->privateData;

virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1);

if ((flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) || - (domain->id != -1 && flags == VIR_DOMAIN_DEVICE_MODIFY_CURRENT)) { + (minidef->id != -1 && flags == VIR_DOMAIN_DEVICE_MODIFY_CURRENT)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("Xm driver only supports modifying persistent config")); return -1; @@ -1332,7 +1334,7 @@ xenXMDomainDetachDeviceFlags(virDomainPtr domain,

xenUnifiedLock(priv);

- if (!(filename = virHashLookup(priv->nameConfigMap, domain->name))) + if (!(filename = virHashLookup(priv->nameConfigMap, minidef->name))) goto cleanup; if (!(entry = virHashLookup(priv->configCache, filename))) goto cleanup; @@ -1390,7 +1392,7 @@ xenXMDomainDetachDeviceFlags(virDomainPtr domain, /* If this fails, should we try to undo our changes to the * in-memory representation of the config file. I say not! */ - if (xenXMConfigSaveFile(domain->conn, entry->filename, entry->def) < 0) + if (xenXMConfigSaveFile(conn, entry->filename, entry->def) < 0) goto cleanup;

ret = 0; diff --git a/src/xen/xm_internal.h b/src/xen/xm_internal.h index 28087d3..7d64dc6 100644 --- a/src/xen/xm_internal.h +++ b/src/xen/xm_internal.h @@ -85,11 +85,13 @@ int xenXMDomainBlockPeek (virDomainPtr dom, const char *path, unsigned long long int xenXMDomainGetAutostart(virDomainPtr dom, int *autostart); int xenXMDomainSetAutostart(virDomainPtr dom, int autostart);

-int xenXMDomainAttachDeviceFlags(virDomainPtr domain, +int xenXMDomainAttachDeviceFlags(virConnectPtr conn, + virDomainDefPtr def, const char *xml, unsigned int flags);

-int xenXMDomainDetachDeviceFlags(virDomainPtr domain, +int xenXMDomainDetachDeviceFlags(virConnectPtr conn, + virDomainDefPtr def, const char *xml, unsigned int flags);

Daniel P. Berrange wrote:

From: "Daniel P. Berrange" <berrange@redhat.com>

Introduce use of a virDomainDefPtr in the domain autostart 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@redhat.com> --- src/xen/xen_driver.c | 26 ++++++++++++++++++++++---- src/xen/xend_internal.c | 14 +++++++++----- src/xen/xend_internal.h | 10 ++++++---- src/xen/xm_internal.c | 22 ++++++++++++---------- src/xen/xm_internal.h | 6 ++++-- 5 files changed, 53 insertions(+), 25 deletions(-)

ACK. Regards, Jim

diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index f5f6407..43b3020 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -1811,22 +1811,40 @@ static int xenUnifiedDomainGetAutostart(virDomainPtr dom, int *autostart) { xenUnifiedPrivatePtr priv = dom->conn->privateData; + virDomainDefPtr def = NULL; + int ret = -1; + + if (!(def = xenGetDomainDefForDom(dom))) + goto cleanup;

if (priv->xendConfigVersion < XEND_CONFIG_VERSION_3_0_4) - return xenXMDomainGetAutostart(dom, autostart); + ret = xenXMDomainGetAutostart(def, autostart); else - return xenDaemonDomainGetAutostart(dom, autostart); + ret = xenDaemonDomainGetAutostart(dom->conn, def, autostart); + +cleanup: + virDomainDefFree(def); + return ret; }

static int xenUnifiedDomainSetAutostart(virDomainPtr dom, int autostart) { xenUnifiedPrivatePtr priv = dom->conn->privateData; + virDomainDefPtr def = NULL; + int ret = -1; + + if (!(def = xenGetDomainDefForDom(dom))) + goto cleanup;

if (priv->xendConfigVersion < XEND_CONFIG_VERSION_3_0_4) - return xenXMDomainSetAutostart(dom, autostart); + ret = xenXMDomainSetAutostart(def, autostart); else - return xenDaemonDomainSetAutostart(dom, autostart); + ret = xenDaemonDomainSetAutostart(dom->conn, def, autostart); + +cleanup: + virDomainDefFree(def); + return ret; }

static char * diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index 2715a3e..3d852d2 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -2546,12 +2546,14 @@ cleanup: }

int -xenDaemonDomainGetAutostart(virDomainPtr domain, int *autostart) +xenDaemonDomainGetAutostart(virConnectPtr conn, + virDomainDefPtr def, + int *autostart) { struct sexpr *root; const char *tmp;

- root = sexpr_get(domain->conn, "/xend/domain/%s?detail=1", domain->name); + root = sexpr_get(conn, "/xend/domain/%s?detail=1", def->name); if (root == NULL) { virReportError(VIR_ERR_XEN_CALL, "%s", _("xenDaemonGetAutostart failed to find this domain")); @@ -2570,14 +2572,16 @@ xenDaemonDomainGetAutostart(virDomainPtr domain, int *autostart) }

int -xenDaemonDomainSetAutostart(virDomainPtr domain, int autostart) +xenDaemonDomainSetAutostart(virConnectPtr conn, + virDomainDefPtr def, + int autostart) { struct sexpr *root, *autonode; virBuffer buffer = VIR_BUFFER_INITIALIZER; char *content = NULL; int ret = -1;

- root = sexpr_get(domain->conn, "/xend/domain/%s?detail=1", domain->name); + root = sexpr_get(conn, "/xend/domain/%s?detail=1", def->name); if (root == NULL) { virReportError(VIR_ERR_XEN_CALL, "%s", _("xenDaemonSetAutostart failed to find this domain")); @@ -2616,7 +2620,7 @@ xenDaemonDomainSetAutostart(virDomainPtr domain, int autostart)

content = virBufferContentAndReset(&buffer);

- if (xend_op(domain->conn, "", "op", "new", "config", content, NULL) != 0) { + if (xend_op(conn, "", "op", "new", "config", content, NULL) != 0) { virReportError(VIR_ERR_XEN_CALL, "%s", _("Failed to redefine sexpr")); goto error; diff --git a/src/xen/xend_internal.h b/src/xen/xend_internal.h index 62b85ef..3a7c0ac 100644 --- a/src/xen/xend_internal.h +++ b/src/xen/xend_internal.h @@ -167,10 +167,12 @@ int xenDaemonUpdateDeviceFlags(virConnectPtr conn, virDomainDefPtr def, const char *xml, unsigned int flags); -int xenDaemonDomainGetAutostart (virDomainPtr dom, - int *autostart); -int xenDaemonDomainSetAutostart (virDomainPtr domain, - int autostart); +int xenDaemonDomainGetAutostart(virConnectPtr conn, + virDomainDefPtr def, + int *autostart); +int xenDaemonDomainSetAutostart(virConnectPtr conn, + virDomainDefPtr def, + int autostart);

int xenDaemonCreateXML(virConnectPtr conn, virDomainDefPtr def); virDomainDefPtr xenDaemonLookupByUUID(virConnectPtr conn, const unsigned char *uuid); diff --git a/src/xen/xm_internal.c b/src/xen/xm_internal.c index c2d9915..bc98cf1 100644 --- a/src/xen/xm_internal.c +++ b/src/xen/xm_internal.c @@ -1417,28 +1417,29 @@ xenXMDomainBlockPeek(virDomainPtr dom ATTRIBUTE_UNUSED,

static char * -xenXMAutostartLinkName(virDomainPtr dom) +xenXMAutostartLinkName(virDomainDefPtr def) { char *ret; - if (virAsprintf(&ret, "/etc/xen/auto/%s", dom->name) < 0) + if (virAsprintf(&ret, "/etc/xen/auto/%s", def->name) < 0) return NULL; return ret; }

static char * -xenXMDomainConfigName(virDomainPtr dom) +xenXMDomainConfigName(virDomainDefPtr def) { char *ret; - if (virAsprintf(&ret, "/etc/xen/%s", dom->name) < 0) + if (virAsprintf(&ret, "/etc/xen/%s", def->name) < 0) return NULL; return ret; }

int -xenXMDomainGetAutostart(virDomainPtr dom, int *autostart) +xenXMDomainGetAutostart(virDomainDefPtr def, + int *autostart) { - char *linkname = xenXMAutostartLinkName(dom); - char *config = xenXMDomainConfigName(dom); + char *linkname = xenXMAutostartLinkName(def); + char *config = xenXMDomainConfigName(def); int ret = -1;

if (!linkname || !config) { @@ -1464,10 +1465,11 @@ cleanup:

int -xenXMDomainSetAutostart(virDomainPtr dom, int autostart) +xenXMDomainSetAutostart(virDomainDefPtr def, + int autostart) { - char *linkname = xenXMAutostartLinkName(dom); - char *config = xenXMDomainConfigName(dom); + char *linkname = xenXMAutostartLinkName(def); + char *config = xenXMDomainConfigName(def); int ret = -1;

if (!linkname || !config) { diff --git a/src/xen/xm_internal.h b/src/xen/xm_internal.h index 7d64dc6..78cd15c 100644 --- a/src/xen/xm_internal.h +++ b/src/xen/xm_internal.h @@ -82,8 +82,10 @@ int xenXMDomainUndefine(virConnectPtr conn, virDomainDefPtr def);

int xenXMDomainBlockPeek (virDomainPtr dom, const char *path, unsigned long long offset, size_t size, void *buffer);

-int xenXMDomainGetAutostart(virDomainPtr dom, int *autostart); -int xenXMDomainSetAutostart(virDomainPtr dom, int autostart); +int xenXMDomainGetAutostart(virDomainDefPtr def, + int *autostart); +int xenXMDomainSetAutostart(virDomainDefPtr def, + int autostart);

int xenXMDomainAttachDeviceFlags(virConnectPtr conn, virDomainDefPtr def,

Daniel P. Berrange wrote:

From: "Daniel P. Berrange" <berrange@redhat.com>

Introduce use of a virDomainDefPtr in the domain scheduler 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@redhat.com> --- src/xen/xen_driver.c | 45 ++++++++++++++++++++++++++++++++++++--------- src/xen/xen_hypervisor.c | 19 +++++++++++-------- src/xen/xen_hypervisor.h | 16 +++++++++------- src/xen/xend_internal.c | 27 +++++++++++++++------------ src/xen/xend_internal.h | 9 ++++++--- 5 files changed, 77 insertions(+), 39 deletions(-)

diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index 43b3020..5ab1a52 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -1851,17 +1851,26 @@ static char * xenUnifiedDomainGetSchedulerType(virDomainPtr dom, int *nparams) { xenUnifiedPrivatePtr priv = dom->conn->privateData; + virDomainDefPtr def = NULL; + char *ret = NULL; + + if (!(def = xenGetDomainDefForDom(dom))) + goto cleanup;

if (dom->id < 0) { if (priv->xendConfigVersion < XEND_CONFIG_VERSION_3_0_4) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Cannot change scheduler parameters")); - return NULL; + goto cleanup; } - return xenDaemonGetSchedulerType(dom, nparams); + ret = xenDaemonGetSchedulerType(dom->conn, nparams); } else { - return xenHypervisorGetSchedulerType(dom, nparams); + ret = xenHypervisorGetSchedulerType(dom->conn, nparams); } + +cleanup: + virDomainDefFree(def); + return ret; }

static int @@ -1871,19 +1880,28 @@ xenUnifiedDomainGetSchedulerParametersFlags(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 (dom->id < 0) { if (priv->xendConfigVersion < XEND_CONFIG_VERSION_3_0_4) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Cannot change scheduler parameters")); - return -1; + goto cleanup; } - return xenDaemonGetSchedulerParameters(dom, params, nparams); + ret = xenDaemonGetSchedulerParameters(dom->conn, def, params, nparams); } else { - return xenHypervisorGetSchedulerParameters(dom, params, nparams); + ret = xenHypervisorGetSchedulerParameters(dom->conn, def, params, nparams); } + +cleanup: + virDomainDefFree(def); + return ret; }

static int @@ -1902,19 +1920,28 @@ xenUnifiedDomainSetSchedulerParametersFlags(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 (dom->id < 0) { if (priv->xendConfigVersion < XEND_CONFIG_VERSION_3_0_4) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Cannot change scheduler parameters")); - return -1; + goto cleanup; } - return xenDaemonSetSchedulerParameters(dom, params, nparams); + ret = xenDaemonSetSchedulerParameters(dom->conn, def, params, nparams); } else { - return xenHypervisorSetSchedulerParameters(dom, params, nparams); + ret = xenHypervisorSetSchedulerParameters(dom->conn, def, params, nparams); } + +cleanup: + virDomainDefFree(def); + return ret; }

static int diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c index b97b329..2525566 100644 --- a/src/xen/xen_hypervisor.c +++ b/src/xen/xen_hypervisor.c @@ -1113,10 +1113,11 @@ virXen_getdomaininfo(int handle, int first_domain, xen_getdomaininfo *dominfo) * Returns scheduler name or NULL in case of failure */ char * -xenHypervisorGetSchedulerType(virDomainPtr domain, int *nparams) +xenHypervisorGetSchedulerType(virConnectPtr conn, + int *nparams)

Still fits on one line. And function comments need updated here and where similar changes are made below.

{ char *schedulertype = NULL; - xenUnifiedPrivatePtr priv = domain->conn->privateData; + xenUnifiedPrivatePtr priv = conn->privateData;

/* * Support only hv_versions.dom_interface >=5 @@ -1176,11 +1177,12 @@ xenHypervisorGetSchedulerType(virDomainPtr domain, int *nparams) * Returns 0 or -1 in case of failure */ int -xenHypervisorGetSchedulerParameters(virDomainPtr domain, +xenHypervisorGetSchedulerParameters(virConnectPtr conn, + virDomainDefPtr def, virTypedParameterPtr params, int *nparams) { - xenUnifiedPrivatePtr priv = domain->conn->privateData; + xenUnifiedPrivatePtr priv = conn->privateData;

/* * Support only hv_versions.dom_interface >=5 @@ -1218,7 +1220,7 @@ xenHypervisorGetSchedulerParameters(virDomainPtr domain, case XEN_SCHEDULER_CREDIT: memset(&op_dom, 0, sizeof(op_dom)); op_dom.cmd = XEN_V2_OP_SCHEDULER; - op_dom.domain = (domid_t) domain->id; + op_dom.domain = (domid_t) def->id; op_dom.u.getschedinfo.sched_id = XEN_SCHEDULER_CREDIT; op_dom.u.getschedinfo.cmd = XEN_DOMCTL_SCHEDOP_getinfo; ret = xenHypervisorDoV2Dom(priv->handle, &op_dom); @@ -1262,13 +1264,14 @@ xenHypervisorGetSchedulerParameters(virDomainPtr domain, * Returns 0 or -1 in case of failure */ int -xenHypervisorSetSchedulerParameters(virDomainPtr domain, +xenHypervisorSetSchedulerParameters(virConnectPtr conn, + virDomainDefPtr def, virTypedParameterPtr params, int nparams) { int i; unsigned int val; - xenUnifiedPrivatePtr priv = domain->conn->privateData; + xenUnifiedPrivatePtr priv = conn->privateData; char buf[256];

if (nparams == 0) { @@ -1313,7 +1316,7 @@ xenHypervisorSetSchedulerParameters(virDomainPtr domain, case XEN_SCHEDULER_CREDIT: { memset(&op_dom, 0, sizeof(op_dom)); op_dom.cmd = XEN_V2_OP_SCHEDULER; - op_dom.domain = (domid_t) domain->id; + op_dom.domain = (domid_t) def->id; op_dom.u.getschedinfo.sched_id = XEN_SCHEDULER_CREDIT; op_dom.u.getschedinfo.cmd = XEN_DOMCTL_SCHEDOP_putinfo;

diff --git a/src/xen/xen_hypervisor.h b/src/xen/xen_hypervisor.h index 1cf1e14..1e5bb67 100644 --- a/src/xen/xen_hypervisor.h +++ b/src/xen/xen_hypervisor.h @@ -106,18 +106,20 @@ int xenHypervisorGetVcpuMax (virConnectPtr conn, virDomainDefPtr def) ATTRIBUTE_NONNULL (1);

-char * xenHypervisorGetSchedulerType (virDomainPtr domain, +char * xenHypervisorGetSchedulerType (virConnectPtr conn, int *nparams)

Is it better to fix this non-standard whitespace as the code is touched, or with one cleanup patch? This code rarely gets touched, so maybe the latter.

ATTRIBUTE_NONNULL (1);

-int xenHypervisorGetSchedulerParameters(virDomainPtr domain, - virTypedParameterPtr params, - int *nparams) +int xenHypervisorGetSchedulerParameters(virConnectPtr conn, + virDomainDefPtr def, + virTypedParameterPtr params, + int *nparams) ATTRIBUTE_NONNULL (1);

-int xenHypervisorSetSchedulerParameters(virDomainPtr domain, - virTypedParameterPtr params, - int nparams) +int xenHypervisorSetSchedulerParameters(virConnectPtr conn, + virDomainDefPtr def, + virTypedParameterPtr params, + int nparams) ATTRIBUTE_NONNULL (1);

int xenHypervisorDomainBlockStats (virDomainPtr domain, diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index 3d852d2..3bcd19b 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -2972,9 +2972,10 @@ error: * caller or NULL in case of failure */ char * -xenDaemonGetSchedulerType(virDomainPtr domain, int *nparams) +xenDaemonGetSchedulerType(virConnectPtr conn, + int *nparams)

Fits on one line, and comment update here and below. ACK. Regards, Jim

{ - xenUnifiedPrivatePtr priv = domain->conn->privateData; + xenUnifiedPrivatePtr priv = conn->privateData; struct sexpr *root; const char *ret = NULL; char *schedulertype = NULL; @@ -2986,7 +2987,7 @@ xenDaemonGetSchedulerType(virDomainPtr domain, int *nparams) return NULL; }

- root = sexpr_get(domain->conn, "/xend/node/"); + root = sexpr_get(conn, "/xend/node/"); if (root == NULL) return NULL;

@@ -3037,11 +3038,12 @@ error: * Returns 0 or -1 in case of failure */ int -xenDaemonGetSchedulerParameters(virDomainPtr domain, +xenDaemonGetSchedulerParameters(virConnectPtr conn, + virDomainDefPtr def, virTypedParameterPtr params, int *nparams) { - xenUnifiedPrivatePtr priv = domain->conn->privateData; + xenUnifiedPrivatePtr priv = conn->privateData; struct sexpr *root; char *sched_type = NULL; int sched_nparam = 0; @@ -3055,12 +3057,12 @@ xenDaemonGetSchedulerParameters(virDomainPtr domain, }

/* look up the information by domain name */ - root = sexpr_get(domain->conn, "/xend/domain/%s?detail=1", domain->name); + root = sexpr_get(conn, "/xend/domain/%s?detail=1", def->name); if (root == NULL) return -1;

/* get the scheduler type */ - sched_type = xenDaemonGetSchedulerType(domain, &sched_nparam); + sched_type = xenDaemonGetSchedulerType(conn, &sched_nparam); if (sched_type == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to get a scheduler name")); @@ -3139,11 +3141,12 @@ error: * Returns 0 or -1 in case of failure */ int -xenDaemonSetSchedulerParameters(virDomainPtr domain, +xenDaemonSetSchedulerParameters(virConnectPtr conn, + virDomainDefPtr def, virTypedParameterPtr params, int nparams) { - xenUnifiedPrivatePtr priv = domain->conn->privateData; + xenUnifiedPrivatePtr priv = conn->privateData; struct sexpr *root; char *sched_type = NULL; int i; @@ -3158,12 +3161,12 @@ xenDaemonSetSchedulerParameters(virDomainPtr domain, }

/* look up the information by domain name */ - root = sexpr_get(domain->conn, "/xend/domain/%s?detail=1", domain->name); + root = sexpr_get(conn, "/xend/domain/%s?detail=1", def->name); if (root == NULL) return -1;

/* get the scheduler type */ - sched_type = xenDaemonGetSchedulerType(domain, &sched_nparam); + sched_type = xenDaemonGetSchedulerType(conn, &sched_nparam); if (sched_type == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to get a scheduler name")); @@ -3217,7 +3220,7 @@ xenDaemonSetSchedulerParameters(virDomainPtr domain, snprintf(buf_cap, sizeof(buf_cap), "%s", cap); }

- ret = xend_op(domain->conn, domain->name, "op", + ret = xend_op(conn, def->name, "op", "domain_sched_credit_set", "weight", buf_weight, "cap", buf_cap, NULL); break; diff --git a/src/xen/xend_internal.h b/src/xen/xend_internal.h index 3a7c0ac..cef7da4 100644 --- a/src/xen/xend_internal.h +++ b/src/xen/xend_internal.h @@ -189,11 +189,14 @@ int xenDaemonDomainMigratePerform (virConnectPtr conn,

int xenDaemonDomainBlockPeek (virDomainPtr domain, const char *path, unsigned long long offset, size_t size, void *buffer);

-char * xenDaemonGetSchedulerType(virDomainPtr domain, int *nparams); -int xenDaemonGetSchedulerParameters(virDomainPtr domain, +char * xenDaemonGetSchedulerType(virConnectPtr conn, + int *nparams); +int xenDaemonGetSchedulerParameters(virConnectPtr conn, + virDomainDefPtr def, virTypedParameterPtr params, int *nparams); -int xenDaemonSetSchedulerParameters(virDomainPtr domain, +int xenDaemonSetSchedulerParameters(virConnectPtr conn, + virDomainDefPtr def, virTypedParameterPtr params, int nparams);

Daniel P. Berrange wrote:

From: "Daniel P. Berrange" <berrange@redhat.com>

Introduce use of a virDomainDefPtr in the domain stats & peek 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@redhat.com> --- src/xen/block_stats.c | 6 +++--- src/xen/block_stats.h | 2 +- src/xen/xen_driver.c | 37 +++++++++++++++++++++++++++++++++---- src/xen/xen_hypervisor.c | 11 ++++++----- src/xen/xen_hypervisor.h | 9 +++++---- src/xen/xend_internal.c | 21 +++++++++++---------- src/xen/xend_internal.h | 7 ++++++- src/xen/xm_internal.c | 3 ++- src/xen/xm_internal.h | 7 ++++++- 9 files changed, 73 insertions(+), 30 deletions(-)

diff --git a/src/xen/block_stats.c b/src/xen/block_stats.c index ded8d7f..56a3901 100644 --- a/src/xen/block_stats.c +++ b/src/xen/block_stats.c @@ -359,16 +359,16 @@ xenLinuxDomainDeviceID(int domid, const char *path)

int xenLinuxDomainBlockStats(xenUnifiedPrivatePtr priv, - virDomainPtr dom, + virDomainDefPtr def, const char *path, struct _virDomainBlockStats *stats) { - int device = xenLinuxDomainDeviceID(dom->id, path); + int device = xenLinuxDomainDeviceID(def->id, path);

if (device < 0) return -1;

- return read_bd_stats(priv, device, dom->id, stats); + return read_bd_stats(priv, device, def->id, stats); }

#endif /* __linux__ */ diff --git a/src/xen/block_stats.h b/src/xen/block_stats.h index 0a3c40a..6633d97 100644 --- a/src/xen/block_stats.h +++ b/src/xen/block_stats.h @@ -28,7 +28,7 @@ # include "xen_driver.h"

extern int xenLinuxDomainBlockStats (xenUnifiedPrivatePtr priv, - virDomainPtr dom, const char *path, + virDomainDefPtr def, const char *path, struct _virDomainBlockStats *stats);

extern int xenLinuxDomainDeviceID(int domid, const char *dev); diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index 5ab1a52..7c00b70 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -1957,14 +1957,34 @@ static int xenUnifiedDomainBlockStats(virDomainPtr dom, const char *path, struct _virDomainBlockStats *stats) { - return xenHypervisorDomainBlockStats(dom, path, stats); + virDomainDefPtr def = NULL; + int ret = -1; + + if (!(def = xenGetDomainDefForDom(dom))) + goto cleanup; + + ret = xenHypervisorDomainBlockStats(dom->conn, def, path, stats); + +cleanup: + virDomainDefFree(def); + return ret; }

static int xenUnifiedDomainInterfaceStats(virDomainPtr dom, const char *path, struct _virDomainInterfaceStats *stats) { - return xenHypervisorDomainInterfaceStats(dom, path, stats); + virDomainDefPtr def = NULL; + int ret = -1; + + if (!(def = xenGetDomainDefForDom(dom))) + goto cleanup; + + ret = xenHypervisorDomainInterfaceStats(def, path, stats); + +cleanup: + virDomainDefFree(def); + return ret; }

static int @@ -1973,13 +1993,22 @@ xenUnifiedDomainBlockPeek(virDomainPtr dom, const char *path, void *buffer, unsigned int flags) { xenUnifiedPrivatePtr priv = dom->conn->privateData; + virDomainDefPtr def = NULL; + int ret = -1;

virCheckFlags(0, -1);

+ if (!(def = xenGetDomainDefForDom(dom))) + goto cleanup; + if (dom->id < 0 && priv->xendConfigVersion < XEND_CONFIG_VERSION_3_0_4) - return xenXMDomainBlockPeek(dom, path, offset, size, buffer); + ret = xenXMDomainBlockPeek(dom->conn, def, path, offset, size, buffer); else - return xenDaemonDomainBlockPeek(dom, path, offset, size, buffer); + ret = xenDaemonDomainBlockPeek(dom->conn, def, path, offset, size, buffer); + +cleanup: + virDomainDefFree(def); + return ret; }

static int diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c index 2525566..612ac77 100644 --- a/src/xen/xen_hypervisor.c +++ b/src/xen/xen_hypervisor.c @@ -1368,17 +1368,18 @@ xenHypervisorSetSchedulerParameters(virConnectPtr conn,

int -xenHypervisorDomainBlockStats(virDomainPtr dom, +xenHypervisorDomainBlockStats(virConnectPtr conn, + virDomainDefPtr def, const char *path, struct _virDomainBlockStats *stats) { #ifdef __linux__ - xenUnifiedPrivatePtr priv = dom->conn->privateData; + xenUnifiedPrivatePtr priv = conn->privateData; int ret;

xenUnifiedLock(priv); /* Need to lock because it hits the xenstore handle :-( */ - ret = xenLinuxDomainBlockStats(priv, dom, path, stats); + ret = xenLinuxDomainBlockStats(priv, def, path, stats); xenUnifiedUnlock(priv); return ret; #else @@ -1396,7 +1397,7 @@ xenHypervisorDomainBlockStats(virDomainPtr dom, * virNetwork interface, as yet not decided. */ int -xenHypervisorDomainInterfaceStats(virDomainPtr dom, +xenHypervisorDomainInterfaceStats(virDomainDefPtr def, const char *path, struct _virDomainInterfaceStats *stats) { @@ -1411,7 +1412,7 @@ xenHypervisorDomainInterfaceStats(virDomainPtr dom, _("invalid path, should be vif<domid>.<n>.")); return -1; } - if (rqdomid != dom->id) { + if (rqdomid != def->id) { virReportError(VIR_ERR_INVALID_ARG, "%s", _("invalid path, vif<domid> should match this domain ID")); return -1; diff --git a/src/xen/xen_hypervisor.h b/src/xen/xen_hypervisor.h index 1e5bb67..6aeab79 100644 --- a/src/xen/xen_hypervisor.h +++ b/src/xen/xen_hypervisor.h @@ -122,13 +122,14 @@ int xenHypervisorSetSchedulerParameters(virConnectPtr conn, int nparams) ATTRIBUTE_NONNULL (1);

-int xenHypervisorDomainBlockStats (virDomainPtr domain, +int xenHypervisorDomainBlockStats (virConnectPtr conn, + virDomainDefPtr def, const char *path, struct _virDomainBlockStats *stats) ATTRIBUTE_NONNULL (1); -int xenHypervisorDomainInterfaceStats (virDomainPtr domain, - const char *path, - struct _virDomainInterfaceStats *stats) +int xenHypervisorDomainInterfaceStats (virDomainDefPtr def, + const char *path, + struct _virDomainInterfaceStats *stats) ATTRIBUTE_NONNULL (1);

int xenHypervisorNodeGetCellsFreeMemory(virConnectPtr conn, diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index 3bcd19b..273408d 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -3247,13 +3247,14 @@ error: * Returns 0 if successful, -1 if error */ int -xenDaemonDomainBlockPeek(virDomainPtr domain, +xenDaemonDomainBlockPeek(virConnectPtr conn, + virDomainDefPtr minidef,

Function comments need updated. ACK. Regards, Jim

const char *path, unsigned long long offset, size_t size, void *buffer) { - xenUnifiedPrivatePtr priv = domain->conn->privateData; + xenUnifiedPrivatePtr priv = conn->privateData; struct sexpr *root = NULL; int fd = -1, ret = -1; virDomainDefPtr def; @@ -3263,12 +3264,12 @@ xenDaemonDomainBlockPeek(virDomainPtr domain, const char *actual;

/* Security check: The path must correspond to a block device. */ - if (domain->id > 0) - root = sexpr_get(domain->conn, "/xend/domain/%d?detail=1", - domain->id); - else if (domain->id < 0) - root = sexpr_get(domain->conn, "/xend/domain/%s?detail=1", - domain->name); + if (minidef->id > 0) + root = sexpr_get(conn, "/xend/domain/%d?detail=1", + minidef->id); + else if (minidef->id < 0) + root = sexpr_get(conn, "/xend/domain/%s?detail=1", + minidef->name); else { /* This call always fails for dom0. */ virReportError(VIR_ERR_OPERATION_INVALID, @@ -3283,8 +3284,8 @@ xenDaemonDomainBlockPeek(virDomainPtr domain,

id = xenGetDomIdFromSxpr(root, priv->xendConfigVersion); xenUnifiedLock(priv); - tty = xenStoreDomainGetConsolePath(domain->conn, id); - vncport = xenStoreDomainGetVNCPort(domain->conn, id); + tty = xenStoreDomainGetConsolePath(conn, id); + vncport = xenStoreDomainGetVNCPort(conn, id); xenUnifiedUnlock(priv);

if (!(def = xenParseSxpr(root, priv->xendConfigVersion, NULL, tty, diff --git a/src/xen/xend_internal.h b/src/xen/xend_internal.h index cef7da4..aa05130 100644 --- a/src/xen/xend_internal.h +++ b/src/xen/xend_internal.h @@ -187,7 +187,12 @@ int xenDaemonDomainMigratePerform (virConnectPtr conn, 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); +int xenDaemonDomainBlockPeek(virConnectPtr conn, + virDomainDefPtr def, + const char *path, + unsigned long long offset, + size_t size, + void *buffer);

char * xenDaemonGetSchedulerType(virConnectPtr conn, int *nparams); diff --git a/src/xen/xm_internal.c b/src/xen/xm_internal.c index bc98cf1..740c4df 100644 --- a/src/xen/xm_internal.c +++ b/src/xen/xm_internal.c @@ -1404,7 +1404,8 @@ xenXMDomainDetachDeviceFlags(virConnectPtr conn, }

int -xenXMDomainBlockPeek(virDomainPtr dom ATTRIBUTE_UNUSED, +xenXMDomainBlockPeek(virConnectPtr conn ATTRIBUTE_UNUSED, + virDomainDefPtr def ATTRIBUTE_UNUSED, const char *path ATTRIBUTE_UNUSED, unsigned long long offset ATTRIBUTE_UNUSED, size_t size ATTRIBUTE_UNUSED, diff --git a/src/xen/xm_internal.h b/src/xen/xm_internal.h index 78cd15c..25b4fd5 100644 --- a/src/xen/xm_internal.h +++ b/src/xen/xm_internal.h @@ -80,7 +80,12 @@ int xenXMDomainCreate(virConnectPtr conn, 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); +int xenXMDomainBlockPeek(virConnectPtr conn, + virDomainDefPtr def, + const char *path, + unsigned long long offset, + size_t size, + void *buffer);

int xenXMDomainGetAutostart(virDomainDefPtr def, int *autostart);

Daniel P. Berrange wrote:

From: "Daniel P. Berrange" <berrange@redhat.com>

Introduce use of a virDomainDefPtr in the domain coredump 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@redhat.com> --- src/xen/xen_driver.c | 14 +++++++++++++- src/xen/xend_internal.c | 9 +++++---- src/xen/xend_internal.h | 4 +++- 3 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index 7c00b70..a6c87ce 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -1193,7 +1193,19 @@ xenUnifiedDomainRestore(virConnectPtr conn, const char *from) static int xenUnifiedDomainCoreDump(virDomainPtr dom, const char *to, unsigned int flags) { - return xenDaemonDomainCoreDump(dom, to, flags); + virDomainDefPtr def = NULL; + int ret = -1; + + virCheckFlags(0, -1); + + if (!(def = xenGetDomainDefForDom(dom))) + goto cleanup; + + ret = xenDaemonDomainCoreDump(dom->conn, def, to, flags); + +cleanup: + virDomainDefFree(def); + return ret; }

static int diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index 273408d..8dc1a2d 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -1447,19 +1447,20 @@ xenDaemonDomainSave(virConnectPtr conn, * Returns 0 in case of success, -1 in case of error. */ int -xenDaemonDomainCoreDump(virDomainPtr domain, +xenDaemonDomainCoreDump(virConnectPtr conn, + virDomainDefPtr def,

Comments need updated. ACK. Regards, Jim

const char *filename, unsigned int flags) { virCheckFlags(VIR_DUMP_LIVE | VIR_DUMP_CRASH, -1);

- 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, + return xend_op(conn, def->name, "op", "dump", "file", filename, "live", (flags & VIR_DUMP_LIVE ? "1" : "0"), "crash", (flags & VIR_DUMP_CRASH ? "1" : "0"), diff --git a/src/xen/xend_internal.h b/src/xen/xend_internal.h index aa05130..b2d4368 100644 --- a/src/xen/xend_internal.h +++ b/src/xen/xend_internal.h @@ -95,7 +95,9 @@ int xenDaemonDomainDestroy(virConnectPtr conn, virDomainDefPtr def); int xenDaemonDomainSave(virConnectPtr conn, virDomainDefPtr def, const char *filename); -int xenDaemonDomainCoreDump(virDomainPtr domain, const char *filename, +int xenDaemonDomainCoreDump(virConnectPtr conn, + virDomainDefPtr def, + const char *filename, unsigned int flags); int xenDaemonDomainRestore(virConnectPtr conn, const char *filename); int xenDaemonDomainSetMemory(virConnectPtr conn,