Re: [Xen-devel] [PATCH v4 02/15] libxl: sanitize error handling in libxl_get_max_{cpus, nodes}

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2013-11-22 at 19:56 +0100, Dario Faggioli wrote:
> @@ -4351,7 +4349,7 @@ libxl_cputopology *libxl_get_cpu_topology(libxl_ctx 
> *ctx, int *nb_cpu_out)
>      int max_cpus;
>  
>      max_cpus = libxl_get_max_cpus(ctx);
> -    if (max_cpus == 0)
> +    if (max_cpus <= 0)

Doesn't this slightly odd error condition (which includes 0 as an error)
actually stem from  xc_get_max_cpus's broken handling of the case where
xc_physinfo failed?

It'd be better IMHO to make sure that this call chain ultimately returns
either a negative error or a positive number of cpus and never 0. I'd be
inclined to do that in xc_get_max_cpus. I suppose then
xc_get_{cpu,node}map_size would need error handling too. Handling it in
libxl_get_max_cpus might be reasonable too. That function really ought
to be returning libxl error codes, not -1 and errno as it currently
does.

If xc_physinfo is failing it seems unlikely that anything subsequent to
that is going to make much useful progress.

> @@ -4538,10 +4536,8 @@ libxl_vcpuinfo *libxl_list_vcpu(libxl_ctx *ctx, 
> uint32_t domid,
>      }
>  
>      for (*nb_vcpu = 0; *nb_vcpu <= domaininfo.max_vcpu_id; ++*nb_vcpu, 
> ++ptr) {
> -        if (libxl_cpu_bitmap_alloc(ctx, &ptr->cpumap, 0)) {
> -            LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "allocating cpumap");
> +        if (libxl_cpu_bitmap_alloc(ctx, &ptr->cpumap, 0))
>              return NULL;

Isn't this leaking ret == ptr here and everywhere else this returns?

> -        }
>          if (xc_vcpu_getinfo(ctx->xch, domid, *nb_vcpu, &vcpuinfo) == -1) {
>              LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting vcpu info");
>              return NULL;
> @@ -5304,7 +5300,7 @@ int libxl_get_freecpus(libxl_ctx *ctx, libxl_bitmap 
> *cpumap)
>      int ncpus;
>  
>      ncpus = libxl_get_max_cpus(ctx);
> -    if (ncpus == 0)
> +    if (ncpus <= 0)
>          return ERROR_FAIL;

If libxl_get_max_cpus returned a proper rc you could propagate it.

>  
>      cpumap->map = xc_cpupool_freeinfo(ctx->xch);
> diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
> index 9f5f589..1815422 100644
> --- a/tools/libxl/libxl_utils.c
> +++ b/tools/libxl/libxl_utils.c
> @@ -651,6 +651,56 @@ char *libxl_bitmap_to_hex_string(libxl_ctx *ctx, const 
> libxl_bitmap *bitmap)
>      return q;
>  }
>  
> +int libxl_cpu_bitmap_alloc(libxl_ctx *ctx, libxl_bitmap *cpumap, int 
> max_cpus)

You seem to have combined code motion with actual changes here. Please
don't do that.

> +{
> +    GC_INIT(ctx);
> +    int rc = 0;
> +
> +    if (max_cpus < 0) {
> +        rc = ERROR_INVAL;

No need to log here?

> +        goto out;
> +    }
> +    if (max_cpus == 0)
> +        max_cpus = libxl_get_max_cpus(ctx);
> +    if (max_cpus <= 0) {
> +        LOG(ERROR, "failed to retrieve the maximum number of cpus");
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +    /* This can't fail: no need to check and log */
> +    libxl_bitmap_alloc(ctx, cpumap, max_cpus);
> +
> + out:
> +    GC_FREE;
> +    return rc;
> +}
> +
> +int libxl_node_bitmap_alloc(libxl_ctx *ctx, libxl_bitmap *nodemap,
> +                            int max_nodes)

This is identical to cpu_bitmap alloc other than the parameter name and
the log message, isn't it? A common internal function, perhaps taking a
const char *what would eliminate that.

> +{
> +    GC_INIT(ctx);
> +    int rc = 0;
> +
> +    if (max_nodes < 0) {
> +        rc = ERROR_INVAL;
> +        goto out;
> +    }
> +
> +    if (max_nodes == 0)
> +        max_nodes = libxl_get_max_nodes(ctx);
> +    if (max_nodes <= 0) {
> +        LOG(ERROR, "failed to retrieve the maximum number of nodes");
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +    /* This can't fail: no need to check and log */
> +    libxl_bitmap_alloc(ctx, nodemap, max_nodes);
> +
> + out:
> +    GC_FREE;
> +    return rc;
> +}
> +
>  int libxl_nodemap_to_cpumap(libxl_ctx *ctx,
>                              const libxl_bitmap *nodemap,
>                              libxl_bitmap *cpumap)
> @@ -719,12 +769,16 @@ int libxl_cpumap_to_nodemap(libxl_ctx *ctx,
>  
>  int libxl_get_max_cpus(libxl_ctx *ctx)
>  {
> -    return xc_get_max_cpus(ctx->xch);
> +    int max_cpus = xc_get_max_cpus(ctx->xch);
> +
> +    return max_cpus <= 0 ? ERROR_FAIL : max_cpus;

Aha, so there is indeed no need for all the <= changes in the callers.
Good!

Ian.



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