Re: [ovs-dev] [PATCH 1/9] ofproto/trace: Fix memory leak in oftrace_push_ct_state()

2017-11-01 Thread Yi-Hung Wei 
Thanks Greg and Ben for review. It looks like this patch is
independent to the reset of the series. I will pull it out and send
v2.

Thanks,

-Yi-Hung

On Tue, Oct 31, 2017 at 3:18 PM, Ben Pfaff  wrote:
> On Fri, Aug 25, 2017 at 03:51:11PM -0700, Yi-Hung Wei wrote:
>> Free the allocated memory in the pop function.
>>
>> Fixes: 0f2f05bbcf743 ("ofproto/trace: Add --ct-next option to ofproto/trace")
>> Signed-off-by: Yi-Hung Wei 
>> ---
>>  ofproto/ofproto-dpif-trace.c | 13 -
>>  1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c
>> index 38d11002f290..a45c9cfd9619 100644
>> --- a/ofproto/ofproto-dpif-trace.c
>> +++ b/ofproto/ofproto-dpif-trace.c
>> @@ -128,12 +128,14 @@ oftrace_push_ct_state(struct ovs_list *next_ct_states, 
>> uint32_t ct_state)
>>  ovs_list_push_back(next_ct_states, &next_ct_state->node);
>>  }
>>
>> -static uint32_t
>> -oftrace_pop_ct_state(struct ovs_list *next_ct_states)
>> +static void
>> +oftrace_pop_ct_state(struct ovs_list *next_ct_states, uint32_t *ct_state)
>>  {
>>  struct oftrace_next_ct_state *s;
>>  LIST_FOR_EACH_POP (s, node, next_ct_states) {
>> -return s->state;
>> +*ct_state = s->state;
>> +free(s);
>> +return;
>>  }
>>  OVS_NOT_REACHED();
>>  }
>
> Thanks for the fix!
>
> I don't understand why the function return type needs to change.  Can
> you change this to preserve the return type, while fixing the memory
> leak?
>
> Thanks,
>
> Ben.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/9] ofproto/trace: Fix memory leak in oftrace_push_ct_state()

2017-10-31 Thread Ben Pfaff 
On Fri, Aug 25, 2017 at 03:51:11PM -0700, Yi-Hung Wei wrote:
> Free the allocated memory in the pop function.
> 
> Fixes: 0f2f05bbcf743 ("ofproto/trace: Add --ct-next option to ofproto/trace")
> Signed-off-by: Yi-Hung Wei 
> ---
>  ofproto/ofproto-dpif-trace.c | 13 -
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c
> index 38d11002f290..a45c9cfd9619 100644
> --- a/ofproto/ofproto-dpif-trace.c
> +++ b/ofproto/ofproto-dpif-trace.c
> @@ -128,12 +128,14 @@ oftrace_push_ct_state(struct ovs_list *next_ct_states, 
> uint32_t ct_state)
>  ovs_list_push_back(next_ct_states, &next_ct_state->node);
>  }
>  
> -static uint32_t
> -oftrace_pop_ct_state(struct ovs_list *next_ct_states)
> +static void
> +oftrace_pop_ct_state(struct ovs_list *next_ct_states, uint32_t *ct_state)
>  {
>  struct oftrace_next_ct_state *s;
>  LIST_FOR_EACH_POP (s, node, next_ct_states) {
> -return s->state;
> +*ct_state = s->state;
> +free(s);
> +return;
>  }
>  OVS_NOT_REACHED();
>  }

Thanks for the fix!

I don't understand why the function return type needs to change.  Can
you change this to preserve the return type, while fixing the memory
leak?

Thanks,

Ben.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/9] ofproto/trace: Fix memory leak in oftrace_push_ct_state()

2017-08-31 Thread Greg Rose 

On 08/25/2017 03:51 PM, Yi-Hung Wei wrote:

Free the allocated memory in the pop function.

Fixes: 0f2f05bbcf743 ("ofproto/trace: Add --ct-next option to ofproto/trace")
Signed-off-by: Yi-Hung Wei 
---
  ofproto/ofproto-dpif-trace.c | 13 -
  1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c
