Re: [Xen-devel] [PATCH 13/28] libxl: only free cputopology if it was allocated in libxl_{cpu, node}map_to_{node, cpu}map

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On mer, 2013-09-18 at 15:37 +1200, Matthew Daley wrote:
> Coverity-ID: 1055294
> Coverity-ID: 1055295
>
Ok, forgive my coverity ignorance and sorry if I should know this
already, but I do I use these IDs to get some useful information?

> Signed-off-by: Matthew Daley <mattjd@xxxxxxxxx>
> ---
>  tools/libxl/libxl_utils.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
> index 4309e5e..8e567a8 100644
> --- a/tools/libxl/libxl_utils.c
> +++ b/tools/libxl/libxl_utils.c
> @@ -664,7 +664,8 @@ int libxl_nodemap_to_cpumap(libxl_ctx *ctx,
>              libxl_bitmap_set(cpumap, i);
>      }
>   out:
> -    libxl_cputopology_list_free(tinfo, nr_cpus);
> +    if (tinfo != NULL)
> +        libxl_cputopology_list_free(tinfo, nr_cpus);
>      return rc;
>  }
I was about to reply exactly as in the other e-mail (the reply to
12/28), but in this case I think there is some small rom for issues,
since nr_cpus is not initialized.

In fact, libxl_nodemap_to_cpumap() looks like this:

 648 it libxl_nodemap_to_cpumap(libxl_ctx *ctx,
 649                             const libxl_bitmap *nodemap,
 650                             libxl_bitmap *cpumap)
 651 {
 652     libxl_cputopology *tinfo = NULL;
 653     int nr_cpus, i, rc = 0;
 654 
 655     tinfo = libxl_get_cpu_topology(ctx, &nr_cpus);
 656     if (tinfo == NULL) {
 657         rc = ERROR_FAIL;
 658         goto out;
 659     }
 660 
 661     libxl_bitmap_set_none(cpumap);
 662     for (i = 0; i < nr_cpus; i++) {
 663         if (libxl_bitmap_test(nodemap, tinfo[i].node))
 664             libxl_bitmap_set(cpumap, i);
 665     }
 666  out:
 667     libxl_cputopology_list_free(tinfo, nr_cpus);
 668     return rc;
 669 }

Still, I think I'd like this fixed by doing something like this:

 653     int nr_cpus = 0, i, rc = 0;

rather than with what this patch does, for the same reasons already
explained in the other e-mails.

I'm up for sending such patch myself, but that will be in a while, as
right now I'm away from my workstation.

Is that fine with you?

>  
> @@ -685,7 +686,8 @@ int libxl_cpumap_to_nodemap(libxl_ctx *ctx,
>      libxl_for_each_set_bit(i, *cpumap)
>          libxl_bitmap_set(nodemap, tinfo[i].node);
>   out:
> -    libxl_cputopology_list_free(tinfo, nr_cpus);
> +    if (tinfo != NULL)
> +        libxl_cputopology_list_free(tinfo, nr_cpus);
>      return rc;
>  }
>  
And the same applies here, of course.

Is that fine with you guys?

Regards,
Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

Attachment: signature.asc
Description: This is a digitally signed message part

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