[PATCH] tools/libxl: Correct use of phyinfo_{init, dispose}()

[PATCH] tools/libxl: Correct use of phyinfo_{init, dispose}()

[Andrew Cooper]

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>

For my cpuid work which I have just started, I added an array to physinfo.
Coverity then frowned at me when it spotted all the memory leaks.

Technically speaking, 4.6 isn't broken due to not having an allocation to free
in _dispose(), but this patch might still be worth taking in 4.6.
---
 tools/libxl/libxl.c       |   12 ++++++++++++
 tools/libxl/libxl_utils.c |    9 ++++-----
 tools/libxl/xl_cmdimpl.c  |    6 +++++-
 3 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 083f099..56cf6ed 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -4615,6 +4615,7 @@ static int libxl__fill_dom0_memory_info(libxl__gc *gc, uint32_t *target_memkb,
     libxl_ctx *ctx = libxl__gc_owner(gc);
 
     libxl_dominfo_init(&info);
+    libxl_physinfo_init(&physinfo);
 
 retry_transaction:
     t = xs_transaction_start(ctx->xsh);
@@ -4653,6 +4654,8 @@ static int libxl__fill_dom0_memory_info(libxl__gc *gc, uint32_t *target_memkb,
     if (rc < 0)
         goto out;
 
+    libxl_physinfo_dispose(&physinfo);
+    libxl_physinfo_init(&physinfo);
     rc = libxl_get_physinfo(ctx, &physinfo);
     if (rc < 0)
         goto out;
@@ -4679,6 +4682,7 @@ static int libxl__fill_dom0_memory_info(libxl__gc *gc, uint32_t *target_memkb,
     }
 
     libxl_dominfo_dispose(&info);
+    libxl_physinfo_dispose(&physinfo);
     return rc;
 }
 
@@ -4944,6 +4948,8 @@ int libxl_get_free_memory(libxl_ctx *ctx, uint32_t *memkb)
     libxl_physinfo info;
     GC_INIT(ctx);
 
+    libxl_physinfo_init(&info);
+
     rc = libxl_get_physinfo(ctx, &info);
     if (rc < 0)
         goto out;
@@ -4952,6 +4958,7 @@ int libxl_get_free_memory(libxl_ctx *ctx, uint32_t *memkb)
 
 out:
     GC_FREE;
+    libxl_physinfo_dispose(&info);
     return rc;
 }
 
@@ -4962,7 +4969,11 @@ int libxl_wait_for_free_memory(libxl_ctx *ctx, uint32_t domid, uint32_t
     libxl_physinfo info;
     GC_INIT(ctx);
 
+    libxl_physinfo_init(&info);
+
     while (wait_secs > 0) {
+        libxl_physinfo_dispose(&info);
+        libxl_physinfo_init(&info);
         rc = libxl_get_physinfo(ctx, &info);
         if (rc < 0)
             goto out;
@@ -4977,6 +4988,7 @@ int libxl_wait_for_free_memory(libxl_ctx *ctx, uint32_t domid, uint32_t
 
 out:
     GC_FREE;
+    libxl_physinfo_dispose(&info);
     return rc;
 }
 
diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
index bfc9699..13dc2de 100644
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -848,11 +848,10 @@ int libxl__count_physical_sockets(libxl__gc *gc, int *sockets)
     libxl_physinfo_init(&info);
 
     rc = libxl_get_physinfo(CTX, &info);
-    if (rc)
-        return rc;
-
-    *sockets = info.nr_cpus / info.threads_per_core
-                            / info.cores_per_socket;
+    if (!rc) {
+        *sockets = info.nr_cpus / info.threads_per_core
+                                / info.cores_per_socket;
+    }
 
     libxl_physinfo_dispose(&info);
     return 0;
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 499a05c..9b77b10 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -5492,9 +5492,11 @@ static void output_physinfo(void)
     libxl_bitmap cpumap;
     int n = 0;
 
+    libxl_physinfo_init(&info);
+
     if (libxl_get_physinfo(ctx, &info) != 0) {
         fprintf(stderr, "libxl_physinfo failed.\n");
-        return;
+        goto out;
     }
     printf("nr_cpus                : %d\n", info.nr_cpus);
     printf("max_cpu_id             : %d\n", info.max_cpu_id);
@@ -5527,6 +5529,8 @@ static void output_physinfo(void)
         printf("free_cpus              : %d\n", n);
         free(cpumap.map);
     }
+
+ out:
     libxl_physinfo_dispose(&info);
     return;
 }
-- 
1.7.10.4

Re: [PATCH] tools/libxl: Correct use of phyinfo_{init, dispose}()

[Wei Liu]

