Simon Horman <[email protected]> writes:

> On Wed, Feb 07, 2024 at 06:38:08PM +0100, Paolo Valerio wrote:
>> The patch, when 'persistent' flag is specified, makes the IP selection
>> in a range persistent across reboots.
>> 
>> Signed-off-by: Paolo Valerio <[email protected]>
>
> Hi Paolo,
>
> I have some minor nits below - which you can feel free to take or leave.
> But overall this looks good to me.
>
> Acked-by: Simon Horman <[email protected]>
>
> ...
>
>> diff --git a/lib/conntrack.c b/lib/conntrack.c
>
> ...
>
>> @@ -2386,12 +2390,23 @@ nat_get_unique_tuple(struct conntrack *ct, struct 
>> conn *conn,
>>      bool pat_proto = fwd_key->nw_proto == IPPROTO_TCP ||
>>                       fwd_key->nw_proto == IPPROTO_UDP ||
>>                       fwd_key->nw_proto == IPPROTO_SCTP;
>> +    uint32_t hash, port_off, basis = ct->hash_basis;
>>      uint16_t min_dport, max_dport, curr_dport;
>>      uint16_t min_sport, max_sport, curr_sport;
>> -    uint32_t hash, port_off;
>>  
>> -    hash = nat_range_hash(fwd_key, ct->hash_basis, nat_info);
>> -    port_off = nat_info->nat_flags & NAT_RANGE_RANDOM ? random_uint32() : 
>> hash;
>> +    if (nat_info->nat_flags & NAT_PERSISTENT) {
>> +        basis = 0;
>> +    }
>
> nit: maybe it is nicer to set basis only once.
>
>     basis = (nat_info->nat_flags & NAT_PERSISTENT) ? 0 : ct->hash_basis;
>
>> +
>> +    hash = nat_range_hash(fwd_key, basis, nat_info);
>> +
>> +    if (nat_info->nat_flags & NAT_RANGE_RANDOM) {
>> +        port_off = random_uint32();
>> +    } else {
>> +        port_off =
>> +            basis ? hash : nat_range_hash(fwd_key, ct->hash_basis, 
>> nat_info);
>> +    }
>> +
>
> nit: maybe this is a little easier on the eyes (completely untested!)?
>
>     if (nat_info->nat_flags & NAT_RANGE_RANDOM) {
>         port_off = random_uint32();
>     } else if (basis) {
>         port_off = hash;
>     } else {
>         port_off = nat_range_hash(fwd_key, ct->hash_basis, nat_info);
>     }
>

thanks Simon for taking a look.
Agreed, looks easier on the eyes. I included your suggestions and your
acks in v3.

I guess the above solve Aaron's suggestions as well.

>>      min_addr = nat_info->min_addr;
>>      max_addr = nat_info->max_addr;
>>  
>
> ...

_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to