[ovs-dev] [PATCH v7 3/4] datapath-windows: NAT integration with conntrack

[Alin Serdean]

No worries.

Let's get them in for the moment and fix them in incremental patches.

Thanks,
Alin.

> -----Original Message-----
> From: Yin Lin [mailto:linyi at vmware.com]
> Sent: Friday, June 9, 2017 6:29 AM
> To: Alin Serdean <aserdean at cloudbasesolutions.com>;
> dev at openvswitch.org; Sairam Venugopal <vsairam at vmware.com>
> 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:aserdean at cloudbasesolutions.com]
> Sent: Tuesday, May 23, 2017 7:18 AM
> To: Yin Lin <linyi at vmware.com>; dev at openvswitch.org
> 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 <linyi at vmware.com> 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:
> >