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