index 38d11002f290..a45c9cfd9619 100644
--- a/ofproto/ofproto-dpif-trace.c
+++ b/ofproto/ofproto-dpif-trace.c
@@ -128,12 +128,14 @@ oftrace_push_ct_state(struct ovs_list *next_ct_states, 
uint32_t ct_state)
  ovs_list_push_back(next_ct_states, &next_ct_state->node);
  }

-static uint32_t
-oftrace_pop_ct_state(struct ovs_list *next_ct_states)
+static void
+oftrace_pop_ct_state(struct ovs_list *next_ct_states, uint32_t *ct_state)
  {
  struct oftrace_next_ct_state *s;
  LIST_FOR_EACH_POP (s, node, next_ct_states) {
-return s->state;
+*ct_state = s->state;
+free(s);
+return;
  }
  OVS_NOT_REACHED();
  }
@@ -404,7 +406,8 @@ static void
  free_ct_states(struct ovs_list *ct_states)
  {
  while (!ovs_list_is_empty(ct_states)) {
-oftrace_pop_ct_state(ct_states);
+uint32_t state;


Maybe a blank line here?


+oftrace_pop_ct_state(ct_states, &state);
  }
  }

@@ -646,7 +649,7 @@ ofproto_trace(struct ofproto_dpif *ofproto, const struct 
flow *flow,
  ds_put_cstr(output, " - resume conntrack with default "
  "ct_state=trk|new (use --ct-next to customize)");
  } else {
-ct_state = oftrace_pop_ct_state(next_ct_states);
+oftrace_pop_ct_state(next_ct_states, &ct_state);
  struct ds s = DS_EMPTY_INITIALIZER;
  format_flags(&s, ct_state_to_string, ct_state, '|');
  ds_put_format(output, " - resume conntrack with ct_state=%s",


Besides the minor nit above it LGTM.

Reviewed-by: Greg Rose 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH 1/9] ofproto/trace: Fix memory leak in oftrace_push_ct_state()

2017-08-25 Thread Yi-Hung Wei 
Free the allocated memory in the pop function.

Fixes: 0f2f05bbcf743 ("ofproto/trace: Add --ct-next option to ofproto/trace")
Signed-off-by: Yi-Hung Wei 
---
 ofproto/ofproto-dpif-trace.c | 13 -
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c
index 38d11002f290..a45c9cfd9619 100644
--- a/ofproto/ofproto-dpif-trace.c
+++ b/ofproto/ofproto-dpif-trace.c
@@ -128,12 +128,14 @@ oftrace_push_ct_state(struct ovs_list *next_ct_states, 
uint32_t ct_state)
 ovs_list_push_back(next_ct_states, &next_ct_state->node);
 }
 
-static uint32_t
-oftrace_pop_ct_state(struct ovs_list *next_ct_states)
+static void
+oftrace_pop_ct_state(struct ovs_list *next_ct_states, uint32_t *ct_state)
 {
 struct oftrace_next_ct_state *s;
 LIST_FOR_EACH_POP (s, node, next_ct_states) {
-return s->state;
+*ct_state = s->state;
+free(s);
+return;
 }
 OVS_NOT_REACHED();
 }
@@ -404,7 +406,8 @@ static void
 free_ct_states(struct ovs_list *ct_states)
 {
 while (!ovs_list_is_empty(ct_states)) {
-oftrace_pop_ct_state(ct_states);
+uint32_t state;
+oftrace_pop_ct_state(ct_states, &state);
 }
 }
 
@@ -646,7 +649,7 @@ ofproto_trace(struct ofproto_dpif *ofproto, const struct 
flow *flow,
 ds_put_cstr(output, " - resume conntrack with default "
 "ct_state=trk|new (use --ct-next to customize)");
 } else {
-ct_state = oftrace_pop_ct_state(next_ct_states);
+oftrace_pop_ct_state(next_ct_states, &ct_state);
 struct ds s = DS_EMPTY_INITIALIZER;
 format_flags(&s, ct_state_to_string, ct_state, '|');
 ds_put_format(output, " - resume conntrack with ct_state=%s",
-- 
2.7.4

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev