| Message ID | 1511180799-16653-2-git-send-email-pkusunyifeng@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,1/2] execution: Fix bug that leaks ovsdb_row | expand |
On Mon, Nov 20, 2017 at 04:26:39AM -0800, Yifeng Sun wrote: > When ofm is not referenced by xc_entry, we should release its > resources by calling ofproto_flow_mod_uninit because no one is > going to use it in this function. > > Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com> > --- > ofproto/ofproto-dpif-xlate.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index 468cd160c60e..fcced344ed8a 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -5123,6 +5123,8 @@ xlate_learn_action(struct xlate_ctx *ctx, const struct ofpact_learn *learn) > entry->learn.ofm = ofm; > entry->learn.limit = learn->limit; > ofm = NULL; > + } else { > + ofproto_flow_mod_uninit(ofm); > } > > if (OVS_UNLIKELY(ctx->xin->trace && !success)) { Thank you for finding this issue. The code in xlate_learn_action() is very hard to understand. (I'm sure that this is partly my fault.) There appear to be cases where 'error' is nonzero but 'success' is true. Does this combination make sense, that is, should error != 0 imply success == false? Thanks, Ben.
It could be 'error' is nonzero but 'success' is true when look into the function ofproto_flow_mod_learn(). The logic is too complex here, sorry I can't reason if this makes sense. Thanks, Yifeng On Wed, Nov 29, 2017 at 1:53 PM, Ben Pfaff <blp@ovn.org> wrote: > On Mon, Nov 20, 2017 at 04:26:39AM -0800, Yifeng Sun wrote: > > When ofm is not referenced by xc_entry, we should release its > > resources by calling ofproto_flow_mod_uninit because no one is > > going to use it in this function. > > > > Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com> > > --- > > ofproto/ofproto-dpif-xlate.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > > index 468cd160c60e..fcced344ed8a 100644 > > --- a/ofproto/ofproto-dpif-xlate.c > > +++ b/ofproto/ofproto-dpif-xlate.c > > @@ -5123,6 +5123,8 @@ xlate_learn_action(struct xlate_ctx *ctx, const > struct ofpact_learn *learn) > > entry->learn.ofm = ofm; > > entry->learn.limit = learn->limit; > > ofm = NULL; > > + } else { > > + ofproto_flow_mod_uninit(ofm); > > } > > > > if (OVS_UNLIKELY(ctx->xin->trace && !success)) { > > Thank you for finding this issue. > > The code in xlate_learn_action() is very hard to understand. (I'm sure > that this is partly my fault.) There appear to be cases where 'error' > is nonzero but 'success' is true. Does this combination make sense, > that is, should error != 0 imply success == false? > > Thanks, > > Ben. >
Well, I hoped that you understood this code better than me, but I understand why you do not. Thanks a lot for finding and fixing the leak. I applied this to master and branch-2.8. (If it does not cause problems for a few days, then I will backport it further.) On Wed, Nov 29, 2017 at 03:42:43PM -0800, Yifeng Sun wrote: > It could be 'error' is nonzero but 'success' is true when look into the > function ofproto_flow_mod_learn(). The logic is too complex here, > sorry I can't reason if this makes sense. > > Thanks, > Yifeng > > On Wed, Nov 29, 2017 at 1:53 PM, Ben Pfaff <blp@ovn.org> wrote: > > > On Mon, Nov 20, 2017 at 04:26:39AM -0800, Yifeng Sun wrote: > > > When ofm is not referenced by xc_entry, we should release its > > > resources by calling ofproto_flow_mod_uninit because no one is > > > going to use it in this function. > > > > > > Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com> > > > --- > > > ofproto/ofproto-dpif-xlate.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > > > index 468cd160c60e..fcced344ed8a 100644 > > > --- a/ofproto/ofproto-dpif-xlate.c > > > +++ b/ofproto/ofproto-dpif-xlate.c > > > @@ -5123,6 +5123,8 @@ xlate_learn_action(struct xlate_ctx *ctx, const > > struct ofpact_learn *learn) > > > entry->learn.ofm = ofm; > > > entry->learn.limit = learn->limit; > > > ofm = NULL; > > > + } else { > > > + ofproto_flow_mod_uninit(ofm); > > > } > > > > > > if (OVS_UNLIKELY(ctx->xin->trace && !success)) { > > > > Thank you for finding this issue. > > > > The code in xlate_learn_action() is very hard to understand. (I'm sure > > that this is partly my fault.) There appear to be cases where 'error' > > is nonzero but 'success' is true. Does this combination make sense, > > that is, should error != 0 imply success == false? > > > > Thanks, > > > > Ben. > >
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 468cd160c60e..fcced344ed8a 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -5123,6 +5123,8 @@ xlate_learn_action(struct xlate_ctx *ctx, const struct ofpact_learn *learn) entry->learn.ofm = ofm; entry->learn.limit = learn->limit; ofm = NULL; + } else { + ofproto_flow_mod_uninit(ofm); } if (OVS_UNLIKELY(ctx->xin->trace && !success)) {
When ofm is not referenced by xc_entry, we should release its resources by calling ofproto_flow_mod_uninit because no one is going to use it in this function. Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com> --- ofproto/ofproto-dpif-xlate.c | 2 ++ 1 file changed, 2 insertions(+)