Hi Darrell,
I am coming back from vacations, sorry the delay. The patch doesn't apply anymore, but I reviewed anyways. 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. > folded into the loop. > > Signed-off-by: Darrell Ball <dlu...@gmail.com> > --- > 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? > 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. > 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;' > @@ -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? > 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? > 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. > 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? > 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) { ... > @@ -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. -- Flavio _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev