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

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



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


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