Re: [ovs-dev] [PATCH 05/10] trigger: Free leaked ovsdb_schema
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
LGTM
Acked-by: William Tu
> ---
> 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, );
> }
> +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
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 05/10] trigger: Free leaked ovsdb_schema
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 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 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
> >
> > On Wed, Sep 11, 2019 at 2:19 PM 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
> >> ---
> >> 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, );
> >> }
> >> +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
> >> d...@openvswitch.org
> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 05/10] trigger: Free leaked ovsdb_schema
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 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
>
> On Wed, Sep 11, 2019 at 2:19 PM 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
>> ---
>> 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, );
>> }
>> +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
>> d...@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 05/10] trigger: Free leaked ovsdb_schema
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
On Wed, Sep 11, 2019 at 2:19 PM 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
> ---
> 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, );
> }
> +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
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH 05/10] trigger: Free leaked ovsdb_schema
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
---
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, );
}
+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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
