No worries. Let's get them in for the moment and fix them in incremental patches.
Thanks, Alin. > -----Original Message----- > From: Yin Lin [mailto:[email protected]] > Sent: Friday, June 9, 2017 6:29 AM > To: Alin Serdean <[email protected]>; > [email protected]; Sairam Venugopal <[email protected]> > Subject: RE: [ovs-dev] [PATCH v7 3/4] datapath-windows: NAT integration > with conntrack > > Hi Alin, > > Thanks for the feedback. They are very sharp catches! I have address all of > them except the last one. The last one is a memory leak that has been there > for a while and not touched by my patch. The fix is not easy either. Take a > look at the following code: > > OVS_CT_ENTRY * > OvsConntrackCreateIcmpEntry(UINT64 now) > { > struct conn_icmp *conn; > > conn = OvsAllocateMemoryWithTag(sizeof(struct conn_icmp), > OVS_CT_POOL_TAG); > if (!conn) { > return NULL; > } > > conn->state = ICMPS_FIRST; > > OvsConntrackUpdateExpiration(&conn->up, now, > icmp_timeouts[conn->state]); > > return &conn->up; > } > > We allocate a conn_icmp structure in the heap, but only returns a reference > to conn->up, so the caller cannot even release the memory. > > We'll have to create another patch to address this. Sai, can you take care of > the memory leak? > > Best regards, > Yin Lin > > -----Original Message----- > From: Alin Serdean [mailto:[email protected]] > Sent: Tuesday, May 23, 2017 7:18 AM > To: Yin Lin <[email protected]>; [email protected] > Subject: RE: [ovs-dev] [PATCH v7 3/4] datapath-windows: NAT integration > with conntrack > > Hi Yin, > > Sorry it took a while to review the patch. > > I just have a few inlined comments. I am stripping the code a bit to be more > readable. > > I will run some tests tonight over the current series to see that everything > is > ok from a functionality perspective. > > Thanks, > Alin. > > > > > This patch integrates NAT module with existing conntrack module. NAT > > action is now supported. > > > > Signed-off-by: Yin Lin <[email protected]> diff --git > > a/datapath-windows/automake.mk b/datapath- windows/automake.mk > index > > 7177630..f1632cc 100644 > > --- a/datapath-windows/automake.mk > > +++ b/datapath-windows/automake.mk > > @@ -16,9 +16,9 @@ EXTRA_DIST += \ > > datapath-windows/ovsext/Conntrack-icmp.c \ > > datapath-windows/ovsext/Conntrack-other.c \ > > datapath-windows/ovsext/Conntrack-related.c \ > > - datapath-windows/ovsext/Conntrack-nat.c \ > > + datapath-windows/ovsext/Conntrack-nat.c \ > > datapath-windows/ovsext/Conntrack-tcp.c \ > > - datapath-windows/ovsext/Conntrack-nat.h \ > > + datapath-windows/ovsext/Conntrack-nat.h \ > [Alin Serdean] Just use tab instead of 4 spaces in patch 2/4 > > datapath-windows/ovsext/Conntrack.c \ > > datapath-windows/ovsext/Conntrack.h \ > > datapath-windows/ovsext/Datapath.c \ > [Alin Serdean] <========================== cut ================> > > + > > + /* NatInfo is always initialized to be disabled, so that if NAT action > > + * fails, we will not end up deleting an non-existent NAT entry. > > + */ > > + if (natInfo != NULL && OvsIsForwardNat(natInfo->natAction)) { > > + entry->natInfo = *natInfo; > > + if (!OvsNatCtEntry(entry)) { > > + return FALSE; > > + } > > + ctx->hash = OvsHashCtKey(&entry->key); > > + } else { > > + entry->natInfo.natAction = natInfo->natAction; > [Alin Serdean] Shouldn't this be guarded for natInfo==NULL? > > + } > > + > [Alin Serdean] <========================== cut ================> > > - OvsCtUpdateFlowKey(key, state, ctx->key.zone, 0, NULL); > > - return entry; > > + break; > > } > > default: > > goto invalid; > > } > > > > + if (commit && !entry) { > > + return NULL; > > + } > > + if (entry && !OvsCtAddEntry(entry, ctx, natInfo, currentTime)) { > [Alin Serdean] Shouldn't we delete the 'entry' here if OvsCtAddEntry failed? > > + return NULL; > > + } > > + OvsCtUpdateFlowKey(key, state, ctx->key.zone, 0, NULL); > > + return entry; > > invalid: > > state |= OVS_CS_F_INVALID; > > OvsCtUpdateFlowKey(key, state, ctx->key.zone, 0, NULL); @@ > > -269,11 > > +289,11 @@ invalid: > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
