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
