[PATCH] xl: free default_{vifscript, bridge, gatewaydev} on exit

[PATCH] xl: free default_{vifscript, bridge, gatewaydev} on exit

[Ian Campbell]

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

Re: [PATCH] xl: free default_{vifscript, bridge, gatewaydev} on exit

[Ian Jackson]

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.

Re: [PATCH] xl: free default_{vifscript, bridge, gatewaydev} on exit

[Ian Campbell]

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.

Re: [PATCH] xl: free default_{vifscript, bridge, gatewaydev} on exit

[George Dunlap]

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>

Re: [PATCH] xl: free default_{vifscript, bridge, gatewaydev} on exit

[Ian Campbell]

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?