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

[Alin Serdean]

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:
>