[libvirt] [PATCH v2 11/14] xen: Resolve resource leak with 'cpuset'

[Laine Stump]

On 01/10/2013 08:42 AM, John Ferlan wrote:
> Make cpuset local to the while loop and free it once done with it each
> time through the loop.  Add a sa_assert() to virBitmapParse() to keep Coverity
> from believing there could be a negative return and possible resource leak.
> ---
>  src/util/virbitmap.c    | 1 +
>  src/xen/xend_internal.c | 8 +++-----
>  2 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c
> index e032374..ca82d1b 100644
> --- a/src/util/virbitmap.c
> +++ b/src/util/virbitmap.c
> @@ -373,6 +373,7 @@ int virBitmapParse(const char *str,
>          }
>      }
>  
> +    sa_assert(ret >= 0);
>      return ret;
>  
>  parse_error:
> diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c
> index 84a25e8..445d336 100644
> --- a/src/xen/xend_internal.c
> +++ b/src/xen/xend_internal.c
> @@ -1113,7 +1113,6 @@ sexpr_to_xend_topology(const struct sexpr *root,
>  {
>      const char *nodeToCpu;
>      const char *cur;
> -    virBitmapPtr cpuset = NULL;
>      int *cpuNums = NULL;
>      int cell, cpu, nb_cpus;
>      int n = 0;
> @@ -1131,6 +1130,7 @@ sexpr_to_xend_topology(const struct sexpr *root,
>  
>      cur = nodeToCpu;
>      while (*cur != 0) {
> +        virBitmapPtr cpuset = NULL;
>          /*
>           * Find the next NUMA cell described in the xend output
>           */
> @@ -1163,28 +1163,26 @@ sexpr_to_xend_topology(const struct sexpr *root,
>              if (used)
>                  cpuNums[n++] = cpu;
>          }
> +        virBitmapFree(cpuset);
>  
>          if (virCapabilitiesAddHostNUMACell(caps,
>                                             cell,
>                                             nb_cpus,
>                                             cpuNums) < 0)
>              goto memory_error;
> +

Seeing this spurious addition of whitespace (which you may have added to
make it more obvious that it wasn't closing the immediately preceding
if() clause) made me notice that the preceding if() was unnecessarily
split onto multiple lines. Can either you, or whoever pushes this patch,
take this chance to combine the multiple lines of

   if (virCapabilitiesAddHostNUMACell(....) < 0)

onto a single line? (it easily fits in 80 columns).

>      }
>      VIR_FREE(cpuNums);
> -    virBitmapFree(cpuset);
>      return 0;
>  
>    parse_error:
>      virReportError(VIR_ERR_XEN_CALL, "%s", _("topology syntax error"));
>    error:
>      VIR_FREE(cpuNums);
> -    virBitmapFree(cpuset);
> -
>      return -1;
>  
>    memory_error:
>      VIR_FREE(cpuNums);
> -    virBitmapFree(cpuset);
>      virReportOOMError();
>      return -1;
>  }

ACK (with or without the above suggestion).