Re: [Xen-devel] [PATCH 6/6] xen/domain: Use IS_ERR_OR_NULL() when checking the return value of domain_create()

[George Dunlap <dunlapg@xxxxxxxxx>]

On Wed, Feb 28, 2018 at 2:14 PM, Andrew Cooper
<andrew.cooper3@xxxxxxxxxx> wrote:
> This means that hitting the fail path with with err set to 0 isn't considered
> as success by the callers.  All current codepaths look fine.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> CC: Julien Grall <julien.grall@xxxxxxx>
> ---
>  xen/arch/arm/mm.c     | 6 +++---
>  xen/arch/arm/setup.c  | 2 +-
>  xen/arch/x86/mm.c     | 6 +++---
>  xen/arch/x86/setup.c  | 2 +-
>  xen/common/domctl.c   | 2 +-
>  xen/common/schedule.c | 2 +-
>  6 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 3c328e2..16a08cf 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -521,7 +521,7 @@ void __init arch_init_memory(void)
>       * their domain field set to dom_xen.
>       */
>      dom_xen = domain_create(DOMID_XEN, DOMCRF_dummy, 0, NULL);
> -    BUG_ON(IS_ERR(dom_xen));
> +    BUG_ON(IS_ERR_OR_NULL(dom_xen));
>
>      /*
>       * Initialise our DOMID_IO domain.
> @@ -529,14 +529,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, NULL);
> -    BUG_ON(IS_ERR(dom_io));
> +    BUG_ON(IS_ERR_OR_NULL(dom_io));
>
>      /*
>       * Initialise our COW domain.
>       * This domain owns sharable pages.
>       */
>      dom_cow = domain_create(DOMID_COW, DOMCRF_dummy, 0, NULL);
> -    BUG_ON(IS_ERR(dom_cow));
> +    BUG_ON(IS_ERR_OR_NULL(dom_cow));
>  }
>
>  static inline lpae_t pte_of_xenaddr(vaddr_t va)
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 032a6a8..6646074 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -857,7 +857,7 @@ void __init start_xen(unsigned long boot_phys_offset,
>      config.nr_spis = gic_number_lines() - 32;
>
>      dom0 = domain_create(0, 0, 0, &config);
> -    if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) )
> +    if ( IS_ERR_OR_NULL(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) )
>              panic("Error creating domain 0");
>
>      dom0->is_privileged = 1;
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index e1f089b..c44b781 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -272,7 +272,7 @@ void __init arch_init_memory(void)
>       * (but be [partly] controlled by Dom0 nevertheless).
>       */
>      dom_xen = domain_create(DOMID_XEN, DOMCRF_dummy, 0, NULL);
> -    BUG_ON(IS_ERR(dom_xen));
> +    BUG_ON(IS_ERR_OR_NULL(dom_xen));
>      INIT_LIST_HEAD(&dom_xen->arch.pdev_list);
>
>      /*
> @@ -281,14 +281,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, NULL);
> -    BUG_ON(IS_ERR(dom_io));
> +    BUG_ON(IS_ERR_OR_NULL(dom_io));
>
>      /*
>       * Initialise our COW domain.
>       * This domain owns sharable pages.
>       */
>      dom_cow = domain_create(DOMID_COW, DOMCRF_dummy, 0, NULL);
> -    BUG_ON(IS_ERR(dom_cow));
> +    BUG_ON(IS_ERR_OR_NULL(dom_cow));
>
>      /*
>       * First 1MB of RAM is historically marked as I/O.  If we booted PVH,
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index ac530ec..92428da 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1640,7 +1640,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>
>      /* Create initial domain 0. */
>      dom0 = domain_create(get_initial_domain_id(), domcr_flags, 0, &config);
> -    if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) )
> +    if ( IS_ERR_OR_NULL(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) )
>          panic("Error creating domain 0");
>
>      if ( !pv_shim )
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index 50f7422..18dcb2d 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -547,7 +547,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
> u_domctl)
>
>          d = domain_create(dom, domcr_flags, op->u.createdomain.ssidref,
>                            &op->u.createdomain.config);
> -        if ( IS_ERR(d) )
> +        if ( IS_ERR_OR_NULL(d) )
>          {
>              ret = PTR_ERR(d);
>              d = NULL;

This will fail to crash if domain_create() returns NULL, but it will
still return 0 from the hypercall (since AFAICT PTR_ERR(NULL) will be
just '0').  Is that what we want?

Given that anywhere else this returns NULL will cause a BUG() or a
panic, I almost feel like a BUG_ON(d == NULL) would be more
appropriate.

Everything else looks good to me.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel