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@suse.com>
--- 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@lists.xen.org
http://lists.xen.org/xen-devel
Keir Fraser
2012-Sep-03 07:36 UTC
Re: [PATCH] make domain_create() return a proper error code
On 03/09/2012 07:57, "Jan Beulich" <JBeulich@suse.com> 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@suse.com>Acked-by: Keir Fraser <keir@xen.org> 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@lists.xen.org > http://lists.xen.org/xen-devel
Ian Campbell
2012-Sep-03 08:47 UTC
Re: [PATCH] make domain_create() return a proper error code
On Mon, 2012-09-03 at 07:57 +0100, Jan Beulich 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@suse.com> > > --- 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) )You probably wanted to change this one too? I''m not sure the first message really buys much -- I''d be happy to nuke it too. Ian.
Jan Beulich
2012-Sep-03 08:58 UTC
Re: [PATCH] make domain_create() return a proper error code
>>> On 03.09.12 at 10:47, Ian Campbell <Ian.Campbell@citrix.com> wrote: > On Mon, 2012-09-03 at 07:57 +0100, Jan Beulich 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@suse.com> >> >> --- 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) ) > > You probably wanted to change this one too?Oh, indeed. I blindly implied the printk() there would be a panic() already.> I''m not sure the first message really buys much -- I''d be happy to nuke > it too.Yes, I would be in favor of that. Will you do the adjustment, or shall I submit an incremental patch? Jan
Ian Campbell
2012-Sep-03 09:00 UTC
Re: [PATCH] make domain_create() return a proper error code
> > @@ -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) ) > > You probably wanted to change this one too? > > I''m not sure the first message really buys much -- I''d be happy to nuke > it too.8<------------------------ # HG changeset patch # User Ian Campbell <ian.campbell@citrix.com> # Date 1346662775 -3600 # Node ID c4e822e1b491bb7efa962b38fff6f007f01596b5 # Parent 241186e96a1ece42ad3bd14901b62d872f4abd9e arm: correctly check for error on dom0 allocation Drop the redundant printk Signed-off-by: Ian Campbell <ian.campbell@citrix.com> diff -r 241186e96a1e -r c4e822e1b491 xen/arch/arm/setup.c --- a/xen/arch/arm/setup.c Mon Sep 03 09:57:33 2012 +0100 +++ b/xen/arch/arm/setup.c Mon Sep 03 09:59:35 2012 +0100 @@ -238,9 +238,7 @@ void __init start_xen(unsigned long boot /* Create initial domain 0. */ dom0 = domain_create(0, 0, 0); - if ( IS_ERR(dom0) ) - printk("domain_create failed\n"); - 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;
Ian Campbell
2012-Sep-03 09:06 UTC
Re: [PATCH] make domain_create() return a proper error code
On Mon, 2012-09-03 at 09:58 +0100, Jan Beulich wrote:> >>> On 03.09.12 at 10:47, Ian Campbell <Ian.Campbell@citrix.com> wrote: > > On Mon, 2012-09-03 at 07:57 +0100, Jan Beulich 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@suse.com> > >> > >> --- 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) ) > > > > You probably wanted to change this one too? > > Oh, indeed. I blindly implied the printk() there would be a > panic() already. > > > I''m not sure the first message really buys much -- I''d be happy to nuke > > it too. > > Yes, I would be in favor of that. Will you do the adjustment, > or shall I submit an incremental patch?I''ve posted a patch, I think it and your mail probably crossed in the air...
Jan Beulich
2012-Sep-03 09:07 UTC
Re: [PATCH] make domain_create() return a proper error code
>>> On 03.09.12 at 11:00, Ian Campbell <Ian.Campbell@citrix.com> wrote: >> > @@ -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) ) >> >> You probably wanted to change this one too? >> >> I''m not sure the first message really buys much -- I''d be happy to nuke >> it too. > > 8<------------------------ > > # HG changeset patch > # User Ian Campbell <ian.campbell@citrix.com> > # Date 1346662775 -3600 > # Node ID c4e822e1b491bb7efa962b38fff6f007f01596b5 > # Parent 241186e96a1ece42ad3bd14901b62d872f4abd9e > arm: correctly check for error on dom0 allocation > > Drop the redundant printk > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>Thanks and (if you care) ack. Jan> diff -r 241186e96a1e -r c4e822e1b491 xen/arch/arm/setup.c > --- a/xen/arch/arm/setup.c Mon Sep 03 09:57:33 2012 +0100 > +++ b/xen/arch/arm/setup.c Mon Sep 03 09:59:35 2012 +0100 > @@ -238,9 +238,7 @@ void __init start_xen(unsigned long boot > > /* Create initial domain 0. */ > dom0 = domain_create(0, 0, 0); > - if ( IS_ERR(dom0) ) > - printk("domain_create failed\n"); > - 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;
Ian Campbell
2012-Sep-03 10:23 UTC
Re: [PATCH] make domain_create() return a proper error code
On Mon, 2012-09-03 at 10:07 +0100, Jan Beulich wrote:> >>> On 03.09.12 at 11:00, Ian Campbell <Ian.Campbell@citrix.com> wrote: > >> > @@ -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) ) > >> > >> You probably wanted to change this one too? > >> > >> I''m not sure the first message really buys much -- I''d be happy to nuke > >> it too. > > > > 8<------------------------ > > > > # HG changeset patch > > # User Ian Campbell <ian.campbell@citrix.com> > > # Date 1346662775 -3600 > > # Node ID c4e822e1b491bb7efa962b38fff6f007f01596b5 > > # Parent 241186e96a1ece42ad3bd14901b62d872f4abd9e > > arm: correctly check for error on dom0 allocation > > > > Drop the redundant printk > > > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > > Thanks and (if you care) ack.Thanks, I think any ack is worthwhile. Applied.> > Jan > > > diff -r 241186e96a1e -r c4e822e1b491 xen/arch/arm/setup.c > > --- a/xen/arch/arm/setup.c Mon Sep 03 09:57:33 2012 +0100 > > +++ b/xen/arch/arm/setup.c Mon Sep 03 09:59:35 2012 +0100 > > @@ -238,9 +238,7 @@ void __init start_xen(unsigned long boot > > > > /* Create initial domain 0. */ > > dom0 = domain_create(0, 0, 0); > > - if ( IS_ERR(dom0) ) > > - printk("domain_create failed\n"); > > - 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; > > >