Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
>
> Introduce use of a virDomainDefPtr in the domain save
> 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@xxxxxxxxxx>
> ---
> src/xen/xen_driver.c | 72 ++++++++++++++++++++++++++++++++++++-------------
> src/xen/xend_internal.c | 23 +++++++++-------
> src/xen/xend_internal.h | 7 +++--
> src/xen/xm_internal.c | 25 ++++++++---------
> src/xen/xm_internal.h | 3 ++-
> 5 files changed, 86 insertions(+), 44 deletions(-)
>
> diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c
> index 68a86b7..89b038c 100644
> --- a/src/xen/xen_driver.c
> +++ b/src/xen/xen_driver.c
> @@ -1038,14 +1038,25 @@ static int
> xenUnifiedDomainSaveFlags(virDomainPtr dom, const char *to, const char *dxml,
> unsigned int flags)
> {
> + int ret = -1;
> + virDomainDefPtr def;
> +
> virCheckFlags(0, -1);
> +
> if (dxml) {
> virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
> _("xml modification unsupported"));
> return -1;
> }
>
> - return xenDaemonDomainSave(dom, to);
> + if (!(def = xenGetDomainDefForDom(dom)))
> + goto cleanup;
> +
> + ret = xenDaemonDomainSave(dom->conn, def, to);
> +
> +cleanup:
> + virDomainDefFree(def);
> + return ret;
> }
>
> static int
> @@ -1055,11 +1066,12 @@ xenUnifiedDomainSave(virDomainPtr dom, const char *to)
> }
>
> static char *
> -xenUnifiedDomainManagedSavePath(xenUnifiedPrivatePtr priv, virDomainPtr dom)
> +xenUnifiedDomainManagedSavePath(xenUnifiedPrivatePtr priv,
> + virDomainDefPtr def)
>
This still fits on one line.
> {
> char *ret;
>
> - if (virAsprintf(&ret, "%s/%s.save", priv->saveDir, dom->name) < 0) {
> + if (virAsprintf(&ret, "%s/%s.save", priv->saveDir, def->name) < 0) {
> virReportOOMError();
> return NULL;
> }
> @@ -1072,19 +1084,23 @@ static int
> xenUnifiedDomainManagedSave(virDomainPtr dom, unsigned int flags)
> {
> xenUnifiedPrivatePtr priv = dom->conn->privateData;
> - char *name;
> + char *name = NULL;
> + virDomainDefPtr def = NULL;
> int ret = -1;
>
> virCheckFlags(0, -1);
>
> - name = xenUnifiedDomainManagedSavePath(priv, dom);
> - if (!name)
> + if (!(def = xenGetDomainDefForDom(dom)))
> + goto cleanup;
> +
> + if (!(name = xenUnifiedDomainManagedSavePath(priv, def)))
> goto cleanup;
>
> - ret = xenDaemonDomainSave(dom, name);
> + ret = xenDaemonDomainSave(dom->conn, def, name);
>
> cleanup:
> VIR_FREE(name);
> + virDomainDefFree(def);
> return ret;
> }
>
> @@ -1092,17 +1108,23 @@ static int
> xenUnifiedDomainHasManagedSaveImage(virDomainPtr dom, unsigned int flags)
> {
> xenUnifiedPrivatePtr priv = dom->conn->privateData;
> - char *name;
> + char *name = NULL;
> + virDomainDefPtr def = NULL;
> int ret = -1;
>
> virCheckFlags(0, -1);
>
> - name = xenUnifiedDomainManagedSavePath(priv, dom);
> - if (!name)
> - return ret;
> + if (!(def = xenGetDomainDefForDom(dom)))
> + goto cleanup;
> +
> + if (!(name = xenUnifiedDomainManagedSavePath(priv, def)))
> + goto cleanup;
>
> ret = virFileExists(name);
> +
> +cleanup:
> VIR_FREE(name);
> + virDomainDefFree(def);
> return ret;
> }
>
> @@ -1110,16 +1132,21 @@ static int
> xenUnifiedDomainManagedSaveRemove(virDomainPtr dom, unsigned int flags)
> {
> xenUnifiedPrivatePtr priv = dom->conn->privateData;
> - char *name;
> + char *name = NULL;
> + virDomainDefPtr def = NULL;
> int ret = -1;
>
> virCheckFlags(0, -1);
>
> - name = xenUnifiedDomainManagedSavePath(priv, dom);
> - if (!name)
> - return ret;
> + if (!(def = xenGetDomainDefForDom(dom)))
> + goto cleanup;
> +
> + if (!(name = xenUnifiedDomainManagedSavePath(priv, def)))
> + goto cleanup;
>
> ret = unlink(name);
> +
> +cleanup:
> VIR_FREE(name);
> return ret;
> }
> @@ -1496,12 +1523,15 @@ xenUnifiedDomainCreateWithFlags(virDomainPtr dom, unsigned int flags)
> {
> xenUnifiedPrivatePtr priv = dom->conn->privateData;
> int ret = -1;
> + virDomainDefPtr def = NULL;
> char *name = NULL;
>
> virCheckFlags(0, -1);
>
> - name = xenUnifiedDomainManagedSavePath(priv, dom);
> - if (!name)
> + if (!(def = xenGetDomainDefForDom(dom)))
> + goto cleanup;
> +
> + if (!(name = xenUnifiedDomainManagedSavePath(priv, def)))
> goto cleanup;
>
> if (virFileExists(name)) {
> @@ -1512,11 +1542,15 @@ xenUnifiedDomainCreateWithFlags(virDomainPtr dom, unsigned int flags)
> }
>
> if (priv->xendConfigVersion < XEND_CONFIG_VERSION_3_0_4)
> - ret = xenXMDomainCreate(dom);
> + ret = xenXMDomainCreate(dom->conn, def);
> else
> - ret = xenDaemonDomainCreate(dom);
> + ret = xenDaemonDomainCreate(dom->conn, def);
> +
> + if (ret >= 0)
> + dom->id = def->id;
>
> cleanup:
> + virDomainDefFree(def);
> VIR_FREE(name);
> return ret;
> }
> diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c
> index c10567c..9555b33 100644
> --- a/src/xen/xend_internal.c
> +++ b/src/xen/xend_internal.c
> @@ -1413,22 +1413,24 @@ xenDaemonDomainGetOSType(virConnectPtr conn,
> * Returns 0 in case of success, -1 (with errno) in case of error.
> */
> int
> -xenDaemonDomainSave(virDomainPtr domain, const char *filename)
> +xenDaemonDomainSave(virConnectPtr conn,
> + virDomainDefPtr def,
> + const char *filename)
> {
> - 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;
> }
>
> /* We can't save the state of Domain-0, that would mean stopping it too */
> - if (domain->id == 0) {
> + if (def->id == 0) {
> virReportError(VIR_ERR_INVALID_ARG, "%s",
> _("Cannot save host domain"));
> return -1;
> }
>
> - return xend_op(domain->conn, domain->name, "op", "save", "file", filename, NULL);
> + return xend_op(conn, def->name, "op", "save", "file", filename, NULL);
> }
>
> /**
> @@ -2862,17 +2864,18 @@ xenDaemonDomainDefineXML(virConnectPtr conn, const char *xmlDesc)
> return NULL;
> }
> int
> -xenDaemonDomainCreate(virDomainPtr domain)
> +xenDaemonDomainCreate(virConnectPtr conn,
> + virDomainDefPtr def)
>
Fits on one line.
> {
> int ret;
>
> - ret = xend_op(domain->conn, domain->name, "op", "start", NULL);
> + ret = xend_op(conn, def->name, "op", "start", NULL);
>
> if (ret == 0) {
> - int id = xenDaemonDomainLookupByName_ids(domain->conn, domain->name,
> - domain->uuid);
> + int id = xenDaemonDomainLookupByName_ids(conn, def->name,
> + def->uuid);
>
Fits on one line now.
> if (id > 0)
> - domain->id = id;
> + def->id = id;
> }
>
> return ret;
> diff --git a/src/xen/xend_internal.h b/src/xen/xend_internal.h
> index 87e0a0f..01d321f 100644
> --- a/src/xen/xend_internal.h
> +++ b/src/xen/xend_internal.h
> @@ -92,7 +92,9 @@ 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 xenDaemonDomainSave(virConnectPtr conn,
> + virDomainDefPtr def,
> + const char *filename);
> int xenDaemonDomainCoreDump(virDomainPtr domain, const char *filename,
> unsigned int flags);
> int xenDaemonDomainRestore(virConnectPtr conn, const char *filename);
> @@ -131,7 +133,8 @@ int xenDaemonDetachDeviceFlags(virDomainPtr domain,
> unsigned int flags);
>
> virDomainPtr xenDaemonDomainDefineXML(virConnectPtr xend, const char *sexpr);
> -int xenDaemonDomainCreate(virDomainPtr domain);
> +int xenDaemonDomainCreate(virConnectPtr conn,
> + virDomainDefPtr def);
>
Still fits on one line.
> int xenDaemonDomainUndefine(virDomainPtr domain);
>
> int xenDaemonDomainSetVcpus (virDomainPtr domain,
> diff --git a/src/xen/xm_internal.c b/src/xen/xm_internal.c
> index ce44e39..6c884d9 100644
> --- a/src/xen/xm_internal.c
> +++ b/src/xen/xm_internal.c
> @@ -893,48 +893,49 @@ cleanup:
> * Start a domain from an existing defined config file
> */
> int
> -xenXMDomainCreate(virDomainPtr domain)
> +xenXMDomainCreate(virConnectPtr conn,
> + virDomainDefPtr def)
>
And again.
> {
> char *sexpr;
> int ret = -1;
> - xenUnifiedPrivatePtr priv= domain->conn->privateData;
> + xenUnifiedPrivatePtr priv = conn->privateData;
> const char *filename;
> xenXMConfCachePtr entry = NULL;
>
> xenUnifiedLock(priv);
>
> - if (!(filename = virHashLookup(priv->nameConfigMap, domain->name)))
> + if (!(filename = virHashLookup(priv->nameConfigMap, def->name)))
> goto error;
>
> if (!(entry = virHashLookup(priv->configCache, filename)))
> goto error;
>
> - if (!(sexpr = xenFormatSxpr(domain->conn, entry->def, priv->xendConfigVersion)))
> + if (!(sexpr = xenFormatSxpr(conn, entry->def, priv->xendConfigVersion)))
> goto error;
>
> - ret = xenDaemonDomainCreateXML(domain->conn, sexpr);
> + ret = xenDaemonDomainCreateXML(conn, sexpr);
> VIR_FREE(sexpr);
> if (ret != 0)
> goto error;
>
> - if ((ret = xenDaemonDomainLookupByName_ids(domain->conn, domain->name,
> + if ((ret = xenDaemonDomainLookupByName_ids(conn, def->name,
> entry->def->uuid)) < 0)
> goto error;
> - domain->id = ret;
> + def->id = ret;
>
> - if (xend_wait_for_devices(domain->conn, domain->name) < 0)
> + if (xend_wait_for_devices(conn, def->name) < 0)
> goto error;
>
> - if (xenDaemonDomainResume(domain->conn, entry->def) < 0)
> + if (xenDaemonDomainResume(conn, entry->def) < 0)
> goto error;
>
> xenUnifiedUnlock(priv);
> return 0;
>
> error:
> - if (domain->id != -1 && entry) {
> - xenDaemonDomainDestroy(domain->conn, entry->def);
> - domain->id = -1;
> + if (def->id != -1 && entry) {
> + xenDaemonDomainDestroy(conn, entry->def);
> + def->id = -1;
> }
> xenUnifiedUnlock(priv);
> return -1;
> diff --git a/src/xen/xm_internal.h b/src/xen/xm_internal.h
> index 0ae32bc..0521625 100644
> --- a/src/xen/xm_internal.h
> +++ b/src/xen/xm_internal.h
> @@ -65,7 +65,8 @@ virDomainDefPtr xenXMDomainLookupByUUID(virConnectPtr conn, const unsigned char
> int xenXMListDefinedDomains(virConnectPtr conn, char ** const names, int maxnames);
> int xenXMNumOfDefinedDomains(virConnectPtr conn);
>
> -int xenXMDomainCreate(virDomainPtr domain);
> +int xenXMDomainCreate(virConnectPtr conn,
> + virDomainDefPtr def);
>
Fits on one line.
No issues found testing this patch. ACK.
Regards,
Jim
> virDomainPtr xenXMDomainDefineXML(virConnectPtr con, const char *xml);
> int xenXMDomainUndefine(virDomainPtr domain);
>
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list