Paolo Valerio <[email protected]> writes:
> The patch, when 'persistent' flag is specified, makes the IP selection
> in a range persistent across reboots.
>
> Signed-off-by: Paolo Valerio <[email protected]>
> ---
> NEWS | 3 ++-
> lib/conntrack.c | 27 +++++++++++++++++++++------
> lib/conntrack.h | 1 +
> lib/dpif-netdev.c | 2 ++
> 4 files changed, 26 insertions(+), 7 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 93046b963..0c86bba81 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -2,7 +2,8 @@ Post-v3.3.0
> --------------------
> - Userspace datapath:
> * Conntrack now supports 'random' flag for selecting ports in a range
> - while natting.
> + while natting and 'persistent' flag for selection of the IP address
> + from a range.
>
>
> v3.3.0 - xx xxx xxxx
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index e09ecdf33..7868a67f7 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -2202,17 +2202,21 @@ nat_range_hash(const struct conn_key *key, uint32_t
> basis,
> {
> uint32_t hash = basis;
>
> + if (!basis) {
> + hash = ct_addr_hash_add(hash, &key->src.addr);
> + } else {
> + hash = ct_endpoint_hash_add(hash, &key->src);
> + hash = ct_endpoint_hash_add(hash, &key->dst);
> + }
> +
> hash = ct_addr_hash_add(hash, &nat_info->min_addr);
> hash = ct_addr_hash_add(hash, &nat_info->max_addr);
> hash = hash_add(hash,
> ((uint32_t) nat_info->max_port << 16)
> | nat_info->min_port);
> - hash = ct_endpoint_hash_add(hash, &key->src);
> - hash = ct_endpoint_hash_add(hash, &key->dst);
> hash = hash_add(hash, (OVS_FORCE uint32_t) key->dl_type);
> hash = hash_add(hash, key->nw_proto);
> hash = hash_add(hash, key->zone);
> -
> /* The purpose of the second parameter is to distinguish hashes of data
> of
> * different length; our data always has the same length so there is no
> * value in counting. */
> @@ -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;
> + }
> +
> + 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);
> + }
> +
I'm probably misreading this, but the multiple 'if's is confusing me.
Maybe it would be better to write something like:
bool persist = !!(nat_info->nat_flags & NAT_PERSISTENT)
if (nat_info->nat_flags & NAT_RANGE_RANDOM) {
port_off = random_uint32();
} else {
port_off = !persist ?
nat_range_hash(fwd_key, ct->hash_basis, nat_info) :
nat_range_hash(fwd_key, 0, nat_info);
}
Especially because in the above code, ct->hash_basis == '0' would look
the same as persistent (which isn't obvious from the code).
At least, the multiple nested ifs make it difficult to follow what is
going on with basis call and nat_range_hash
Just read Simon's comments, and it's similar kind of comment I guess
:)
> min_addr = nat_info->min_addr;
> max_addr = nat_info->max_addr;
>
> diff --git a/lib/conntrack.h b/lib/conntrack.h
> index 9b0c6aa88..ee7da099e 100644
> --- a/lib/conntrack.h
> +++ b/lib/conntrack.h
> @@ -79,6 +79,7 @@ enum nat_action_e {
>
> enum nat_flags_e {
> NAT_RANGE_RANDOM = 1 << 0,
> + NAT_PERSISTENT = 1 << 1,
> };
>
> struct nat_action_info_t {
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index c3334c667..fbf7ccabd 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -9413,6 +9413,8 @@ dp_execute_cb(void *aux_, struct dp_packet_batch
> *packets_,
> nat_action_info.nat_flags |= NAT_RANGE_RANDOM;
> break;
> case OVS_NAT_ATTR_PERSISTENT:
> + nat_action_info.nat_flags |= NAT_PERSISTENT;
> + break;
> case OVS_NAT_ATTR_PROTO_HASH:
> break;
> case OVS_NAT_ATTR_UNSPEC:
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
