| Message ID | 1568236716-18105-5-git-send-email-pkusunyifeng@gmail.com |
|---|---|
| State | Accepted |
| Commit | 0070f7fed386d6902fdd08b0a5503656011a6d2e |
| Headers | show |
| Series | [ovs-dev,01/10] raft: Free leaked json data | expand |
One minor suggestion here:
Can you also handle freeing result:
diff --git a/ovsdb/trigger.c b/ovsdb/trigger.c
index 6f4ed96b0..0158957d6 100644
--- a/ovsdb/trigger.c
+++ b/ovsdb/trigger.c
@@ -214,6 +214,7 @@ ovsdb_trigger_try(struct ovsdb_trigger *t, long long
int now)
/* Unsatisfied "wait" condition. Take no action now,
retry
* later. */
}
+ json_destroy(result);
return false;
}
Else I can handle that in separate patch. Else, acked by for the series.
Acked-by: Aliasgar Ginwala <aginwala@ebay.com>
On Wed, Sep 11, 2019 at 2:19 PM Yifeng Sun <pkusunyifeng@gmail.com> wrote:
> Valgrind reported:
>
> 1925: schema conversion online - standalone
>
> ==10884== 689 (56 direct, 633 indirect) bytes in 1 blocks are definitely
> lost in loss record 384 of 420
> ==10884== at 0x4C2FB55: calloc (in
> /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==10884== by 0x44A592: xcalloc (util.c:121)
> ==10884== by 0x40E2EC: ovsdb_schema_create (ovsdb.c:41)
> ==10884== by 0x40E688: ovsdb_schema_from_json (ovsdb.c:217)
> ==10884== by 0x416C6F: ovsdb_trigger_try (trigger.c:246)
> ==10884== by 0x40D4DE: ovsdb_jsonrpc_trigger_create
> (jsonrpc-server.c:1119)
> ==10884== by 0x40D4DE: ovsdb_jsonrpc_session_got_request
> (jsonrpc-server.c:986)
> ==10884== by 0x40D4DE: ovsdb_jsonrpc_session_run (jsonrpc-server.c:556)
> ==10884== by 0x40D4DE: ovsdb_jsonrpc_session_run_all
> (jsonrpc-server.c:586)
> ==10884== by 0x40D4DE: ovsdb_jsonrpc_server_run (jsonrpc-server.c:401)
> ==10884== by 0x406A6E: main_loop (ovsdb-server.c:209)
> ==10884== by 0x406A6E: main (ovsdb-server.c:460)
>
> 'new_schema' should also be freed when there is no error.
> This patch fixes it.
>
> Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com>
> ---
> ovsdb/trigger.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ovsdb/trigger.c b/ovsdb/trigger.c
> index 6f4ed96b000b..7e62e90ae381 100644
> --- a/ovsdb/trigger.c
> +++ b/ovsdb/trigger.c
> @@ -254,8 +254,8 @@ ovsdb_trigger_try(struct ovsdb_trigger *t, long long
> int now)
> if (!error) {
> error = ovsdb_convert(t->db, new_schema, &newdb);
> }
> + ovsdb_schema_destroy(new_schema);
> if (error) {
> - ovsdb_schema_destroy(new_schema);
> trigger_convert_error(t, error);
> return false;
> }
> --
> 2.7.4
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Thanks Aginwala, Could you please double check if 'json_destroy(result)' is necessary here? If result != NULL, then it is passed in trigger_success(), which puts result in 't->reply', later, jsonrpc_msg_destroy() will free it. Thanks, Yifeng On Thu, Sep 12, 2019 at 2:00 PM aginwala <aginwala@asu.edu> wrote: > > One minor suggestion here: > Can you also handle freeing result: > diff --git a/ovsdb/trigger.c b/ovsdb/trigger.c > index 6f4ed96b0..0158957d6 100644 > --- a/ovsdb/trigger.c > +++ b/ovsdb/trigger.c > @@ -214,6 +214,7 @@ ovsdb_trigger_try(struct ovsdb_trigger *t, long long int now) > /* Unsatisfied "wait" condition. Take no action now, retry > * later. */ > } > + json_destroy(result); > return false; > } > > Else I can handle that in separate patch. Else, acked by for the series. > Acked-by: Aliasgar Ginwala <aginwala@ebay.com> > > On Wed, Sep 11, 2019 at 2:19 PM Yifeng Sun <pkusunyifeng@gmail.com> wrote: >> >> Valgrind reported: >> >> 1925: schema conversion online - standalone >> >> ==10884== 689 (56 direct, 633 indirect) bytes in 1 blocks are definitely lost in loss record 384 of 420 >> ==10884== at 0x4C2FB55: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) >> ==10884== by 0x44A592: xcalloc (util.c:121) >> ==10884== by 0x40E2EC: ovsdb_schema_create (ovsdb.c:41) >> ==10884== by 0x40E688: ovsdb_schema_from_json (ovsdb.c:217) >> ==10884== by 0x416C6F: ovsdb_trigger_try (trigger.c:246) >> ==10884== by 0x40D4DE: ovsdb_jsonrpc_trigger_create (jsonrpc-server.c:1119) >> ==10884== by 0x40D4DE: ovsdb_jsonrpc_session_got_request (jsonrpc-server.c:986) >> ==10884== by 0x40D4DE: ovsdb_jsonrpc_session_run (jsonrpc-server.c:556) >> ==10884== by 0x40D4DE: ovsdb_jsonrpc_session_run_all (jsonrpc-server.c:586) >> ==10884== by 0x40D4DE: ovsdb_jsonrpc_server_run (jsonrpc-server.c:401) >> ==10884== by 0x406A6E: main_loop (ovsdb-server.c:209) >> ==10884== by 0x406A6E: main (ovsdb-server.c:460) >> >> 'new_schema' should also be freed when there is no error. >> This patch fixes it. >> >> Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com> >> --- >> ovsdb/trigger.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/ovsdb/trigger.c b/ovsdb/trigger.c >> index 6f4ed96b000b..7e62e90ae381 100644 >> --- a/ovsdb/trigger.c >> +++ b/ovsdb/trigger.c >> @@ -254,8 +254,8 @@ ovsdb_trigger_try(struct ovsdb_trigger *t, long long int now) >> if (!error) { >> error = ovsdb_convert(t->db, new_schema, &newdb); >> } >> + ovsdb_schema_destroy(new_schema); >> if (error) { >> - ovsdb_schema_destroy(new_schema); >> trigger_convert_error(t, error); >> return false; >> } >> -- >> 2.7.4 >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Thanks Yifeng: Sure I revisited the trigger flow and even if its disconnected with trigger_success(), ovsdb_jsonrpc_trigger_complete will indeed take care of destroying t->reply via jsonrpc_msg_destroy() which I missed. So you are right, we don't need to json_destroy(result) explicitly here. I take it back! On Fri, Sep 13, 2019 at 9:46 AM Yifeng Sun <pkusunyifeng@gmail.com> wrote: > Thanks Aginwala, > > Could you please double check if 'json_destroy(result)' is necessary here? > If result != NULL, then it is passed in trigger_success(), which puts > result in 't->reply', > later, jsonrpc_msg_destroy() will free it. > > Thanks, > Yifeng > > On Thu, Sep 12, 2019 at 2:00 PM aginwala <aginwala@asu.edu> wrote: > > > > One minor suggestion here: > > Can you also handle freeing result: > > diff --git a/ovsdb/trigger.c b/ovsdb/trigger.c > > index 6f4ed96b0..0158957d6 100644 > > --- a/ovsdb/trigger.c > > +++ b/ovsdb/trigger.c > > @@ -214,6 +214,7 @@ ovsdb_trigger_try(struct ovsdb_trigger *t, long long > int now) > > /* Unsatisfied "wait" condition. Take no action > now, retry > > * later. */ > > } > > + json_destroy(result); > > return false; > > } > > > > Else I can handle that in separate patch. Else, acked by for the series. > > Acked-by: Aliasgar Ginwala <aginwala@ebay.com> > > > > On Wed, Sep 11, 2019 at 2:19 PM Yifeng Sun <pkusunyifeng@gmail.com> > wrote: > >> > >> Valgrind reported: > >> > >> 1925: schema conversion online - standalone > >> > >> ==10884== 689 (56 direct, 633 indirect) bytes in 1 blocks are > definitely lost in loss record 384 of 420 > >> ==10884== at 0x4C2FB55: calloc (in > /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) > >> ==10884== by 0x44A592: xcalloc (util.c:121) > >> ==10884== by 0x40E2EC: ovsdb_schema_create (ovsdb.c:41) > >> ==10884== by 0x40E688: ovsdb_schema_from_json (ovsdb.c:217) > >> ==10884== by 0x416C6F: ovsdb_trigger_try (trigger.c:246) > >> ==10884== by 0x40D4DE: ovsdb_jsonrpc_trigger_create > (jsonrpc-server.c:1119) > >> ==10884== by 0x40D4DE: ovsdb_jsonrpc_session_got_request > (jsonrpc-server.c:986) > >> ==10884== by 0x40D4DE: ovsdb_jsonrpc_session_run > (jsonrpc-server.c:556) > >> ==10884== by 0x40D4DE: ovsdb_jsonrpc_session_run_all > (jsonrpc-server.c:586) > >> ==10884== by 0x40D4DE: ovsdb_jsonrpc_server_run > (jsonrpc-server.c:401) > >> ==10884== by 0x406A6E: main_loop (ovsdb-server.c:209) > >> ==10884== by 0x406A6E: main (ovsdb-server.c:460) > >> > >> 'new_schema' should also be freed when there is no error. > >> This patch fixes it. > >> > >> Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com> > >> --- > >> ovsdb/trigger.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/ovsdb/trigger.c b/ovsdb/trigger.c > >> index 6f4ed96b000b..7e62e90ae381 100644 > >> --- a/ovsdb/trigger.c > >> +++ b/ovsdb/trigger.c > >> @@ -254,8 +254,8 @@ ovsdb_trigger_try(struct ovsdb_trigger *t, long > long int now) > >> if (!error) { > >> error = ovsdb_convert(t->db, new_schema, &newdb); > >> } > >> + ovsdb_schema_destroy(new_schema); > >> if (error) { > >> - ovsdb_schema_destroy(new_schema); > >> trigger_convert_error(t, error); > >> return false; > >> } > >> -- > >> 2.7.4 > >> > >> _______________________________________________ > >> dev mailing list > >> dev@openvswitch.org > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Wed, Sep 11, 2019 at 02:18:31PM -0700, Yifeng Sun wrote: > Valgrind reported: > > 1925: schema conversion online - standalone > > ==10884== 689 (56 direct, 633 indirect) bytes in 1 blocks are definitely lost in loss record 384 of 420 > ==10884== at 0x4C2FB55: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) > ==10884== by 0x44A592: xcalloc (util.c:121) > ==10884== by 0x40E2EC: ovsdb_schema_create (ovsdb.c:41) > ==10884== by 0x40E688: ovsdb_schema_from_json (ovsdb.c:217) > ==10884== by 0x416C6F: ovsdb_trigger_try (trigger.c:246) > ==10884== by 0x40D4DE: ovsdb_jsonrpc_trigger_create (jsonrpc-server.c:1119) > ==10884== by 0x40D4DE: ovsdb_jsonrpc_session_got_request (jsonrpc-server.c:986) > ==10884== by 0x40D4DE: ovsdb_jsonrpc_session_run (jsonrpc-server.c:556) > ==10884== by 0x40D4DE: ovsdb_jsonrpc_session_run_all (jsonrpc-server.c:586) > ==10884== by 0x40D4DE: ovsdb_jsonrpc_server_run (jsonrpc-server.c:401) > ==10884== by 0x406A6E: main_loop (ovsdb-server.c:209) > ==10884== by 0x406A6E: main (ovsdb-server.c:460) > > 'new_schema' should also be freed when there is no error. > This patch fixes it. > > Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com> LGTM Acked-by: William Tu <u9012063@gmail.com> > --- > ovsdb/trigger.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/ovsdb/trigger.c b/ovsdb/trigger.c > index 6f4ed96b000b..7e62e90ae381 100644 > --- a/ovsdb/trigger.c > +++ b/ovsdb/trigger.c > @@ -254,8 +254,8 @@ ovsdb_trigger_try(struct ovsdb_trigger *t, long long int now) > if (!error) { > error = ovsdb_convert(t->db, new_schema, &newdb); > } > + ovsdb_schema_destroy(new_schema); > if (error) { > - ovsdb_schema_destroy(new_schema); > trigger_convert_error(t, error); > return false; > } > -- > 2.7.4 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff --git a/ovsdb/trigger.c b/ovsdb/trigger.c index 6f4ed96b000b..7e62e90ae381 100644 --- a/ovsdb/trigger.c +++ b/ovsdb/trigger.c @@ -254,8 +254,8 @@ ovsdb_trigger_try(struct ovsdb_trigger *t, long long int now) if (!error) { error = ovsdb_convert(t->db, new_schema, &newdb); } + ovsdb_schema_destroy(new_schema); if (error) { - ovsdb_schema_destroy(new_schema); trigger_convert_error(t, error); return false; }
Valgrind reported: 1925: schema conversion online - standalone ==10884== 689 (56 direct, 633 indirect) bytes in 1 blocks are definitely lost in loss record 384 of 420 ==10884== at 0x4C2FB55: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==10884== by 0x44A592: xcalloc (util.c:121) ==10884== by 0x40E2EC: ovsdb_schema_create (ovsdb.c:41) ==10884== by 0x40E688: ovsdb_schema_from_json (ovsdb.c:217) ==10884== by 0x416C6F: ovsdb_trigger_try (trigger.c:246) ==10884== by 0x40D4DE: ovsdb_jsonrpc_trigger_create (jsonrpc-server.c:1119) ==10884== by 0x40D4DE: ovsdb_jsonrpc_session_got_request (jsonrpc-server.c:986) ==10884== by 0x40D4DE: ovsdb_jsonrpc_session_run (jsonrpc-server.c:556) ==10884== by 0x40D4DE: ovsdb_jsonrpc_session_run_all (jsonrpc-server.c:586) ==10884== by 0x40D4DE: ovsdb_jsonrpc_server_run (jsonrpc-server.c:401) ==10884== by 0x406A6E: main_loop (ovsdb-server.c:209) ==10884== by 0x406A6E: main (ovsdb-server.c:460) 'new_schema' should also be freed when there is no error. This patch fixes it. Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com> --- ovsdb/trigger.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)