Re: [Xen-devel] [PATCH] make domain_create() return a proper error code

[Keir Fraser <keir.xen@xxxxxxxxx>]

On 03/09/2012 07:57, "Jan Beulich" <JBeulich@xxxxxxxx> wrote:

> While triggered by the XSA-9 fix, this really is of more general use;
> that fix just pointed out very sharply that the current situation
> with all domain creation failures reported to user (tools) space as
> -ENOMEM is very unfortunate (actively misleading users _and_ support
> personnel).
> 
> Pull over the pointer <-> error code conversion infrastructure from
> Linux, and use it in domain_create() and all it callers.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Acked-by: Keir Fraser <keir@xxxxxxx>

This is straightforward really. Can go in for 4.2.

> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -26,6 +26,7 @@
>  #include <xen/serial.h>
>  #include <xen/sched.h>
>  #include <xen/console.h>
> +#include <xen/err.h>
>  #include <xen/init.h>
>  #include <xen/irq.h>
>  #include <xen/mm.h>
> @@ -237,7 +238,7 @@ void __init start_xen(unsigned long boot
>  
>      /* Create initial domain 0. */
>      dom0 = domain_create(0, 0, 0);
> -    if ( dom0 == NULL )
> +    if ( IS_ERR(dom0) )
>              printk("domain_create failed\n");
>      if ( (dom0 == NULL) || (alloc_dom0_vcpu0() == NULL) )
>              panic("Error creating domain 0\n");
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -91,7 +91,7 @@
>  #include <xen/mm.h>
>  #include <xen/domain.h>
>  #include <xen/sched.h>
> -#include <xen/errno.h>
> +#include <xen/err.h>
>  #include <xen/perfc.h>
>  #include <xen/irq.h>
>  #include <xen/softirq.h>
> @@ -309,7 +309,7 @@ void __init arch_init_memory(void)
>       * their domain field set to dom_xen.
>       */
>      dom_xen = domain_create(DOMID_XEN, DOMCRF_dummy, 0);
> -    BUG_ON(dom_xen == NULL);
> +    BUG_ON(IS_ERR(dom_xen));
>  
>      /*
>       * Initialise our DOMID_IO domain.
> @@ -317,14 +317,14 @@ void __init arch_init_memory(void)
>       * array. Mappings occur at the priv of the caller.
>       */
>      dom_io = domain_create(DOMID_IO, DOMCRF_dummy, 0);
> -    BUG_ON(dom_io == NULL);
> +    BUG_ON(IS_ERR(dom_io));
>      
>      /*
> -     * Initialise our DOMID_IO domain.
> +     * Initialise our COW domain.
>       * This domain owns sharable pages.
>       */
>      dom_cow = domain_create(DOMID_COW, DOMCRF_dummy, 0);
> -    BUG_ON(dom_cow == NULL);
> +    BUG_ON(IS_ERR(dom_cow));
>  
>      /* First 1MB of RAM is historically marked as I/O. */
>      for ( i = 0; i < 0x100; i++ )
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1,6 +1,7 @@
>  #include <xen/config.h>
>  #include <xen/init.h>
>  #include <xen/lib.h>
> +#include <xen/err.h>
>  #include <xen/sched.h>
>  #include <xen/sched-if.h>
>  #include <xen/domain.h>
> @@ -1319,7 +1320,7 @@ void __init __start_xen(unsigned long mb
>  
>      /* Create initial domain 0. */
>      dom0 = domain_create(0, DOMCRF_s3_integrity, 0);
> -    if ( (dom0 == NULL) || (alloc_dom0_vcpu0() == NULL) )
> +    if ( IS_ERR(dom0) || (alloc_dom0_vcpu0() == NULL) )
>          panic("Error creating domain 0\n");
>  
>      dom0->is_privileged = 1;
> --- a/xen/common/cpupool.c
> +++ b/xen/common/cpupool.c
> @@ -370,14 +370,18 @@ out:
>  int cpupool_add_domain(struct domain *d, int poolid)
>  {
>      struct cpupool *c;
> -    int rc = 1;
> +    int rc;
>      int n_dom = 0;
>  
>      if ( poolid == CPUPOOLID_NONE )
>          return 0;
>      spin_lock(&cpupool_lock);
>      c = cpupool_find_by_id(poolid);
> -    if ( (c != NULL) && cpumask_weight(c->cpu_valid) )
> +    if ( c == NULL )
> +        rc = -ESRCH;
> +    else if ( !cpumask_weight(c->cpu_valid) )
> +        rc = -ENODEV;
> +    else
>      {
>          c->n_dom++;
>          n_dom = c->n_dom;
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -9,7 +9,7 @@
>  #include <xen/init.h>
>  #include <xen/lib.h>
>  #include <xen/ctype.h>
> -#include <xen/errno.h>
> +#include <xen/err.h>
>  #include <xen/sched.h>
>  #include <xen/sched-if.h>
>  #include <xen/domain.h>
> @@ -196,17 +196,17 @@ struct domain *domain_create(
>      struct domain *d, **pd;
>      enum { INIT_xsm = 1u<<0, INIT_watchdog = 1u<<1, INIT_rangeset = 1u<<2,
>             INIT_evtchn = 1u<<3, INIT_gnttab = 1u<<4, INIT_arch = 1u<<5 };
> -    int init_status = 0;
> +    int err, init_status = 0;
>      int poolid = CPUPOOLID_NONE;
>  
>      if ( (d = alloc_domain_struct()) == NULL )
> -        return NULL;
> +        return ERR_PTR(-ENOMEM);
>  
>      d->domain_id = domid;
>  
>      lock_profile_register_struct(LOCKPROF_TYPE_PERDOM, d, domid, "Domain");
>  
> -    if ( xsm_alloc_security_domain(d) != 0 )
> +    if ( (err = xsm_alloc_security_domain(d)) != 0 )
>          goto fail;
>      init_status |= INIT_xsm;
>  
> @@ -226,6 +226,7 @@ struct domain *domain_create(
>      spin_lock_init(&d->shutdown_lock);
>      d->shutdown_code = -1;
>  
> +    err = -ENOMEM;
>      if ( !zalloc_cpumask_var(&d->domain_dirty_cpumask) )
>          goto fail;
>  
> @@ -251,7 +252,7 @@ struct domain *domain_create(
>  
>      if ( !is_idle_domain(d) )
>      {
> -        if ( xsm_domain_create(d, ssidref) != 0 )
> +        if ( (err = xsm_domain_create(d, ssidref)) != 0 )
>              goto fail;
>  
>          d->is_paused_by_controller = 1;
> @@ -266,29 +267,30 @@ struct domain *domain_create(
>  
>          radix_tree_init(&d->pirq_tree);
>  
> -        if ( evtchn_init(d) != 0 )
> +        if ( (err = evtchn_init(d)) != 0 )
>              goto fail;
>          init_status |= INIT_evtchn;
>  
> -        if ( grant_table_create(d) != 0 )
> +        if ( (err = grant_table_create(d)) != 0 )
>              goto fail;
>          init_status |= INIT_gnttab;
>  
>          poolid = 0;
>  
> +        err = -ENOMEM;
>          d->mem_event = xzalloc(struct mem_event_per_domain);
>          if ( !d->mem_event )
>              goto fail;
>      }
>  
> -    if ( arch_domain_create(d, domcr_flags) != 0 )
> +    if ( (err = arch_domain_create(d, domcr_flags)) != 0 )
>          goto fail;
>      init_status |= INIT_arch;
>  
> -    if ( cpupool_add_domain(d, poolid) != 0 )
> +    if ( (err = cpupool_add_domain(d, poolid)) != 0 )
>          goto fail;
>  
> -    if ( sched_init_domain(d) != 0 )
> +    if ( (err = sched_init_domain(d)) != 0 )
>          goto fail;
>  
>      if ( !is_idle_domain(d) )
> @@ -329,7 +331,7 @@ struct domain *domain_create(
>          xsm_free_security_domain(d);
>      free_cpumask_var(d->domain_dirty_cpumask);
>      free_domain_struct(d);
> -    return NULL;
> +    return ERR_PTR(err);
>  }
>  
>  
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -9,6 +9,7 @@
>  #include <xen/config.h>
>  #include <xen/types.h>
>  #include <xen/lib.h>
> +#include <xen/err.h>
>  #include <xen/mm.h>
>  #include <xen/sched.h>
>  #include <xen/sched-if.h>
> @@ -455,10 +456,12 @@ long do_domctl(XEN_GUEST_HANDLE(xen_domc
>          if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_oos_off )
>              domcr_flags |= DOMCRF_oos_off;
>  
> -        ret = -ENOMEM;
>          d = domain_create(dom, domcr_flags, op->u.createdomain.ssidref);
> -        if ( d == NULL )
> +        if ( IS_ERR(d) )
> +        {
> +            ret = PTR_ERR(d);
>              break;
> +        }
>  
>          ret = 0;
>  
> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -28,7 +28,7 @@
>  #include <xen/softirq.h>
>  #include <xen/trace.h>
>  #include <xen/mm.h>
> -#include <xen/errno.h>
> +#include <xen/err.h>
>  #include <xen/guest_access.h>
>  #include <xen/multicall.h>
>  #include <xen/cpu.h>
> @@ -1323,7 +1323,7 @@ void __init scheduler_init(void)
>          panic("scheduler returned error on init\n");
>  
>      idle_domain = domain_create(DOMID_IDLE, 0, 0);
> -    BUG_ON(idle_domain == NULL);
> +    BUG_ON(IS_ERR(idle_domain));
>      idle_domain->vcpu = idle_vcpu;
>      idle_domain->max_vcpus = nr_cpu_ids;
>      if ( alloc_vcpu(idle_domain, 0, 0) == NULL )
> --- /dev/null
> +++ b/xen/include/xen/err.h
> @@ -0,0 +1,57 @@
> +#if !defined(__XEN_ERR_H__) && !defined(__ASSEMBLY__)
> +#define __XEN_ERR_H__
> +
> +#include <xen/compiler.h>
> +#include <xen/errno.h>
> +
> +/*
> + * Kernel pointers have redundant information, so we can use a
> + * scheme where we can return either an error code or a dentry
> + * pointer with the same return value.
> + *
> + * This could be a per-architecture thing, to allow different
> + * error and pointer decisions.
> + */
> +#define MAX_ERRNO 4095
> +
> +#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
> +
> +static inline void *__must_check ERR_PTR(long error)
> +{
> + return (void *)error;
> +}
> +
> +static inline long __must_check PTR_ERR(const void *ptr)
> +{
> + return (long)ptr;
> +}
> +
> +static inline long __must_check IS_ERR(const void *ptr)
> +{
> + return IS_ERR_VALUE((unsigned long)ptr);
> +}
> +
> +static inline long __must_check IS_ERR_OR_NULL(const void *ptr)
> +{
> + return !ptr || IS_ERR_VALUE((unsigned long)ptr);
> +}
> +
> +/**
> + * ERR_CAST - Explicitly cast an error-valued pointer to another pointer type
> + * @ptr: The pointer to cast.
> + *
> + * Explicitly cast an error-valued pointer to another pointer type in such a
> + * way as to make it clear that's what's going on.
> + */
> +static inline void * __must_check ERR_CAST(const void *ptr)
> +{
> + /* cast away the const */
> + return (void *)ptr;
> +}
> +
> +static inline int __must_check PTR_RET(const void *ptr)
> +{
> + return IS_ERR(ptr) ? PTR_ERR(ptr) : 0;
> +}
> +
> +#endif /* __XEN_ERR_H__ */
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel