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

Re: [libvirt] [PATCH 05/11] Convert Xen domain start/migration APIs to use virDomainDefPtr



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?

> 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    | 127 ++++++++++++++++++++++++++++++++----------------
>  src/xen/xend_internal.c |  71 +++++++++------------------
>  src/xen/xend_internal.h |  22 ++++++---
>  src/xen/xm_internal.c   |  49 ++++++++-----------
>  src/xen/xm_internal.h   |   7 +--
>  5 files changed, 148 insertions(+), 128 deletions(-)
>
> diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c
> index 89b038c..8b7dec9 100644
> --- a/src/xen/xen_driver.c
> +++ b/src/xen/xen_driver.c
> @@ -1294,18 +1294,31 @@ static char *
>  xenUnifiedDomainGetXMLDesc(virDomainPtr dom, unsigned int flags)
>  {
>      xenUnifiedPrivatePtr priv = dom->conn->privateData;
> +    virDomainDefPtr minidef = NULL;
> +    virDomainDefPtr def = NULL;
> +    char *ret = NULL;
> +
> +    if (!(minidef = xenGetDomainDefForDom(dom)))
> +        goto cleanup;
>  
>      if (dom->id < 0 && priv->xendConfigVersion < XEND_CONFIG_VERSION_3_0_4) {
> -        return xenXMDomainGetXMLDesc(dom, flags);
> +        def = xenXMDomainGetXMLDesc(dom->conn, minidef);
>      } else {
> -        char *cpus, *res;
> +        char *cpus;
>          xenUnifiedLock(priv);
>          cpus = xenDomainUsedCpus(dom);
>          xenUnifiedUnlock(priv);
> -        res = xenDaemonDomainGetXMLDesc(dom, flags, cpus);
> +        def = xenDaemonDomainGetXMLDesc(dom->conn, minidef, cpus);
>          VIR_FREE(cpus);
> -        return res;
>      }
> +
> +    if (def)
> +        ret = virDomainDefFormat(def, flags);
> +
> +cleanup:
> +    virDomainDefFree(def);
> +    virDomainDefFree(minidef);
> +    return ret;
>  }
>  
>  
> @@ -1438,10 +1451,21 @@ xenUnifiedDomainMigratePerform(virDomainPtr dom,
>                                 const char *dname,
>                                 unsigned long resource)
>  {
> +    virDomainDefPtr def = NULL;
> +    int ret = -1;
> +
>      virCheckFlags(XEN_MIGRATION_FLAGS, -1);
>  
> -    return xenDaemonDomainMigratePerform(dom, cookie, cookielen, uri,
> -                                         flags, dname, resource);
> +    if (!(def = xenGetDomainDefForDom(dom)))
> +        goto cleanup;
> +
> +    ret = xenDaemonDomainMigratePerform(dom->conn, def,
> +                                        cookie, cookielen, uri,
> +                                        flags, dname, resource);
> +
> +cleanup:
> +    virDomainDefFree(def);
> +    return ret;
>  }
>  
>  static virDomainPtr
> @@ -1452,45 +1476,37 @@ xenUnifiedDomainMigrateFinish(virConnectPtr dconn,
>                                const char *uri ATTRIBUTE_UNUSED,
>                                unsigned long flags)
>  {
> -    virDomainPtr dom = NULL;
> -    char *domain_xml = NULL;
> -    virDomainPtr dom_new = NULL;
> +    xenUnifiedPrivatePtr priv = dconn->privateData;
> +    virDomainPtr ret = NULL;
> +    virDomainDefPtr minidef = NULL;
> +    virDomainDefPtr def = NULL;
>  
>      virCheckFlags(XEN_MIGRATION_FLAGS, NULL);
>  
> -    if (!(dom = xenUnifiedDomainLookupByName(dconn, dname)))
> -        return NULL;
> +    if (!(minidef = xenGetDomainDefForName(dconn, dname)))
> +        goto cleanup;
>  
>      if (flags & VIR_MIGRATE_PERSIST_DEST) {
> -        domain_xml = xenDaemonDomainGetXMLDesc(dom, 0, NULL);
> -        if (! domain_xml) {
> -            virReportError(VIR_ERR_MIGRATE_PERSIST_FAILED,
> -                           "%s", _("failed to get XML representation of migrated domain"));
> -            goto error;
> -        }
> +        if (!(def = xenDaemonDomainGetXMLDesc(dconn, minidef, NULL)))
> +            goto cleanup;
>  
> -        dom_new = xenDaemonDomainDefineXML(dconn, domain_xml);
> -        if (! dom_new) {
> -            virReportError(VIR_ERR_MIGRATE_PERSIST_FAILED,
> -                           "%s", _("failed to define domain on destination host"));
> -            goto error;
> +        if (priv->xendConfigVersion < XEND_CONFIG_VERSION_3_0_4) {
> +            if (xenXMDomainDefineXML(dconn, def) < 0)
> +                goto cleanup;
> +        } else {
> +            if (xenDaemonDomainDefineXML(dconn, def) < 0)
> +                goto cleanup;
>          }
> -
> -        /* Free additional reference added by Define */
> -        virDomainFree(dom_new);
>      }
>  
> -    VIR_FREE(domain_xml);
> -
> -    return dom;
> -
> +    ret = virGetDomain(dconn, minidef->name, minidef->uuid);
> +    if (ret)
> +        ret->id = minidef->id;
>  
> -error:
> -    virDomainFree(dom);
> -
> -    VIR_FREE(domain_xml);
> -
> -    return NULL;
> +cleanup:
> +    virDomainDefFree(def);
> +    virDomainDefFree(minidef);
> +    return ret;
>  }
>  
>  static int
> @@ -1565,23 +1581,52 @@ static virDomainPtr
>  xenUnifiedDomainDefineXML(virConnectPtr conn, const char *xml)
>  {
>      xenUnifiedPrivatePtr priv = conn->privateData;
> +    virDomainDefPtr def = NULL;
> +    virDomainPtr ret = NULL;
>  
> -    if (priv->xendConfigVersion < XEND_CONFIG_VERSION_3_0_4)
> -        return xenXMDomainDefineXML(conn, xml);
> -    else
> -        return xenDaemonDomainDefineXML(conn, xml);
> +    if (!(def = virDomainDefParseString(xml, priv->caps, priv->xmlopt,
> +                                        1 << VIR_DOMAIN_VIRT_XEN,
> +                                        VIR_DOMAIN_XML_INACTIVE)))
> +        goto cleanup;
> +
> +    if (priv->xendConfigVersion < XEND_CONFIG_VERSION_3_0_4) {
> +        if (xenXMDomainDefineXML(conn, def) < 0)
> +            goto cleanup;
> +        def = NULL; /* XM driver owns it now */
> +    } else {
> +        if (xenDaemonDomainDefineXML(conn, def) < 0)
> +            goto cleanup;
> +    }
> +
> +    ret = virGetDomain(conn, def->name, def->uuid);
> +    if (ret)
> +        ret->id = -1;
> +
> +cleanup:
> +    virDomainDefFree(def);
> +    return ret;
>  }
>  
>  static int
>  xenUnifiedDomainUndefineFlags(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 (priv->xendConfigVersion < XEND_CONFIG_VERSION_3_0_4)
> -        return xenXMDomainUndefine(dom);
> +        ret = xenXMDomainUndefine(dom->conn, def);
>      else
> -        return xenDaemonDomainUndefine(dom);
> +        ret = xenDaemonDomainUndefine(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 9555b33..77c4dec 100644
> --- a/src/xen/xend_internal.c
> +++ b/src/xen/xend_internal.c
> @@ -1604,7 +1604,6 @@ cleanup:
>  /**
>   * xenDaemonDomainGetXMLDesc:
>   * @domain: a domain object
> - * @flags: potential dump flags
>   * @cpus: list of cpu the domain is pinned to.
>   *
>   * Provide an XML description of the domain.
> @@ -1612,27 +1611,15 @@ cleanup:
>   * Returns a 0 terminated UTF-8 encoded XML instance, or NULL in case of error.
>   *         the caller must free() the returned value.
>   */
>   

Comments for this function need updated, e.g. the domain parameter no
longer exists and it now returns a virtDomainDefPtr.

> -char *
> -xenDaemonDomainGetXMLDesc(virDomainPtr domain,
> -                          unsigned int flags,
> +virDomainDefPtr
> +xenDaemonDomainGetXMLDesc(virConnectPtr conn,
> +                          virDomainDefPtr minidef,
>                            const char *cpus)
>  {
> -    virDomainDefPtr def;
> -    char *xml;
> -
> -    /* Flags checked by virDomainDefFormat */
> -
> -    if (!(def = xenDaemonDomainFetch(domain->conn,
> -                                     domain->id,
> -                                     domain->name,
> -                                     cpus)))
> -        return NULL;
> -
> -    xml = virDomainDefFormat(def, flags);
> -
> -    virDomainDefFree(def);
> -
> -    return xml;
> +    return xenDaemonDomainFetch(conn,
> +                                minidef->id,
> +                                minidef->name,
> +                                cpus);
>  }
>  
>  
> @@ -2657,7 +2644,8 @@ xenDaemonDomainMigratePrepare(virConnectPtr dconn ATTRIBUTE_UNUSED,
>  }
>  
>  int
> -xenDaemonDomainMigratePerform(virDomainPtr domain,
> +xenDaemonDomainMigratePerform(virConnectPtr conn,
> +                              virDomainDefPtr def,
>                                const char *cookie ATTRIBUTE_UNUSED,
>                                int cookielen ATTRIBUTE_UNUSED,
>                                const char *uri,
> @@ -2801,7 +2789,7 @@ xenDaemonDomainMigratePerform(virDomainPtr domain,
>       * to our advantage since all parameters supported and required
>       * by current xend can be included without breaking older xend.
>       */
> -    ret = xend_op(domain->conn, domain->name,
> +    ret = xend_op(conn, def->name,
>                    "op", "migrate",
>                    "destination", hostname,
>                    "live", live,
> @@ -2814,34 +2802,24 @@ xenDaemonDomainMigratePerform(virDomainPtr domain,
>      VIR_FREE(hostname);
>  
>      if (ret == 0 && undefined_source)
> -        xenDaemonDomainUndefine(domain);
> +        xenDaemonDomainUndefine(conn, def);
>  
>      VIR_DEBUG("migration done");
>  
>      return ret;
>  }
>  
> -virDomainPtr
> -xenDaemonDomainDefineXML(virConnectPtr conn, const char *xmlDesc)
> +int
> +xenDaemonDomainDefineXML(virConnectPtr conn, virDomainDefPtr def)
>   

xenDaemonDomainGetXMLDesc and xenDaemonDomainDefineXML are poorly named
now that they don't return or accept XML, but that's a super nit :).

>  {
> -    int ret;
> +    int ret = -1;
>      char *sexpr;
> -    virDomainPtr dom;
>      xenUnifiedPrivatePtr priv = conn->privateData;
> -    virDomainDefPtr def;
> -
> -    if (!(def = virDomainDefParseString(xmlDesc, priv->caps, priv->xmlopt,
> -                                        1 << VIR_DOMAIN_VIRT_XEN,
> -                                        VIR_DOMAIN_XML_INACTIVE))) {
> -        virReportError(VIR_ERR_XML_ERROR,
> -                       "%s", _("failed to parse domain description"));
> -        return NULL;
> -    }
>  
>      if (!(sexpr = xenFormatSxpr(conn, def, priv->xendConfigVersion))) {
>          virReportError(VIR_ERR_XML_ERROR,
>                         "%s", _("failed to build sexpr"));
> -        goto error;
> +        goto cleanup;
>      }
>  
>      ret = xend_op(conn, "", "op", "new", "config", sexpr, NULL);
> @@ -2849,20 +2827,15 @@ xenDaemonDomainDefineXML(virConnectPtr conn, const char *xmlDesc)
>      if (ret != 0) {
>          virReportError(VIR_ERR_XEN_CALL,
>                         _("Failed to create inactive domain %s"), def->name);
> -        goto error;
> +        goto cleanup;
>      }
>  
> -    dom = virDomainLookupByName(conn, def->name);
> -    if (dom == NULL) {
> -        goto error;
> -    }
> -    virDomainDefFree(def);
> -    return dom;
> +    ret = 0;
>  
> -  error:
> -    virDomainDefFree(def);
> -    return NULL;
> +cleanup:
> +    return ret;
>  }
> +
>  int
>  xenDaemonDomainCreate(virConnectPtr conn,
>                        virDomainDefPtr def)
> @@ -2882,9 +2855,9 @@ xenDaemonDomainCreate(virConnectPtr conn,
>  }
>  
>  int
> -xenDaemonDomainUndefine(virDomainPtr domain)
> +xenDaemonDomainUndefine(virConnectPtr conn, virDomainDefPtr def)
>  {
> -    return xend_op(domain->conn, domain->name, "op", "delete", NULL);
> +    return xend_op(conn, def->name, "op", "delete", NULL);
>  }
>  
>  /**
> diff --git a/src/xen/xend_internal.h b/src/xen/xend_internal.h
> index 01d321f..1284db3 100644
> --- a/src/xen/xend_internal.h
> +++ b/src/xen/xend_internal.h
> @@ -111,8 +111,9 @@ int xenDaemonDomainGetState(virConnectPtr conn,
>                              virDomainDefPtr def,
>                              int *state,
>                              int *reason);
> -char *xenDaemonDomainGetXMLDesc(virDomainPtr domain, unsigned int flags,
> -                                const char *cpus);
> +virDomainDefPtr xenDaemonDomainGetXMLDesc(virConnectPtr conn,
> +                                          virDomainDefPtr def,
> +                                          const char *cpus);
>  unsigned long long xenDaemonDomainGetMaxMemory(virConnectPtr conn,
>                                                 virDomainDefPtr def);
>  char **xenDaemonListDomainsOld(virConnectPtr xend);
> @@ -132,10 +133,12 @@ int xenDaemonDetachDeviceFlags(virDomainPtr domain,
>                                 const char *xml,
>                                 unsigned int flags);
>  
> -virDomainPtr xenDaemonDomainDefineXML(virConnectPtr xend, const char *sexpr);
> +int xenDaemonDomainDefineXML(virConnectPtr conn,
> +                             virDomainDefPtr def);
>  int xenDaemonDomainCreate(virConnectPtr conn,
>                            virDomainDefPtr def);
> -int xenDaemonDomainUndefine(virDomainPtr domain);
> +int xenDaemonDomainUndefine(virConnectPtr conn,
> +                            virDomainDefPtr def);
>   

These still fit on one line, but this whole files has inconsistent use
of whitespace.

>  
>  int	xenDaemonDomainSetVcpus		(virDomainPtr domain,
>                                           unsigned int vcpus);
> @@ -163,8 +166,15 @@ int xenDaemonDomainSetAutostart          (virDomainPtr domain,
>  virDomainPtr xenDaemonCreateXML(virConnectPtr conn, const char *xmlDesc);
>  virDomainDefPtr xenDaemonLookupByUUID(virConnectPtr conn, const unsigned char *uuid);
>  virDomainDefPtr xenDaemonLookupByName(virConnectPtr conn, const char *domname);
> -int xenDaemonDomainMigratePrepare (virConnectPtr dconn, char **cookie, int *cookielen, const char *uri_in, char **uri_out, unsigned long flags, const char *dname, unsigned long resource);
> -int xenDaemonDomainMigratePerform (virDomainPtr domain, const char *cookie, int cookielen, const char *uri, unsigned long flags, const char *dname, unsigned long resource);
> +int xenDaemonDomainMigratePrepare (virConnectPtr dconn,
> +                                   char **cookie, int *cookielen,
> +                                   const char *uri_in, char **uri_out,
> +                                   unsigned long flags, const char *dname, unsigned long resource);
> +int xenDaemonDomainMigratePerform (virConnectPtr conn,
> +                                   virDomainDefPtr def,
> +                                   const char *cookie, int cookielen,
> +                                   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);
>  
> diff --git a/src/xen/xm_internal.c b/src/xen/xm_internal.c
> index 6c884d9..79f773d 100644
> --- a/src/xen/xm_internal.c
> +++ b/src/xen/xm_internal.c
> @@ -500,28 +500,29 @@ error:
>   * Turn a config record into a lump of XML describing the
>   * domain, suitable for later feeding for virDomainCreateXML
>   */
> -char *
> -xenXMDomainGetXMLDesc(virDomainPtr domain, unsigned int flags)
> +virDomainDefPtr
> +xenXMDomainGetXMLDesc(virConnectPtr conn,
> +                      virDomainDefPtr def)
>   

Still fits on one line.

>  {
> -    xenUnifiedPrivatePtr priv = domain->conn->privateData;
> +    xenUnifiedPrivatePtr priv = conn->privateData;
>      const char *filename;
>      xenXMConfCachePtr entry;
> -    char *ret = NULL;
> +    virDomainDefPtr ret = NULL;
>  
>      /* Flags checked by virDomainDefFormat */
>  
> -    if (domain->id != -1)
> -        return NULL;
> -
>      xenUnifiedLock(priv);
>  
> -    if (!(filename = virHashLookup(priv->nameConfigMap, domain->name)))
> +    if (!(filename = virHashLookup(priv->nameConfigMap, def->name)))
>          goto cleanup;
>  
>      if (!(entry = virHashLookup(priv->configCache, filename)))
>          goto cleanup;
>  
> -    ret = virDomainDefFormat(entry->def, flags);
> +    ret = virDomainDefCopy(entry->def,
> +                           priv->caps,
> +                           priv->xmlopt,
> +                           false);
>  
>  cleanup:
>      xenUnifiedUnlock(priv);
> @@ -945,13 +946,11 @@ xenXMDomainCreate(virConnectPtr conn,
>   * Create a config file for a domain, based on an XML
>   * document describing its config
>   */
> -virDomainPtr
> -xenXMDomainDefineXML(virConnectPtr conn, const char *xml)
> +int
> +xenXMDomainDefineXML(virConnectPtr conn, virDomainDefPtr def)
>  {
> -    virDomainPtr ret;
>      char *filename = NULL;
>      const char *oldfilename;
> -    virDomainDefPtr def = NULL;
>      virConfPtr conf = NULL;
>      xenXMConfCachePtr entry = NULL;
>      xenUnifiedPrivatePtr priv = conn->privateData;
> @@ -960,14 +959,7 @@ xenXMDomainDefineXML(virConnectPtr conn, const char *xml)
>  
>      if (!xenInotifyActive(conn) && xenXMConfigCacheRefresh(conn) < 0) {
>          xenUnifiedUnlock(priv);
> -        return NULL;
> -    }
> -
> -    if (!(def = virDomainDefParseString(xml, priv->caps, priv->xmlopt,
> -                                        1 << VIR_DOMAIN_VIRT_XEN,
> -                                        VIR_DOMAIN_XML_INACTIVE))) {
> -        xenUnifiedUnlock(priv);
> -        return NULL;
> +        return -1;
>      }
>  
>      if (!(conf = xenFormatXM(conn, def, priv->xendConfigVersion)))
> @@ -1061,10 +1053,9 @@ xenXMDomainDefineXML(virConnectPtr conn, const char *xml)
>          goto error;
>      }
>  
> -    ret = virGetDomain(conn, def->name, def->uuid);
>      xenUnifiedUnlock(priv);
>      VIR_FREE(filename);
> -    return ret;
> +    return 0;
>  
>   error:
>      VIR_FREE(filename);
> @@ -1072,25 +1063,25 @@ xenXMDomainDefineXML(virConnectPtr conn, const char *xml)
>          VIR_FREE(entry->filename);
>      VIR_FREE(entry);
>      virConfFree(conf);
> -    virDomainDefFree(def);
>      xenUnifiedUnlock(priv);
> -    return NULL;
> +    return -1;
>  }
>  
>  /*
>   * Delete a domain from disk
>   */
>  int
> -xenXMDomainUndefine(virDomainPtr domain)
> +xenXMDomainUndefine(virConnectPtr conn,
> +                    virDomainDefPtr def)
>   

Still fits on one line.

>  {
> -    xenUnifiedPrivatePtr priv = domain->conn->privateData;
> +    xenUnifiedPrivatePtr priv = conn->privateData;
>      const char *filename;
>      xenXMConfCachePtr entry;
>      int ret = -1;
>  
>      xenUnifiedLock(priv);
>  
> -    if (!(filename = virHashLookup(priv->nameConfigMap, domain->name)))
> +    if (!(filename = virHashLookup(priv->nameConfigMap, def->name)))
>          goto cleanup;
>  
>      if (!(entry = virHashLookup(priv->configCache, filename)))
> @@ -1100,7 +1091,7 @@ xenXMDomainUndefine(virDomainPtr domain)
>          goto cleanup;
>  
>      /* Remove the name -> filename mapping */
> -    if (virHashRemoveEntry(priv->nameConfigMap, domain->name) < 0)
> +    if (virHashRemoveEntry(priv->nameConfigMap, def->name) < 0)
>          goto cleanup;
>  
>      /* Remove the config record itself */
> diff --git a/src/xen/xm_internal.h b/src/xen/xm_internal.h
> index 0521625..5a434b9 100644
> --- a/src/xen/xm_internal.h
> +++ b/src/xen/xm_internal.h
> @@ -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.

Regards,
Jim

>  int xenXMDomainSetMemory(virConnectPtr conn,
>                           virDomainDefPtr def,
>                           unsigned long memory);
> @@ -67,8 +68,8 @@ int xenXMNumOfDefinedDomains(virConnectPtr conn);
>  
>  int xenXMDomainCreate(virConnectPtr conn,
>                        virDomainDefPtr def);
> -virDomainPtr xenXMDomainDefineXML(virConnectPtr con, const char *xml);
> -int xenXMDomainUndefine(virDomainPtr domain);
> +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);
>  
>   


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