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

[Matthew Daley <mattjd@xxxxxxxxx>]

On Wed, Sep 18, 2013 at 8:18 PM, Dario Faggioli
<dario.faggioli@xxxxxxxxxx> wrote:
> 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?

You can if you have access to the scan results. I've just been adding
them to the commit messages as that's what I've seen done in other
projects (and other people's Coverity-related Xen patches here). It
also helps in situations like these.

>
>> 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.

Right.

>
> 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?

Sure, or I can do it myself soon (ie. as a reply to this, or in the
next batch of patches). There's no rush (AFAIK ;) )

- Matthew

>
>>
>> @@ -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)
>

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