On 12/28/17, 5:28 AM, "[email protected] on behalf of Flavio
Leitner" <[email protected] on behalf of [email protected]> wrote:
Hi Darrell,
I am coming back from vacations, sorry the delay.
The patch doesn't apply anymore, but I reviewed anyways.
Thank you Flavio !
This patch was superceded and now outdated, but let me fold your comments for
V3,
since there is now a request to combine some weakly related patches including
this one into a series.
Cheers Darrell
On Sun, Nov 19, 2017 at 01:02:19PM -0800, Darrell Ball wrote:
> Fix up some instances where variable declarations were not close
> enough to their use, as these were missed before. This is the
> preferred art in OVS code and flagged heavily in code reviews.
> This is highly desirable due to code clarity reasons.
>
> There are also some cases where newlines were not needed by prior art
> and some cases where they were needed but missed.
>
> There was one case where there was a missing space after "}".
>
> There were a few cases where for loop index decalrations could be
^^^^^^^^^^^^
typo.
Thanks
> folded into the loop.
>
> Signed-off-by: Darrell Ball <[email protected]>
> ---
> lib/conntrack.c | 190
+++++++++++++++++++++++++-------------------------------
> 1 file changed, 85 insertions(+), 105 deletions(-)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index f5a3aa9..b8d0e77 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -295,10 +295,10 @@ conntrack_init(struct conntrack *ct)
>
> for (i = 0; i < CONNTRACK_BUCKETS; i++) {
> struct conntrack_bucket *ctb = &ct->buckets[i];
> -
> ct_lock_init(&ctb->lock);
> ct_lock_lock(&ctb->lock);
> hmap_init(&ctb->connections);
> +
> for (j = 0; j < ARRAY_SIZE(ctb->exp_lists); j++) {
> ovs_list_init(&ctb->exp_lists[j]);
> }
> @@ -319,17 +319,16 @@ conntrack_init(struct conntrack *ct)
> void
> conntrack_destroy(struct conntrack *ct)
> {
> - unsigned i;
> -
> latch_set(&ct->clean_thread_exit);
> pthread_join(ct->clean_thread, NULL);
> latch_destroy(&ct->clean_thread_exit);
> - for (i = 0; i < CONNTRACK_BUCKETS; i++) {
> - struct conntrack_bucket *ctb = &ct->buckets[i];
> - struct conn *conn;
>
> + for (unsigned i = 0; i < CONNTRACK_BUCKETS; i++) {
> + struct conntrack_bucket *ctb = &ct->buckets[i];
> ovs_mutex_destroy(&ctb->cleanup_mutex);
> ct_lock_lock(&ctb->lock);
> +
> + struct conn *conn;
> HMAP_FOR_EACH_POP (conn, node, &ctb->connections) {
> if (conn->conn_type == CT_CONN_TYPE_DEFAULT) {
> atomic_count_dec(&ct->n_conn);
> @@ -390,7 +389,6 @@ write_ct_md(struct dp_packet *pkt, uint16_t zone,
const struct conn *conn,
Add empty line here?
> pkt->md.ct_orig_tuple_ipv6 = false;
and here?
Thanks, for both
> if (key) {
> if (key->dl_type == htons(ETH_TYPE_IP)) {
> -
> pkt->md.ct_orig_tuple.ipv4 = (struct ovs_key_ct_tuple_ipv4) {
> key->src.addr.ipv4_aligned,
> key->dst.addr.ipv4_aligned,
> @@ -447,14 +445,13 @@ is_ftp_ctl(const struct dp_packet *pkt)
> return (ip_proto == IPPROTO_TCP &&
> (th->tcp_src == htons(CT_IPPORT_FTP) ||
> th->tcp_dst == htons(CT_IPPORT_FTP)));
> -
> }
>
> static bool
> is_tftp_ctl(const struct dp_packet *pkt)
> {
> - uint8_t ip_proto = get_ip_proto(pkt);
> - struct udp_header *uh = dp_packet_l4(pkt);
> + uint8_t ip_proto = get_ip_proto(pkt);
> + struct udp_header *uh = dp_packet_l4(pkt);
>
> /* CT_IPPORT_TFTP is used because IPPORT_TFTP in not defined in OSX,
> * at least in in.h. Since this value will never change, remove
> @@ -462,7 +459,6 @@ is_tftp_ctl(const struct dp_packet *pkt)
> #define CT_IPPORT_TFTP 69
> return (ip_proto == IPPROTO_UDP &&
> uh->udp_dst == htons(CT_IPPORT_TFTP));
> -
> }
>
> static void
> @@ -599,7 +595,6 @@ reverse_nat_packet(struct dp_packet *pkt, const
struct conn *conn)
> struct ip_header *inner_l3 = (struct ip_header *) (icmp + 1);
> extract_l3_ipv4(&inner_key, inner_l3, tail - ((char *)inner_l3)
- pad,
> &inner_l4, false);
> -
> pkt->l3_ofs += (char *) inner_l3 - (char *) nh;
> pkt->l4_ofs += inner_l4 - (char *) icmp;
>
> @@ -610,6 +605,7 @@ reverse_nat_packet(struct dp_packet *pkt, const
struct conn *conn)
> packet_set_ipv4_addr(pkt, &inner_l3->ip_dst,
> conn->key.dst.addr.ipv4_aligned);
> }
> +
> reverse_pat_packet(pkt, conn);
> icmp->icmp_csum = 0;
> icmp->icmp_csum = csum(icmp, tail - (char *) icmp - pad);
> @@ -712,6 +708,7 @@ conn_seq_skew_set(struct conntrack *ct, const struct
conn_key *key,
> unsigned bucket = hash_to_bucket(hash);
> ct_lock_lock(&ct->buckets[bucket].lock);
> struct conn *conn = conn_lookup(ct, key, now);
> +
> if (conn && seq_skew) {
> conn->seq_skew = seq_skew;
> conn->seq_skew_dir = seq_skew_dir;
> @@ -724,7 +721,6 @@ nat_clean(struct conntrack *ct, struct conn *conn,
> struct conntrack_bucket *ctb)
> OVS_REQUIRES(ctb->lock)
> {
> - long long now = time_msec();
> ct_rwlock_wrlock(&ct->resources_lock);
> nat_conn_keys_remove(&ct->nat_conn_keys, &conn->rev_key,
ct->hash_basis);
> ct_rwlock_unlock(&ct->resources_lock);
> @@ -736,8 +732,8 @@ nat_clean(struct conntrack *ct, struct conn *conn,
Not sure about this chunk because it has empty lines separating
declarations from code which isn't the same in other functions.
Good point – I removed 3 empty lines.
> ct_lock_lock(&ct->buckets[bucket_rev_conn].lock);
> ct_rwlock_wrlock(&ct->resources_lock);
>
> + long long now = time_msec();
> struct conn *rev_conn = conn_lookup(ct, &conn->rev_key, now);
> -
> struct nat_conn_key_node *nat_conn_key_node =
> nat_conn_keys_lookup(&ct->nat_conn_keys, &conn->rev_key,
> ct->hash_basis);
> @@ -751,8 +747,8 @@ nat_clean(struct conntrack *ct, struct conn *conn,
> &rev_conn->node);
> free(rev_conn);
> }
> - delete_conn(conn);
>
> + delete_conn(conn);
> ct_rwlock_unlock(&ct->resources_lock);
> ct_lock_unlock(&ct->buckets[bucket_rev_conn].lock);
> ct_lock_lock(&ctb->lock);
> @@ -779,24 +775,23 @@ conn_not_found(struct conntrack *ct, struct
dp_packet *pkt,
> struct conn_lookup_ctx *ctx, bool commit, long long now,
> const struct nat_action_info_t *nat_action_info,
> struct conn *conn_for_un_nat_copy,
> - const char *helper,
> - const struct alg_exp_node *alg_exp)
> + const char *helper, const struct alg_exp_node *alg_exp)
> {
> - unsigned bucket = hash_to_bucket(ctx->hash);
> struct conn *nc = NULL;
>
> if (!valid_new(pkt, &ctx->key)) {
> pkt->md.ct_state = CS_INVALID;
> return nc;
> }
> +
> pkt->md.ct_state = CS_NEW;
> +
> if (alg_exp) {
> pkt->md.ct_state |= CS_RELATED;
> }
>
> if (commit) {
> unsigned int n_conn_limit;
> -
> atomic_read_relaxed(&ct->n_conn_limit, &n_conn_limit);
>
> if (atomic_count_get(&ct->n_conn) >= n_conn_limit) {
> @@ -804,6 +799,7 @@ conn_not_found(struct conntrack *ct, struct dp_packet
*pkt,
> return nc;
> }
>
> + unsigned bucket = hash_to_bucket(ctx->hash);
> nc = new_conn(&ct->buckets[bucket], pkt, &ctx->key, now);
> ctx->conn = nc;
> nc->rev_key = nc->key;
> @@ -847,8 +843,8 @@ conn_not_found(struct conntrack *ct, struct dp_packet
*pkt,
> } else {
> *conn_for_un_nat_copy = *nc;
> ct_rwlock_wrlock(&ct->resources_lock);
> - bool nat_res = nat_select_range_tuple(
> - ct, nc, conn_for_un_nat_copy);
> + bool nat_res = nat_select_range_tuple(ct, nc,
> +
conn_for_un_nat_copy);
>
> if (!nat_res) {
> goto nat_res_exhaustion;
> @@ -929,6 +925,7 @@ conn_update_state(struct conntrack *ct, struct
dp_packet *pkt,
> OVS_NOT_REACHED();
> }
> }
> +
> return create_new_conn;
> }
>
> @@ -962,7 +959,6 @@ create_un_nat_conn(struct conntrack *ct, struct conn
*conn_for_un_nat_copy,
> nat_conn_keys_lookup(&ct->nat_conn_keys, &nc->key,
ct->hash_basis);
> if (nat_conn_key_node && !conn_key_cmp(&nat_conn_key_node->value,
> &nc->rev_key) && !rev_conn) {
> -
> hmap_insert(&ct->buckets[un_nat_conn_bucket].connections,
> &nc->node, un_nat_hash);
> } else {
> @@ -1022,7 +1018,6 @@ check_orig_tuple(struct conntrack *ct, struct
dp_packet *pkt,
> if (ctx_in->key.dl_type == htons(ETH_TYPE_IP)) {
> ctx.key.src.addr.ipv4_aligned =
pkt->md.ct_orig_tuple.ipv4.ipv4_src;
> ctx.key.dst.addr.ipv4_aligned =
pkt->md.ct_orig_tuple.ipv4.ipv4_dst;
> -
> if (ctx_in->key.nw_proto == IPPROTO_ICMP) {
> ctx.key.src.icmp_id = ctx_in->key.src.icmp_id;
> ctx.key.dst.icmp_id = ctx_in->key.dst.icmp_id;
> @@ -1037,7 +1032,6 @@ check_orig_tuple(struct conntrack *ct, struct
dp_packet *pkt,
> } else {
> ctx.key.src.addr.ipv6_aligned =
pkt->md.ct_orig_tuple.ipv6.ipv6_src;
> ctx.key.dst.addr.ipv6_aligned =
pkt->md.ct_orig_tuple.ipv6.ipv6_dst;
> -
> if (ctx_in->key.nw_proto == IPPROTO_ICMPV6) {
> ctx.key.src.icmp_id = ctx_in->key.src.icmp_id;
> ctx.key.dst.icmp_id = ctx_in->key.dst.icmp_id;
> @@ -1053,7 +1047,6 @@ check_orig_tuple(struct conntrack *ct, struct
dp_packet *pkt,
>
> ctx.key.dl_type = ctx_in->key.dl_type;
> ctx.key.zone = pkt->md.ct_zone;
> -
> ctx.hash = conn_key_hash(&ctx.key, ct->hash_basis);
> *bucket = hash_to_bucket(ctx.hash);
> ct_lock_lock(&ct->buckets[*bucket].lock);
> @@ -1091,17 +1084,13 @@ process_one(struct conntrack *ct, struct
dp_packet *pkt,
>
> if (OVS_LIKELY(conn)) {
> if (conn->conn_type == CT_CONN_TYPE_UN_NAT) {
> -
> ctx->reply = true;
> -
> struct conn_lookup_ctx ctx2;
> ctx2.conn = NULL;
> ctx2.key = conn->rev_key;
> ctx2.hash = conn_key_hash(&conn->rev_key, ct->hash_basis);
> -
> ct_lock_unlock(&ct->buckets[bucket].lock);
> bucket = hash_to_bucket(ctx2.hash);
> -
> ct_lock_lock(&ct->buckets[bucket].lock);
> conn_key_lookup(&ct->buckets[bucket], &ctx2, now);
>
> @@ -1145,11 +1134,9 @@ process_one(struct conntrack *ct, struct dp_packet
*pkt,
> if (nat_action_info && !create_new_conn) {
> handle_nat(pkt, conn, zone, ctx->reply, ctx->icmp_related);
> }
> -
> - }else if (check_orig_tuple(ct, pkt, ctx, now, &bucket, &conn,
> - nat_action_info)) {
> - create_new_conn = conn_update_state(ct, pkt, ctx, &conn, now,
> - bucket);
> + } else if (check_orig_tuple(ct, pkt, ctx, now, &bucket, &conn,
> + nat_action_info)) {
> + create_new_conn = conn_update_state(ct, pkt, ctx, &conn, now,
bucket);
> } else {
> if (ctx->icmp_related) {
> /* An icmp related conn should always be found; no new
> @@ -1161,6 +1148,7 @@ process_one(struct conntrack *ct, struct dp_packet
*pkt,
> }
>
> const struct alg_exp_node *alg_exp = NULL;
> +
> if (OVS_UNLIKELY(create_new_conn)) {
> struct alg_exp_node alg_exp_entry;
>
> @@ -1172,11 +1160,9 @@ process_one(struct conntrack *ct, struct dp_packet
*pkt,
> alg_exp = &alg_exp_entry;
> }
> ct_rwlock_unlock(&ct->resources_lock);
> -
> conn = conn_not_found(ct, pkt, ctx, commit, now, nat_action_info,
> &conn_for_un_nat_copy, helper, alg_exp);
> }
> -
> write_ct_md(pkt, zone, conn, &ctx->key, alg_exp);
>
> if (conn && setmark) {
> @@ -1265,7 +1251,6 @@ set_label(struct dp_packet *pkt, struct conn *conn,
>
> memcpy(&v, val, sizeof v);
> memcpy(&m, mask, sizeof m);
> -
> pkt->md.ct_label.u64.lo = v.u64.lo
> | (pkt->md.ct_label.u64.lo & ~(m.u64.lo));
> pkt->md.ct_label.u64.hi = v.u64.hi
looks like it is missing the empty line after 'ovs_u128 v, m;'
It seems the blank line is there at least in V2, but I noted it.
> @@ -1286,10 +1271,9 @@ sweep_bucket(struct conntrack *ct, struct
conntrack_bucket *ctb,
> {
> struct conn *conn, *next;
> long long min_expiration = LLONG_MAX;
> - unsigned i;
> size_t count = 0;
>
> - for (i = 0; i < N_CT_TM; i++) {
> + for (unsigned i = 0; i < N_CT_TM; i++) {
> LIST_FOR_EACH_SAFE (conn, next, exp_node, &ctb->exp_lists[i]) {
> if (conn->conn_type == CT_CONN_TYPE_DEFAULT) {
> if (!conn_expired(conn, now) || count >= limit) {
> @@ -1311,6 +1295,7 @@ sweep_bucket(struct conntrack *ct, struct
conntrack_bucket *ctb,
> size_t alg_exp_count = hmap_count(&ct->alg_expectations);
> /* XXX: revisit this. */
> size_t max_to_expire = MAX(alg_exp_count/10, MAX_ALG_EXP_TO_EXPIRE);
> +
Why?
Right, I was doing this for what I guessed others liked. Let me note this for
future reference more generally though.
This block is now gone since the expectation expiry is changed in another patch.
> count = 0;
> ct_rwlock_wrlock(&ct->resources_lock);
> struct alg_exp_node *alg_exp_node, *alg_exp_node_next;
> @@ -1340,11 +1325,10 @@ conntrack_clean(struct conntrack *ct, long long
now)
> long long next_wakeup = now + CT_TM_MIN;
> unsigned int n_conn_limit;
> size_t clean_count = 0;
> - unsigned i;
>
> atomic_read_relaxed(&ct->n_conn_limit, &n_conn_limit);
>
> - for (i = 0; i < CONNTRACK_BUCKETS; i++) {
> + for (unsigned i = 0; i < CONNTRACK_BUCKETS; i++) {
> struct conntrack_bucket *ctb = &ct->buckets[i];
> size_t prev_count;
> long long min_exp;
> @@ -1371,7 +1355,6 @@ conntrack_clean(struct conntrack *ct, long long now)
> }
>
> ct_lock_unlock(&ctb->lock);
> -
> ctb->next_cleanup = MIN(min_exp, now + CT_TM_MIN);
>
> next_bucket:
> @@ -1417,7 +1400,6 @@ clean_thread_main(void *f_)
> long long now = time_msec();
>
> next_wake = conntrack_clean(ct, now);
> -
Shouldn't this chunk be removing the empty line after 'now'
declaration instead?
Oops; you are right
> if (next_wake < now) {
> poll_timer_wait_until(now + CT_CLEAN_MIN_INTERVAL);
> } else {
> @@ -1443,7 +1425,6 @@ extract_l3_ipv4(struct conn_key *key, const void
*data, size_t size,
> const char **new_data, bool validate_checksum)
> {
> const struct ip_header *ip = data;
> - size_t ip_len;
>
> if (new_data) {
> if (OVS_UNLIKELY(size < IP_HEADER_LEN)) {
> @@ -1451,7 +1432,7 @@ extract_l3_ipv4(struct conn_key *key, const void
*data, size_t size,
> }
> }
>
> - ip_len = IP_IHL(ip->ip_ihl_ver) * 4;
> + size_t ip_len = IP_IHL(ip->ip_ihl_ver) * 4;
>
> if (new_data) {
> if (OVS_UNLIKELY(ip_len < IP_HEADER_LEN)) {
> @@ -1496,24 +1477,23 @@ extract_l3_ipv6(struct conn_key *key, const void
*data, size_t size,
> }
> }
>
> - uint8_t nw_proto = ip6->ip6_nxt;
> - uint8_t nw_frag = 0;
> -
> data = ip6 + 1;
> size -= sizeof *ip6;
> + uint8_t nw_proto = ip6->ip6_nxt;
> + uint8_t nw_frag = 0;
>
> if (!parse_ipv6_ext_hdrs(&data, &size, &nw_proto, &nw_frag)) {
> return false;
> }
>
> - if (new_data) {
> - *new_data = data;
> - }
> -
> if (nw_frag) {
> return false;
> }
>
> + if (new_data) {
> + *new_data = data;
> + }
> +
This is doing more than style changes and should not be included.
It is a non-functional change, but I can splice out as a separate patch, if you
feel strongly about it.
> key->src.addr.ipv6 = ip6->ip6_src;
> key->dst.addr.ipv6 = ip6->ip6_dst;
> key->nw_proto = nw_proto;
> @@ -1567,6 +1547,7 @@ check_l4_udp(const struct conn_key *key, const void
*data, size_t size,
> }
>
> size_t udp_len = ntohs(udp->udp_len);
> +
> if (OVS_UNLIKELY(udp_len < UDP_HEADER_LEN || udp_len > size)) {
> return false;
> }
> @@ -1592,12 +1573,11 @@ check_l4_icmp6(const struct conn_key *key, const
void *data, size_t size,
> static inline bool
> extract_l4_tcp(struct conn_key *key, const void *data, size_t size)
> {
> - const struct tcp_header *tcp = data;
> -
> if (OVS_UNLIKELY(size < TCP_HEADER_LEN)) {
> return false;
> }
>
> + const struct tcp_header *tcp = data;
> key->src.port = tcp->tcp_src;
> key->dst.port = tcp->tcp_dst;
>
> @@ -1608,12 +1588,11 @@ extract_l4_tcp(struct conn_key *key, const void
*data, size_t size)
> static inline bool
> extract_l4_udp(struct conn_key *key, const void *data, size_t size)
> {
> - const struct udp_header *udp = data;
> -
> if (OVS_UNLIKELY(size < UDP_HEADER_LEN)) {
> return false;
> }
>
> + const struct udp_header *udp = data;
> key->src.port = udp->udp_src;
> key->dst.port = udp->udp_dst;
>
> @@ -1657,12 +1636,12 @@ static inline int
> extract_l4_icmp(struct conn_key *key, const void *data, size_t size,
> bool *related)
> {
> - const struct icmp_header *icmp = data;
> -
> if (OVS_UNLIKELY(size < ICMP_HEADER_LEN)) {
> return false;
> }
>
> + const struct icmp_header *icmp = data;
> +
> switch (icmp->icmp_type) {
> case ICMP4_ECHO_REQUEST:
> case ICMP4_ECHO_REPLY:
> @@ -1776,7 +1755,6 @@ extract_l4_icmp6(struct conn_key *key, const void
*data, size_t size,
> const char *l3 = (const char *) icmp6 + 8;
> const char *tail = (const char *) data + size;
> const char *l4 = NULL;
> - bool ok;
>
> if (!related) {
> return false;
> @@ -1784,7 +1762,7 @@ extract_l4_icmp6(struct conn_key *key, const void
*data, size_t size,
>
> memset(&inner_key, 0, sizeof inner_key);
> inner_key.dl_type = htons(ETH_TYPE_IPV6);
> - ok = extract_l3_ipv6(&inner_key, l3, tail - l3, &l4);
> + bool ok = extract_l3_ipv6(&inner_key, l3, tail - l3, &l4);
> if (!ok) {
> return false;
> }
> @@ -1855,8 +1833,6 @@ conn_key_extract(struct conntrack *ct, struct
dp_packet *pkt, ovs_be16 dl_type,
> const struct eth_header *l2 = dp_packet_eth(pkt);
> const struct ip_header *l3 = dp_packet_l3(pkt);
> const char *l4 = dp_packet_l4(pkt);
> - const char *tail = dp_packet_tail(pkt);
> - bool ok;
>
> memset(ctx, 0, sizeof *ctx);
>
> @@ -1898,9 +1874,11 @@ conn_key_extract(struct conntrack *ct, struct
dp_packet *pkt, ovs_be16 dl_type,
> * we use a sparse representation (miniflow).
> *
> */
> + const char *tail = dp_packet_tail(pkt);
> + bool ok;
> ctx->key.dl_type = dl_type;
Add an empty line?
right
> if (ctx->key.dl_type == htons(ETH_TYPE_IP)) {
> - bool hwol_bad_l3_csum = dp_packet_ip_checksum_bad(pkt);
> + bool hwol_bad_l3_csum = dp_packet_ip_checksum_bad(pkt);
> if (hwol_bad_l3_csum) {
> ok = false;
> } else {
and then a bit lower (not visible here), there are two empty lines
before 'if (ok)'
> @@ -1951,7 +1929,6 @@ static uint32_t
> conn_key_hash(const struct conn_key *key, uint32_t basis)
> {
> uint32_t hsrc, hdst, hash;
> -
> hsrc = hdst = basis;
> hsrc = ct_endpoint_hash_add(hsrc, &key->src);
> hdst = ct_endpoint_hash_add(hdst, &key->dst);
> @@ -1970,9 +1947,7 @@ conn_key_hash(const struct conn_key *key, uint32_t
basis)
> static void
> conn_key_reverse(struct conn_key *key)
> {
> - struct ct_endpoint tmp;
> -
> - tmp = key->src;
> + struct ct_endpoint tmp = key->src;
> key->src = key->dst;
> key->dst = tmp;
> }
> @@ -1985,18 +1960,19 @@ nat_ipv6_addrs_delta(struct in6_addr
*ipv6_aligned_min,
> uint8_t *ipv6_min_lo = &ipv6_aligned_min->s6_addr[0] +
sizeof(uint64_t);
> uint8_t *ipv6_max_hi = &ipv6_aligned_max->s6_addr[0];
> uint8_t *ipv6_max_lo = &ipv6_aligned_max->s6_addr[0] +
sizeof(uint64_t);
> -
> ovs_be64 addr6_64_min_hi;
> ovs_be64 addr6_64_min_lo;
> +
> memcpy(&addr6_64_min_hi, ipv6_min_hi, sizeof addr6_64_min_hi);
> memcpy(&addr6_64_min_lo, ipv6_min_lo, sizeof addr6_64_min_lo);
> -
> ovs_be64 addr6_64_max_hi;
> ovs_be64 addr6_64_max_lo;
> +
> memcpy(&addr6_64_max_hi, ipv6_max_hi, sizeof addr6_64_max_hi);
> memcpy(&addr6_64_max_lo, ipv6_max_lo, sizeof addr6_64_max_lo);
>
> uint64_t diff;
> +
> if (addr6_64_min_hi == addr6_64_max_hi &&
> ntohll(addr6_64_min_lo) <= ntohll(addr6_64_max_lo)) {
> diff = ntohll(addr6_64_max_lo) - ntohll(addr6_64_min_lo);
missing empty line before the next if (diff > 0xfffffffe) { ...
thanks
> @@ -2025,6 +2001,7 @@ nat_ipv6_addr_increment(struct in6_addr
*ipv6_aligned, uint32_t increment)
> uint8_t *ipv6_lo = &ipv6_aligned->s6_addr[0] + sizeof(ovs_be64);
> ovs_be64 addr6_64_hi;
> ovs_be64 addr6_64_lo;
> +
> memcpy(&addr6_64_hi, ipv6_hi, sizeof addr6_64_hi);
> memcpy(&addr6_64_lo, ipv6_lo, sizeof addr6_64_lo);
>
> @@ -2048,16 +2025,13 @@ static uint32_t
> nat_range_hash(const struct conn *conn, uint32_t basis)
> {
> uint32_t hash = basis;
> -
> hash = ct_addr_hash_add(hash, &conn->nat_info->min_addr);
> hash = ct_addr_hash_add(hash, &conn->nat_info->max_addr);
> hash = hash_add(hash,
> (conn->nat_info->max_port << 16)
> | conn->nat_info->min_port);
> -
> hash = ct_endpoint_hash_add(hash, &conn->key.src);
> hash = ct_endpoint_hash_add(hash, &conn->key.dst);
> -
> hash = hash_add(hash, (OVS_FORCE uint32_t) conn->key.dl_type);
> hash = hash_add(hash, conn->key.nw_proto);
> hash = hash_add(hash, conn->key.zone);
> @@ -2078,7 +2052,6 @@ nat_select_range_tuple(struct conntrack *ct, const
struct conn *conn,
> uint16_t min_port;
> uint16_t max_port;
> uint16_t first_port;
> -
> uint32_t hash = nat_range_hash(conn, ct->hash_basis);
>
> if ((conn->nat_info->nat_action & NAT_ACTION_SRC) &&
> @@ -2201,7 +2174,6 @@ nat_conn_keys_lookup(struct hmap *nat_conn_keys,
> {
> struct nat_conn_key_node *nat_conn_key_node;
> uint32_t nat_conn_key_hash = conn_key_hash(key, basis);
> -
> HMAP_FOR_EACH_WITH_HASH (nat_conn_key_node, node, nat_conn_key_hash,
> nat_conn_keys) {
> if (!conn_key_cmp(&nat_conn_key_node->key, key)) {
> @@ -2218,7 +2190,6 @@ nat_conn_keys_insert(struct hmap *nat_conn_keys,
const struct conn *nat_conn,
> {
> struct nat_conn_key_node *nat_conn_key_node =
> nat_conn_keys_lookup(nat_conn_keys, &nat_conn->rev_key, basis);
> -
> if (!nat_conn_key_node) {
> struct nat_conn_key_node *nat_conn_key = xzalloc(sizeof
*nat_conn_key);
> nat_conn_key->key = nat_conn->rev_key;
> @@ -2239,7 +2210,6 @@ nat_conn_keys_remove(struct hmap *nat_conn_keys,
> {
> struct nat_conn_key_node *nat_conn_key_node;
> uint32_t nat_conn_key_hash = conn_key_hash(key, basis);
> -
> HMAP_FOR_EACH_WITH_HASH (nat_conn_key_node, node, nat_conn_key_hash,
> nat_conn_keys) {
> if (!conn_key_cmp(&nat_conn_key_node->key, key)) {
> @@ -2303,9 +2273,7 @@ static struct conn *
> new_conn(struct conntrack_bucket *ctb, struct dp_packet *pkt,
> struct conn_key *key, long long now)
> {
> - struct conn *newconn;
> -
> - newconn = l4_protos[key->nw_proto]->new_conn(ctb, pkt, now);
> + struct conn *newconn = l4_protos[key->nw_proto]->new_conn(ctb, pkt,
now);
>
> if (newconn) {
> newconn->key = *key;
> @@ -2362,24 +2330,20 @@ static void
> conn_to_ct_dpif_entry(const struct conn *conn, struct ct_dpif_entry
*entry,
> long long now, int bkt)
> {
> - struct ct_l4_proto *class;
> - long long expiration;
> memset(entry, 0, sizeof *entry);
> conn_key_to_tuple(&conn->key, &entry->tuple_orig);
> conn_key_to_tuple(&conn->rev_key, &entry->tuple_reply);
> -
> entry->zone = conn->key.zone;
> entry->mark = conn->mark;
> -
> memcpy(&entry->labels, &conn->label, sizeof entry->labels);
> /* Not implemented yet */
> entry->timestamp.start = 0;
> entry->timestamp.stop = 0;
>
> - expiration = conn->expiration - now;
> + long long expiration = conn->expiration - now;
> entry->timeout = (expiration > 0) ? expiration / 1000 : 0;
>
> - class = l4_protos[conn->key.nw_proto];
> + struct ct_l4_proto *class = l4_protos[conn->key.nw_proto];
> if (class->conn_get_protoinfo) {
> class->conn_get_protoinfo(conn, &entry->protoinfo);
> }
> @@ -2397,12 +2361,13 @@ conntrack_dump_start(struct conntrack *ct, struct
conntrack_dump *dump,
> const uint16_t *pzone, int *ptot_bkts)
> {
> memset(dump, 0, sizeof(*dump));
> +
> if (pzone) {
> dump->zone = *pzone;
> dump->filter_zone = true;
> }
> - dump->ct = ct;
>
> + dump->ct = ct;
> *ptot_bkts = CONNTRACK_BUCKETS;
>
> return 0;
> @@ -2493,7 +2458,6 @@ expectation_lookup(struct hmap *alg_expectations,
> struct conn_key check_key = *key;
> check_key.src.port = ALG_WC_SRC_PORT;
> struct alg_exp_node *alg_exp_node;
> -
> uint32_t alg_exp_conn_key_hash = conn_key_hash(&check_key, basis);
> HMAP_FOR_EACH_WITH_HASH (alg_exp_node, node,
> alg_exp_conn_key_hash,
> @@ -2558,9 +2522,9 @@ expectation_create(struct conntrack *ct,
> }
>
> alg_exp_node->alg_nat_repl_addr = alg_nat_repl_addr;
> - uint32_t alg_exp_conn_key_hash =
> - conn_key_hash(&alg_exp_node->key,
> - ct->hash_basis);
> + uint32_t alg_exp_conn_key_hash = conn_key_hash(&alg_exp_node->key,
> + ct->hash_basis);
> +
> hmap_insert(&ct->alg_expectations,
> &alg_exp_node->node,
> alg_exp_conn_key_hash);
> @@ -2606,7 +2570,6 @@ repl_ftp_v4_addr(struct dp_packet *pkt, ovs_be32
v4_addr_rep,
>
> size_t remain_size = tcp_payload_length(pkt) -
> addr_offset_from_ftp_data_start;
> -
> int overall_delta = 0;
> char *byte_str = ftp_data_start + addr_offset_from_ftp_data_start;
>
> @@ -2614,6 +2577,7 @@ repl_ftp_v4_addr(struct dp_packet *pkt, ovs_be32
v4_addr_rep,
> for (uint8_t i = 0; i < 4; i++) {
> /* Find the end of the string for this octet. */
> char *next_delim = memchr(byte_str, ',', 4);
> +
> ovs_assert(next_delim);
> int substr_size = next_delim - byte_str;
> remain_size -= substr_size;
> @@ -2675,7 +2639,6 @@ static enum ftp_ctl_pkt
> detect_ftp_ctl_type(const struct conn_lookup_ctx *ctx,
> struct dp_packet *pkt)
> {
> -
> char ftp_msg[LARGEST_FTP_MSG_OF_INTEREST + 1] = {0};
> get_ftp_ctl_msg(pkt, ftp_msg);
> if (ctx->key.dl_type == htons(ETH_TYPE_IPV6)) {
> @@ -2708,9 +2671,9 @@ process_ftp_ctl_v4(struct conntrack *ct,
> *ftp_data_v4_start = tcp_hdr + tcp_hdr_len;
> char ftp_msg[LARGEST_FTP_MSG_OF_INTEREST + 1] = {0};
> get_ftp_ctl_msg(pkt, ftp_msg);
> -
> char *ftp = ftp_msg;
> enum ct_alg_mode mode;
> +
> if (!strncasecmp(ftp, FTP_PORT_CMD, strlen(FTP_PORT_CMD))) {
> ftp = ftp_msg + strlen(FTP_PORT_CMD);
> mode = CT_FTP_MODE_ACTIVE;
> @@ -2733,8 +2696,8 @@ process_ftp_ctl_v4(struct conntrack *ct,
>
> char *ip_addr_start = ftp;
> *addr_offset_from_ftp_data_start = ip_addr_start - ftp_msg;
> - uint8_t comma_count = 0;
>
> + uint8_t comma_count = 0;
> while (comma_count < 4 && *ftp) {
> if (*ftp == ',') {
> comma_count++;
> @@ -2757,6 +2720,7 @@ process_ftp_ctl_v4(struct conntrack *ct,
> }
>
> char *save_ftp = ftp;
> +
> ftp = terminate_number_str(ftp, MAX_FTP_PORT_DGTS);
> if (!ftp) {
> return CT_FTP_CTL_INVALID;
> @@ -2773,11 +2737,11 @@ process_ftp_ctl_v4(struct conntrack *ct,
>
> uint16_t port_hs = value;
> port_hs <<= 8;
> -
> /* Skip over comma. */
> ftp++;
> save_ftp = ftp;
> bool digit_found = false;
> +
> while (isdigit(*ftp)) {
> ftp++;
> digit_found = true;
> @@ -2798,6 +2762,7 @@ process_ftp_ctl_v4(struct conntrack *ct,
> if (65535 - port_hs < port_lo_hs) {
> return CT_FTP_CTL_INVALID;
> }
> +
> port_hs |= port_lo_hs;
> ovs_be16 port = htons(port_hs);
> ovs_be32 conn_ipv4_addr;
> @@ -2816,8 +2781,7 @@ process_ftp_ctl_v4(struct conntrack *ct,
> OVS_NOT_REACHED();
> }
>
> - ovs_be32 ftp_ipv4_addr;
> - ftp_ipv4_addr = ip_addr.s_addr;
> + ovs_be32 ftp_ipv4_addr = ip_addr.s_addr;
> /* Although most servers will block this exploit, there may be some
> * less well managed. */
> if (ftp_ipv4_addr != conn_ipv4_addr && ftp_ipv4_addr !=
*v4_addr_rep) {
> @@ -2857,25 +2821,30 @@ process_ftp_ctl_v6(struct conntrack *ct,
>
> char *ftp = ftp_msg;
> struct in6_addr ip6_addr;
> +
> if (!strncasecmp(ftp, FTP_EPRT_CMD, strlen(FTP_EPRT_CMD))) {
> ftp = ftp_msg + strlen(FTP_EPRT_CMD);
> ftp = skip_non_digits(ftp);
> +
> if (*ftp != FTP_AF_V6 || isdigit(ftp[1])) {
> return CT_FTP_CTL_INVALID;
> }
> +
> /* Jump over delimiter. */
> ftp += 2;
>
> - char *ip_addr_start = ftp;
> memset(&ip6_addr, 0, sizeof ip6_addr);
> + char *ip_addr_start = ftp;
> *addr_offset_from_ftp_data_start = ip_addr_start - ftp_msg;
> ftp = skip_ipv6_digits(ftp);
> *ftp = 0;
> *addr_size = ftp - ip_addr_start;
> +
> int rc2 = inet_pton(AF_INET6, ip_addr_start, &ip6_addr);
> if (rc2 != 1) {
> return CT_FTP_CTL_INVALID;
> }
> +
> ftp++;
> *mode = CT_FTP_MODE_ACTIVE;
> } else {
> @@ -2893,14 +2862,17 @@ process_ftp_ctl_v6(struct conntrack *ct,
> }
>
> char *save_ftp = ftp;
> +
> ftp = terminate_number_str(ftp, MAX_EXT_FTP_PORT_DGTS);
> if (!ftp) {
> return CT_FTP_CTL_INVALID;
> }
> +
> int value;
> if (!str_to_int(save_ftp, 10, &value)) {
> return CT_FTP_CTL_INVALID;
> }
> +
> if (value > CT_MAX_L4_PORT) {
> return CT_FTP_CTL_INVALID;
> }
> @@ -2947,6 +2919,7 @@ repl_ftp_v6_addr(struct dp_packet *pkt, struct
ct_addr v6_addr_rep,
> /* Do conservative check for pathological MTU usage. */
> uint32_t orig_used_size = dp_packet_size(pkt);
> uint16_t allocated_size = dp_packet_get_allocated(pkt);
> +
> if (orig_used_size + MAX_FTP_V6_NAT_DELTA > allocated_size) {
> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
> VLOG_WARN_RL(&rl, "Unsupported effective MTU %u used with FTP",
> @@ -2956,6 +2929,7 @@ repl_ftp_v6_addr(struct dp_packet *pkt, struct
ct_addr v6_addr_rep,
>
> const char *rc;
> char v6_addr_str[IPV6_SCAN_LEN] = {0};
> +
> rc = inet_ntop(AF_INET6, &v6_addr_rep.ipv6_aligned, v6_addr_str,
> IPV6_SCAN_LEN - 1);
> ovs_assert(rc != NULL);
> @@ -2966,6 +2940,7 @@ repl_ftp_v6_addr(struct dp_packet *pkt, struct
ct_addr v6_addr_rep,
> addr_offset_from_ftp_data_start;
>
> char *pkt_addr_str = ftp_data_start +
addr_offset_from_ftp_data_start;
> +
> replace_substring(pkt_addr_str, addr_size, remain_size,
> v6_addr_str, replace_addr_size);
>
> @@ -3000,18 +2975,19 @@ handle_ftp_ctl(struct conntrack *ct, const struct
conn_lookup_ctx *ctx,
>
> struct ovs_16aligned_ip6_hdr *nh6 = dp_packet_l3(pkt);
> int64_t seq_skew = 0;
> +
> if (ftp_ctl == CT_FTP_CTL_OTHER) {
> seq_skew = conn_for_expectation->seq_skew;
> } else if (ftp_ctl == CT_FTP_CTL_INTEREST) {
> enum ftp_ctl_pkt rc;
> if (ctx->key.dl_type == htons(ETH_TYPE_IPV6)) {
> - rc = process_ftp_ctl_v6(ct, pkt, conn_for_expectation,
> - now, &v6_addr_rep, &ftp_data_start,
> + rc = process_ftp_ctl_v6(ct, pkt, conn_for_expectation, now,
> + &v6_addr_rep, &ftp_data_start,
> &addr_offset_from_ftp_data_start,
> &addr_size, &mode);
> } else {
> - rc = process_ftp_ctl_v4(ct, pkt, conn_for_expectation,
> - now, &v4_addr_rep, &ftp_data_start,
> + rc = process_ftp_ctl_v4(ct, pkt, conn_for_expectation, now,
> + &v4_addr_rep, &ftp_data_start,
> &addr_offset_from_ftp_data_start);
> }
> if (rc == CT_FTP_CTL_INVALID) {
> @@ -3021,6 +2997,7 @@ handle_ftp_ctl(struct conntrack *ct, const struct
conn_lookup_ctx *ctx,
> return;
> } else if (rc == CT_FTP_CTL_INTEREST) {
> uint16_t ip_len;
> +
> if (ctx->key.dl_type == htons(ETH_TYPE_IPV6)) {
> seq_skew = repl_ftp_v6_addr(pkt, v6_addr_rep,
ftp_data_start,
>
addr_offset_from_ftp_data_start,
> @@ -3053,9 +3030,9 @@ handle_ftp_ctl(struct conntrack *ct, const struct
conn_lookup_ctx *ctx,
> }
>
> struct tcp_header *th = dp_packet_l4(pkt);
> +
> if (do_seq_skew_adj && seq_skew != 0) {
> if (ctx->reply != conn_for_expectation->seq_skew_dir) {
> -
> uint32_t tcp_ack = ntohl(get_16aligned_be32(&th->tcp_ack));
>
> if ((seq_skew > 0) && (tcp_ack < seq_skew)) {
> @@ -3070,6 +3047,7 @@ handle_ftp_ctl(struct conntrack *ct, const struct
conn_lookup_ctx *ctx,
> put_16aligned_be32(&th->tcp_ack, new_tcp_ack);
> } else {
> uint32_t tcp_seq = ntohl(get_16aligned_be32(&th->tcp_seq));
> +
> if ((seq_skew > 0) && (UINT32_MAX - tcp_seq < seq_skew)) {
> tcp_seq = seq_skew - (UINT32_MAX - tcp_seq);
> } else if ((seq_skew < 0) && (tcp_seq < -seq_skew)) {
> @@ -3083,15 +3061,17 @@ handle_ftp_ctl(struct conntrack *ct, const struct
conn_lookup_ctx *ctx,
> }
> }
>
> - const char *tail = dp_packet_tail(pkt);
> - uint8_t pad = dp_packet_l2_pad_size(pkt);
> th->tcp_csum = 0;
> uint32_t tcp_csum;
> +
> if (ctx->key.dl_type == htons(ETH_TYPE_IPV6)) {
> tcp_csum = packet_csum_pseudoheader6(nh6);
> } else {
> tcp_csum = packet_csum_pseudoheader(l3_hdr);
> }
> +
> + const char *tail = dp_packet_tail(pkt);
> + uint8_t pad = dp_packet_l2_pad_size(pkt);
> th->tcp_csum = csum_finish(
> csum_continue(tcp_csum, th, tail - (char *) th - pad));
> return;
otherwise it looks good to me.
Thank you
--
Flavio
_______________________________________________
dev mailing list
[email protected]
https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=2GVGwyFIspoCtXFCSqJczRWFdIX179VsFNczawLFwy4&s=vHh2bIPRzi3iqzsc0WDasV4ao318xltIe3tycEwelJs&e=
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
