| Message ID | 1569831660-31998-1-git-send-email-damjan.skvarc@gmail.com |
|---|---|
| State | Accepted |
| Commit | 3e0205082b52c8d46a9d2c9095087f6460a6ff30 |
| Headers | show |
| Series | [ovs-dev,1/1] ovsdb-client: fix memory leak in do_needs_conversion() and do_convert() | expand |
On Mon, Sep 30, 2019 at 10:21:00AM +0200, Damijan Skvarc wrote: > Memory leak itself is not so important, however the problem is that > it is caused by forgetting to close rpc channel which might in > a long term lead to the leak of system resources. > > Memory leak is reported by Valgrin running test suite and is expressed as: Thanks for the fix. I don't understand why this is necessary. These functions are called from main(), which closes the rpc connection just afterward: command->handler(rpc, database, argc - optind, argv + optind); free(database); jsonrpc_close(rpc); Can you take a closer look?
Hi Ben and thanks for review. I double checked and I can confirm the patch is ok. Changes which are applied to rpc pointer within do_convert() and do_need_conversion() function are local to functions and not propagated to outside world. This is because rpc is passed to command handler functions as pointer and reference to the pointer. Therefore main() could not know rpc has been changed inside command handler functions. regards,Damijan. On Mon, Sep 30, 2019 at 10:47 PM Ben Pfaff <blp@ovn.org> wrote: > On Mon, Sep 30, 2019 at 10:21:00AM +0200, Damijan Skvarc wrote: > > Memory leak itself is not so important, however the problem is that > > it is caused by forgetting to close rpc channel which might in > > a long term lead to the leak of system resources. > > > > Memory leak is reported by Valgrin running test suite and is expressed > as: > > Thanks for the fix. > > I don't understand why this is necessary. These functions are called > from main(), which closes the rpc connection just afterward: > > command->handler(rpc, database, argc - optind, argv + optind); > > free(database); > jsonrpc_close(rpc); > > Can you take a closer look? >
You are right. Thank you. I applied this to master. On Tue, Oct 01, 2019 at 10:23:45AM +0200, Damijan Skvarc wrote: > Hi Ben and thanks for review. > > I double checked and I can confirm the patch is ok. > Changes which are applied to rpc pointer within do_convert() and > do_need_conversion() function are local to > functions and not propagated to outside world. This is because rpc is > passed to command handler functions as > pointer and reference to the pointer. Therefore main() could not know rpc > has been changed inside command > handler functions. > > regards,Damijan. > > > On Mon, Sep 30, 2019 at 10:47 PM Ben Pfaff <blp@ovn.org> wrote: > > > On Mon, Sep 30, 2019 at 10:21:00AM +0200, Damijan Skvarc wrote: > > > Memory leak itself is not so important, however the problem is that > > > it is caused by forgetting to close rpc channel which might in > > > a long term lead to the leak of system resources. > > > > > > Memory leak is reported by Valgrin running test suite and is expressed > > as: > > > > Thanks for the fix. > > > > I don't understand why this is necessary. These functions are called > > from main(), which closes the rpc connection just afterward: > > > > command->handler(rpc, database, argc - optind, argv + optind); > > > > free(database); > > jsonrpc_close(rpc); > > > > Can you take a closer look? > >
diff --git a/ovsdb/ovsdb-client.c b/ovsdb/ovsdb-client.c index e3ef372..e78e9d9 100644 --- a/ovsdb/ovsdb-client.c +++ b/ovsdb/ovsdb-client.c @@ -1657,6 +1657,7 @@ do_convert(struct jsonrpc *rpc, const char *database_ OVS_UNUSED, check_txn(jsonrpc_transact_block(rpc, request, &reply), &reply); jsonrpc_msg_destroy(reply); ovsdb_schema_destroy(new_schema); + jsonrpc_close(rpc); } static void @@ -1677,6 +1678,7 @@ do_needs_conversion(struct jsonrpc *rpc, const char *database_ OVS_UNUSED, puts(ovsdb_schema_equal(schema1, schema2) ? "no" : "yes"); ovsdb_schema_destroy(schema1); ovsdb_schema_destroy(schema2); + jsonrpc_close(rpc); } struct dump_table_aux {
Memory leak itself is not so important, however the problem is that it is caused by forgetting to close rpc channel which might in a long term lead to the leak of system resources. Memory leak is reported by Valgrin running test suite and is expressed as: ==29472== 784 (600 direct, 184 indirect) bytes in 1 blocks are definitely lost in loss record 23 of 23 ==29472== at 0x4C2FB55: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==29472== by 0x449D32: xcalloc (util.c:121) ==29472== by 0x432147: jsonrpc_open (jsonrpc.c:87) ==29472== by 0x40ABBE: open_jsonrpc (ovsdb-client.c:528) ==29472== by 0x40ABBE: open_rpc (ovsdb-client.c:143) ==29472== by 0x40AE50: do_needs_conversion (ovsdb-client.c:1670) ==29472== by 0x405F76: main (ovsdb-client.c:282) ==29464== 784 (600 direct, 184 indirect) bytes in 1 blocks are definitely lost in loss record 23 of 23 ==29464== at 0x4C2FB55: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==29464== by 0x449D32: xcalloc (util.c:121) ==29464== by 0x432147: jsonrpc_open (jsonrpc.c:87) ==29464== by 0x40ABBE: open_jsonrpc (ovsdb-client.c:528) ==29464== by 0x40ABBE: open_rpc (ovsdb-client.c:143) ==29464== by 0x40AF5A: do_convert (ovsdb-client.c:1644) ==29464== by 0x405F76: main (ovsdb-client.c:282) Signed-off-by: Damijan Skvarc <damjan.skvarc@gmail.com> --- ovsdb/ovsdb-client.c | 2 ++ 1 file changed, 2 insertions(+)