Xen devel - Jun 2013 - [PATCH] xl: free default_{vifscript, bridge, gatewaydev} on exit

These leaks aren''t serious, since they are only in xl but this makes
"xl list"
clean according to valgrind, which is useful from the point of view of
eliminating false positives when looking at the state of libxl (where leaks
matter).

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: George.Dunlap@citrix.com
---

I''m inclined to suggest that these are 4.4 material.
---
 tools/libxl/xl.c |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/tools/libxl/xl.c b/tools/libxl/xl.c
index 3c141bf..0f3acb9 100644
--- a/tools/libxl/xl.c
+++ b/tools/libxl/xl.c
@@ -276,6 +276,21 @@ static void xl_ctx_free(void)
         free(lockfile);
         lockfile = NULL;
     }
+
+    if (default_vifscript) {
+        free(default_vifscript);
+        default_vifscript = NULL;
+    }
+
+    if (default_bridge) {
+        free(default_bridge);
+        default_bridge = NULL;
+    }
+
+    if (default_gatewaydev) {
+        free(default_gatewaydev);
+        default_gatewaydev = NULL;
+    }
 }
 
 int main(int argc, char **argv)
-- 
1.7.2.5

Ian Campbell writes ("[PATCH] xl: free
default_{vifscript,bridge,gatewaydev} on exit"):
> These leaks aren''t serious, since they are only in xl but this
makes "xl list"
> clean according to valgrind, which is useful from the point of view of
> eliminating false positives when looking at the state of libxl (where leaks
> matter).
...
> +    if (default_vifscript) {
> +        free(default_vifscript);
> +        default_vifscript = NULL;
These ifs are unnecessary.  free(NULL) is a no-op.

I would write:

+        free(default_vifscript);   default_vifscript = NULL;
+        free(default_bridge);      default_bridge = NULL;
+        free(default_gatewaydev);  default_gatewaydev = NULL;

which is nice and regular.

Ian.

On Wed, 2013-06-12 at 13:41 +0100, Ian Jackson wrote:
> Ian Campbell writes ("[PATCH] xl: free
default_{vifscript,bridge,gatewaydev} on exit"):
> > These leaks aren''t serious, since they are only in xl but
this makes "xl list"
> > clean according to valgrind, which is useful from the point of view of
> > eliminating false positives when looking at the state of libxl (where
leaks
> > matter).
> ...
> > +    if (default_vifscript) {
> > +        free(default_vifscript);
> > +        default_vifscript = NULL;
> 
> These ifs are unnecessary.  free(NULL) is a no-op.
> 
> I would write:
> 
> +        free(default_vifscript);   default_vifscript = NULL;
> +        free(default_bridge);      default_bridge = NULL;
> +        free(default_gatewaydev);  default_gatewaydev = NULL;
> 
> which is nice and regular.
So would I except this is the prevailing style in that function for some
reason (even though I have a feeling I wrote it).

Ian.

On 12/06/13 13:38, Ian Campbell wrote:
> These leaks aren''t serious, since they are only in xl but this
makes "xl list"
> clean according to valgrind, which is useful from the point of view of
> eliminating false positives when looking at the state of libxl (where leaks
> matter).
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: George.Dunlap@citrix.com
Risk of introducing bugs? Almost nil.
Makes xen 4.3 more awesome?  Yes, by allowing valgrind to run cleanly.

Re the release:

Acked-by: George Dunlap <george.dunlap@eu.citrix.com>

On Wed, 2013-06-12 at 14:19 +0100, George Dunlap wrote:
> On 12/06/13 13:38, Ian Campbell wrote:
> > These leaks aren''t serious, since they are only in xl but
this makes "xl list"
> > clean according to valgrind, which is useful from the point of view of
> > eliminating false positives when looking at the state of libxl (where
leaks
> > matter).
> >
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > Cc: George.Dunlap@citrix.com
> 
> Risk of introducing bugs? Almost nil.
> Makes xen 4.3 more awesome?  Yes, by allowing valgrind to run cleanly.
> 
> Re the release:
> 
> Acked-by: George Dunlap <george.dunlap@eu.citrix.com>
Uh, seems these have languished in my queue since then, since more than
two weeks have passed I don''t want to just carry that ack over. I think
it is now too late?