[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 04/11] Convert Xen domain managed save driver methods to use virDomainDefPtr



Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange redhat com>
>
> 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 redhat com>
> ---
>  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);
>  
>   


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]