On Tue, Aug 04, 2015 at 07:58:38PM +0100, Andrew Cooper wrote:
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
[...]
> index bfc9699..13dc2de 100644
> --- a/tools/libxl/libxl_utils.c
> +++ b/tools/libxl/libxl_utils.c
> @@ -848,11 +848,10 @@ int libxl__count_physical_sockets(libxl__gc *gc, int *sockets)
>      libxl_physinfo_init(&info);
>  
>      rc = libxl_get_physinfo(CTX, &info);
> -    if (rc)
> -        return rc;
> -
> -    *sockets = info.nr_cpus / info.threads_per_core
> -                            / info.cores_per_socket;
> +    if (!rc) {
> +        *sockets = info.nr_cpus / info.threads_per_core
> +                                / info.cores_per_socket;
> +    }
>  
>      libxl_physinfo_dispose(&info);
>      return 0;

This should return rc.

Wei.

Re: [PATCH] tools/libxl: Correct use of phyinfo_{init, dispose}()

[Andrew Cooper]

On 04/08/2015 22:25, Wei Liu wrote:
> On Tue, Aug 04, 2015 at 07:58:38PM +0100, Andrew Cooper wrote:
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
> [...]
>> index bfc9699..13dc2de 100644
>> --- a/tools/libxl/libxl_utils.c
>> +++ b/tools/libxl/libxl_utils.c
>> @@ -848,11 +848,10 @@ int libxl__count_physical_sockets(libxl__gc *gc, int *sockets)
>>      libxl_physinfo_init(&info);
>>  
>>      rc = libxl_get_physinfo(CTX, &info);
>> -    if (rc)
>> -        return rc;
>> -
>> -    *sockets = info.nr_cpus / info.threads_per_core
>> -                            / info.cores_per_socket;
>> +    if (!rc) {
>> +        *sockets = info.nr_cpus / info.threads_per_core
>> +                                / info.cores_per_socket;
>> +    }
>>  
>>      libxl_physinfo_dispose(&info);
>>      return 0;
> This should return rc.

Coverity spotted that too.  It is fixed in v2 which I havn't posted yet,
mainly pending the maintainers decision as to whether this patch is
worth persuing for 4.6 or not.

~Andrew

Re: [PATCH] tools/libxl: Correct use of phyinfo_{init, dispose}()

[Ian Campbell]

On Tue, 2015-08-04 at 19:58 +0100, Andrew Cooper wrote:
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> 
> For my cpuid work which I have just started, I added an array to physinfo.
> Coverity then frowned at me when it spotted all the memory leaks.
> 
> Technically speaking, 4.6 isn't broken due to not having an allocation to free
> in _dispose(), but this patch might still be worth taking in 4.6.

We've tended to be rather lax about this for internal code when there is no
actual work in the init/dispose functions in the current code base, so
while this improvement is the sort of thing we should/would routinely
except during development window I personally don't think it is worth a
freeze exception.

@@ -4653,6 +4654,8 @@ static int libxl__fill_dom0_memory_info(libxl__gc
> *gc, uint32_t *target_memkb,
>      if (rc < 0)
>          goto out;
>  
> +    libxl_physinfo_dispose(&physinfo);
> +    libxl_physinfo_init(&physinfo);

This pattern is starting to crop up a lot in loops of this type. I wonder
if I should make the idl generate a libxl_FOO_reinit() which just ==
dispose+init?

Ian.

Re: [PATCH] tools/libxl: Correct use of phyinfo_{init, dispose}()

[Andrew Cooper]

On 05/08/15 09:40, Ian Campbell wrote:
> On Tue, 2015-08-04 at 19:58 +0100, Andrew Cooper wrote:
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Ian Campbell <Ian.Campbell@citrix.com>
>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>>
>> For my cpuid work which I have just started, I added an array to physinfo.
>> Coverity then frowned at me when it spotted all the memory leaks.
>>
>> Technically speaking, 4.6 isn't broken due to not having an allocation to free
>> in _dispose(), but this patch might still be worth taking in 4.6.
> We've tended to be rather lax about this for internal code when there is no
> actual work in the init/dispose functions in the current code base, so
> while this improvement is the sort of thing we should/would routinely
> except during development window I personally don't think it is worth a
> freeze exception.

Ok.

>
> @@ -4653,6 +4654,8 @@ static int libxl__fill_dom0_memory_info(libxl__gc
>> *gc, uint32_t *target_memkb,
>>      if (rc < 0)
>>          goto out;
>>  
>> +    libxl_physinfo_dispose(&physinfo);
>> +    libxl_physinfo_init(&physinfo);
> This pattern is starting to crop up a lot in loops of this type. I wonder
> if I should make the idl generate a libxl_FOO_reinit() which just ==
> dispose+init?

Probably a good idea.  I believe I introduced the first use of that
style, finding nothing better as an alternative.

~Andrew