diff mbox

[ovs-dev,PATCHv2] ovn-nbctl: Fix memory leak reported by Valgrind.

Message ID 1463267322-48468-1-git-send-email-u9012063@gmail.com
State Changes Requested
Headers show

Commit Message

William Tu May 14, 2016, 11:08 p.m. UTC 
Definitely lost is reported by test 2026: ovn -- 3 HVs, 1 LS, 3 lports/HV.
  ds_put_char__ (dynamic-string.c:82)
  ds_put_char (dynamic-string.h:88)
  process_escape_args (process.c:103)
  main (ovn-nbctl.c:92)
Another leak shown at ovn-sbctl.c with similar pattern.

Signed-off-by: William Tu <u9012063@gmail.com>
---
 ovn/utilities/ovn-nbctl.c | 12 ++++++++----
 ovn/utilities/ovn-sbctl.c | 12 ++++++++----
 2 files changed, 16 insertions(+), 8 deletions(-)

Comments

Ben Pfaff May 15, 2016, 4:48 a.m. UTC | #1 
On Sat, May 14, 2016 at 04:08:42PM -0700, William Tu wrote:
> Definitely lost is reported by test 2026: ovn -- 3 HVs, 1 LS, 3 lports/HV.
>   ds_put_char__ (dynamic-string.c:82)
>   ds_put_char (dynamic-string.h:88)
>   process_escape_args (process.c:103)
>   main (ovn-nbctl.c:92)
> Another leak shown at ovn-sbctl.c with similar pattern.
> 
> Signed-off-by: William Tu <u9012063@gmail.com>

The return value convention here is odd.  It's returning a boolean, but
it calls it an int, and it uses 0 for success instead of 1.  Please use
a real bool, with true for success and false for failure.

Thanks,

Ben.
William Tu May 15, 2016, 3:53 p.m. UTC | #2 
yes, using bool is better. Resubmit patch here:
https://patchwork.ozlabs.org/patch/622407/

Thanks,
William

On Sat, May 14, 2016 at 9:48 PM, Ben Pfaff <blp@ovn.org> wrote:

> On Sat, May 14, 2016 at 04:08:42PM -0700, William Tu wrote:
> > Definitely lost is reported by test 2026: ovn -- 3 HVs, 1 LS, 3
> lports/HV.
> >   ds_put_char__ (dynamic-string.c:82)
> >   ds_put_char (dynamic-string.h:88)
> >   process_escape_args (process.c:103)
> >   main (ovn-nbctl.c:92)
> > Another leak shown at ovn-sbctl.c with similar pattern.
> >
> > Signed-off-by: William Tu <u9012063@gmail.com>
>
> The return value convention here is odd.  It's returning a boolean, but
> it calls it an int, and it uses 0 for success instead of 1.  Please use
> a real bool, with true for success and false for failure.
>
> Thanks,
>
> Ben.
>
diff mbox

Patch

diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
index 5bdf757..81864c9 100644
--- a/ovn/utilities/ovn-nbctl.c
+++ b/ovn/utilities/ovn-nbctl.c
@@ -67,7 +67,7 @@  static void parse_options(int argc, char *argv[], struct shash *local_options);
 static const char *nbctl_default_db(void);
 static void run_prerequisites(struct ctl_command[], size_t n_commands,
                               struct ovsdb_idl *);
-static void do_nbctl(const char *args, struct ctl_command *, size_t n,
+static int do_nbctl(const char *args, struct ctl_command *, size_t n,
                      struct ovsdb_idl *);
 
 int
@@ -125,7 +125,10 @@  main(int argc, char *argv[])
 
         if (seqno != ovsdb_idl_get_seqno(idl)) {
             seqno = ovsdb_idl_get_seqno(idl);
-            do_nbctl(args, commands, n_commands, idl);
+            if (do_nbctl(args, commands, n_commands, idl) == 0) {
+                free(args);
+                exit(EXIT_SUCCESS);
+            }
         }
 
         if (seqno == ovsdb_idl_get_seqno(idl)) {
@@ -1157,7 +1160,7 @@  run_prerequisites(struct ctl_command *commands, size_t n_commands,
     }
 }
 
-static void
+static int
 do_nbctl(const char *args, struct ctl_command *commands, size_t n_commands,
          struct ovsdb_idl *idl)
 {
@@ -1296,7 +1299,7 @@  do_nbctl(const char *args, struct ctl_command *commands, size_t n_commands,
     ovsdb_idl_txn_destroy(txn);
     ovsdb_idl_destroy(idl);
 
-    exit(EXIT_SUCCESS);
+    return 0;
 
 try_again:
     /* Our transaction needs to be rerun, or a prerequisite was not met.  Free
@@ -1313,6 +1316,7 @@  try_again:
         free(c->table);
     }
     free(error);
+    return 1;
 }
 
 /* Frees the current transaction and the underlying IDL and then calls
diff --git a/ovn/utilities/ovn-sbctl.c b/ovn/utilities/ovn-sbctl.c
index a888333..2a07ce5 100644
--- a/ovn/utilities/ovn-sbctl.c
+++ b/ovn/utilities/ovn-sbctl.c
@@ -81,7 +81,7 @@  static void parse_options(int argc, char *argv[], struct shash *local_options);
 static const char *sbctl_default_db(void);
 static void run_prerequisites(struct ctl_command[], size_t n_commands,
                               struct ovsdb_idl *);
-static void do_sbctl(const char *args, struct ctl_command *, size_t n,
+static int do_sbctl(const char *args, struct ctl_command *, size_t n,
                      struct ovsdb_idl *);
 
 int
@@ -138,7 +138,10 @@  main(int argc, char *argv[])
 
         if (seqno != ovsdb_idl_get_seqno(idl)) {
             seqno = ovsdb_idl_get_seqno(idl);
-            do_sbctl(args, commands, n_commands, idl);
+            if (do_sbctl(args, commands, n_commands, idl) == 0) {
+                free(args);
+                exit(EXIT_SUCCESS);
+            }
         }
 
         if (seqno == ovsdb_idl_get_seqno(idl)) {
@@ -835,7 +838,7 @@  run_prerequisites(struct ctl_command *commands, size_t n_commands,
     }
 }
 
-static void
+static int
 do_sbctl(const char *args, struct ctl_command *commands, size_t n_commands,
          struct ovsdb_idl *idl)
 {
@@ -974,7 +977,7 @@  do_sbctl(const char *args, struct ctl_command *commands, size_t n_commands,
     ovsdb_idl_txn_destroy(txn);
     ovsdb_idl_destroy(idl);
 
-    exit(EXIT_SUCCESS);
+    return 0;
 
 try_again:
     /* Our transaction needs to be rerun, or a prerequisite was not met.  Free
@@ -991,6 +994,7 @@  try_again:
         free(c->table);
     }
     free(error);
+    return 1;
 }
 
 /* Frees the current transaction and the underlying IDL and then calls