| Message ID | 1463327553-52385-1-git-send-email-u9012063@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
"dev" <dev-bounces@openvswitch.org> wrote on 05/15/2016 10:52:33 AM: > From: William Tu <u9012063@gmail.com> > To: dev@openvswitch.org > Date: 05/15/2016 10:53 AM > Subject: [ovs-dev] [PATCHv3] ovn-nbctl: Fix memory leak reported by Valgrind. > Sent by: "dev" <dev-bounces@openvswitch.org> > > 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(-) > > diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c > index 5bdf757..d267829 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 bool 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)) { > + 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 bool > 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 true; > > try_again: > /* Our transaction needs to be rerun, or a prerequisite was notmet. Free > @@ -1313,6 +1316,7 @@ try_again: > free(c->table); > } > free(error); > + return false; > } > > /* 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..c0ee518 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 bool 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)) { > + 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 bool > 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 true; > > try_again: > /* Our transaction needs to be rerun, or a prerequisite was notmet. Free > @@ -991,6 +994,7 @@ try_again: > free(c->table); > } > free(error); > + return false; > } > > /* Frees the current transaction and the underlying IDL and then calls > -- I've not tested this, but it looks much better... Acked-by: Ryan Moats <rmoats@us.ibm.com>
On Sun, May 15, 2016 at 08:52:33AM -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> Applied, thanks!
diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c index 5bdf757..d267829 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 bool 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)) { + 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 bool 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 true; 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 false; } /* 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..c0ee518 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 bool 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)) { + 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 bool 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 true; 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 false; } /* Frees the current transaction and the underlying IDL and then calls
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(-)