On Fri, 17 Nov 2017 19:34:49 +0000 Darrell Ball <db...@vmware.com> wrote:
> This patch mostly breaks OVS coding style in many instances and also invents > its own coding guidelines. > > 1/OVS prefers variable declarations near their usage and your patch violates > extensively. Documentation/internals/contributing/coding-style.rst: - Mixing of declarations and code within a block. Please use this judiciously; keep declarations nicely grouped together in the beginning of a block if possible. so OVS and I disagree with you. > Most of this patch is related to this. > I’ll point out a few to provide examples, but I don’t like this – nack. > > 2/In many instances where this patch moves “&&” at beginning of next line > rather than at end of line > There is no coding style for this and different OVS code uses both styles. > I prefer to have the operator at the end of line as it makes it clear there > is a continuation. It's your preference again. Note the examples from Documentation/internals/contributing/coding-style.rst """ Do not parenthesize the operands of ``&&`` and ``||`` unless operator precedence makes it necessary, or unless the operands are themselves expressions that use ``&&`` and ``||``. Thus: :: if (!isdigit((unsigned char)s[0]) || !isdigit((unsigned char)s[1]) || !isdigit((unsigned char)s[2])) { printf("string %s does not start with 3-digit code\n", s); } but :: if (rule && (!best || rule->priority > best->priority)) { best = rule; } Do parenthesize a subexpression that must be split across more than one line, e.g.: :: *idxp = ((l1_idx << PORT_ARRAY_L1_SHIFT) | (l2_idx << PORT_ARRAY_L2_SHIFT) | (l3_idx << PORT_ARRAY_L3_SHIFT)); """ > There is a missing space after “}” in one instance - thanks Could you point it? I will gladly spin a v2. > There are also use of full 79 line lengths in a few places, which looked ok, > but I did not check carefully. > There is some missed extra newlines after declarations, which generally looks > ok; I’ll check more however. > I also see some extra newlines removed which looked ok, but I’ll check more. > > I’ll submit my own patch since I don’t agree with “1” and “2”. > > > On 11/17/17, 9:52 AM, "ovs-dev-boun...@openvswitch.org on behalf of Flavio > Leitner" <ovs-dev-boun...@openvswitch.org on behalf of f...@sysclose.org> > wrote: > > No functional change, just fixing coding style. > > Signed-off-by: Flavio Leitner <f...@sysclose.org> > --- > lib/conntrack.c | 335 > +++++++++++++++++++++++++++++++------------------------- > 1 file changed, 188 insertions(+), 147 deletions(-) > > diff --git a/lib/conntrack.c b/lib/conntrack.c > index f5a3aa9fa..022a0737b 100644 > --- a/lib/conntrack.c > +++ b/lib/conntrack.c > @@ -320,6 +320,8 @@ void > conntrack_destroy(struct conntrack *ct) > { > unsigned i; > + struct nat_conn_key_node *nat_conn_key_node; > + struct alg_exp_node *alg_exp_node; > > OVS prefers variable declarations near their usage Not sure the size of your screen but to me it looks near enough. > latch_set(&ct->clean_thread_exit); > pthread_join(ct->clean_thread, NULL); > @@ -341,13 +343,11 @@ conntrack_destroy(struct conntrack *ct) > ct_lock_destroy(&ctb->lock); > } > ct_rwlock_wrlock(&ct->resources_lock); > - struct nat_conn_key_node *nat_conn_key_node; > HMAP_FOR_EACH_POP (nat_conn_key_node, node, &ct->nat_conn_keys) { > free(nat_conn_key_node); > } > hmap_destroy(&ct->nat_conn_keys); > > - struct alg_exp_node *alg_exp_node; > HMAP_FOR_EACH_POP (alg_exp_node, node, &ct->alg_expectations) { > free(alg_exp_node); > } > @@ -444,9 +444,9 @@ is_ftp_ctl(const struct dp_packet *pkt) > * the external dependency. */ > #define CT_IPPORT_FTP 21 > > - return (ip_proto == IPPROTO_TCP && > - (th->tcp_src == htons(CT_IPPORT_FTP) || > - th->tcp_dst == htons(CT_IPPORT_FTP))); > + return (ip_proto == IPPROTO_TCP > + && (th->tcp_src == htons(CT_IPPORT_FTP) > + || th->tcp_dst == htons(CT_IPPORT_FTP))); > > I see you prefer the operator at start of line > There is no coding guideline for this and most OVS code uses both operator at > end of line > and beginning of line > I am also curious why you take exception to this only for this file; what is > the reason ? See above. > > > > } > > @@ -460,8 +460,8 @@ is_tftp_ctl(const struct dp_packet *pkt) > * at least in in.h. Since this value will never change, remove > * the external dependency. */ > #define CT_IPPORT_TFTP 69 > - return (ip_proto == IPPROTO_UDP && > - uh->udp_dst == htons(CT_IPPORT_TFTP)); > + return (ip_proto == IPPROTO_UDP > + && uh->udp_dst == htons(CT_IPPORT_TFTP)); > > same comment as above > > } > > @@ -696,10 +696,12 @@ static struct conn * > conn_lookup(struct conntrack *ct, const struct conn_key *key, long long > now) > { > struct conn_lookup_ctx ctx; > + unsigned bucket; > + > ctx.conn = NULL; > ctx.key = *key; > ctx.hash = conn_key_hash(key, ct->hash_basis); > - unsigned bucket = hash_to_bucket(ctx.hash); > + bucket = hash_to_bucket(ctx.hash); > conn_key_lookup(&ct->buckets[bucket], &ctx, now); > return ctx.conn; > > same comment as above > > > } > @@ -710,8 +712,10 @@ conn_seq_skew_set(struct conntrack *ct, const struct > conn_key *key, > { > uint32_t hash = conn_key_hash(key, ct->hash_basis); > unsigned bucket = hash_to_bucket(hash); > + struct conn *conn; > + > ct_lock_lock(&ct->buckets[bucket].lock); > - struct conn *conn = conn_lookup(ct, key, now); > + conn = conn_lookup(ct, key, now); > if (conn && seq_skew) { > conn->seq_skew = seq_skew; > conn->seq_skew_dir = seq_skew_dir; > @@ -724,23 +728,26 @@ nat_clean(struct conntrack *ct, struct conn *conn, > struct conntrack_bucket *ctb) > OVS_REQUIRES(ctb->lock) > { > + struct nat_conn_key_node *nat_conn_key_node; > + struct conn *rev_conn; > long long now = time_msec(); > + uint32_t hash_rev_conn; > + unsigned bucket_rev_conn; > + > 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); > ct_lock_unlock(&ctb->lock); > > - uint32_t hash_rev_conn = conn_key_hash(&conn->rev_key, > ct->hash_basis); > - unsigned bucket_rev_conn = hash_to_bucket(hash_rev_conn); > + hash_rev_conn = conn_key_hash(&conn->rev_key, ct->hash_basis); > + bucket_rev_conn = hash_to_bucket(hash_rev_conn); > > ct_lock_lock(&ct->buckets[bucket_rev_conn].lock); > ct_rwlock_wrlock(&ct->resources_lock); > > - 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); > + rev_conn = conn_lookup(ct, &conn->rev_key, now); > + nat_conn_key_node = nat_conn_keys_lookup(&ct->nat_conn_keys, > + &conn->rev_key, > ct->hash_basis); > > /* In the unlikely event, rev conn was recreated, then skip > * rev_conn cleanup. */ > @@ -784,6 +791,7 @@ conn_not_found(struct conntrack *ct, struct dp_packet > *pkt, > { > unsigned bucket = hash_to_bucket(ctx->hash); > struct conn *nc = NULL; > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5); > > if (!valid_new(pkt, &ctx->key)) { > pkt->md.ct_state = CS_INVALID; > @@ -824,6 +832,8 @@ conn_not_found(struct conntrack *ct, struct dp_packet > *pkt, > nc->nat_info = xmemdup(nat_action_info, sizeof > *nc->nat_info); > > if (alg_exp) { > + bool new_insert; > + > if (alg_exp->passive_mode) { > nc->rev_key.dst.addr = alg_exp->alg_nat_repl_addr; > nc->nat_info->nat_action = NAT_ACTION_SRC; > @@ -833,9 +843,9 @@ conn_not_found(struct conntrack *ct, struct dp_packet > *pkt, > } > *conn_for_un_nat_copy = *nc; > ct_rwlock_wrlock(&ct->resources_lock); > - bool new_insert = > nat_conn_keys_insert(&ct->nat_conn_keys, > - > conn_for_un_nat_copy, > - ct->hash_basis); > + new_insert = nat_conn_keys_insert(&ct->nat_conn_keys, > + conn_for_un_nat_copy, > + ct->hash_basis); > ct_rwlock_unlock(&ct->resources_lock); > if (!new_insert) { > char *log_msg = xasprintf("Pre-existing alg " > @@ -845,11 +855,11 @@ conn_not_found(struct conntrack *ct, struct > dp_packet *pkt, > free(log_msg); > } > } else { > + bool nat_res; > + > *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); > - > + nat_res = nat_select_range_tuple(ct, nc, > conn_for_un_nat_copy); > if (!nat_res) { > goto nat_res_exhaustion; > } > @@ -884,7 +894,6 @@ nat_res_exhaustion: > * this point on unused. */ > memset(conn_for_un_nat_copy, 0, sizeof *conn_for_un_nat_copy); > ct_rwlock_unlock(&ct->resources_lock); > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5); > VLOG_WARN_RL(&rl, "Unable to NAT due to tuple space exhaustion - " > "if DoS attack, use firewalling and/or zone > partitioning."); > return NULL; > @@ -904,11 +913,12 @@ conn_update_state(struct conntrack *ct, struct > dp_packet *pkt, > pkt->md.ct_state |= CS_REPLY_DIR; > } > } else { > + enum ct_update_res res; > + > if ((*conn)->alg_related) { > pkt->md.ct_state |= CS_RELATED; > } > - enum ct_update_res res = conn_update(*conn, &ct->buckets[bucket], > - pkt, ctx->reply, now); > + res = conn_update(*conn, &ct->buckets[bucket], pkt, ctx->reply, > now); > > switch (res) { > case CT_UPDATE_VALID: > @@ -956,10 +966,11 @@ create_un_nat_conn(struct conntrack *ct, struct > conn *conn_for_un_nat_copy, > free(nc); > } > } else { > - ct_rwlock_rdlock(&ct->resources_lock); > + struct nat_conn_key_node *nat_conn_key_node; > > - struct nat_conn_key_node *nat_conn_key_node = > - nat_conn_keys_lookup(&ct->nat_conn_keys, &nc->key, > ct->hash_basis); > + ct_rwlock_rdlock(&ct->resources_lock); > + nat_conn_key_node = 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) { > > @@ -983,9 +994,9 @@ handle_nat(struct dp_packet *pkt, struct conn *conn, > uint16_t zone, bool reply, bool related) > { > if (conn->nat_info && > - (!(pkt->md.ct_state & (CS_SRC_NAT | CS_DST_NAT)) || > - (pkt->md.ct_state & (CS_SRC_NAT | CS_DST_NAT) && > - zone != pkt->md.ct_zone))) { > + (!(pkt->md.ct_state & (CS_SRC_NAT | CS_DST_NAT)) > + || (pkt->md.ct_state & (CS_SRC_NAT | CS_DST_NAT) > + && zone != pkt->md.ct_zone))) { > > if (pkt->md.ct_state & (CS_SRC_NAT | CS_DST_NAT)) { > pkt->md.ct_state &= ~(CS_SRC_NAT | CS_DST_NAT); > @@ -1005,17 +1016,25 @@ check_orig_tuple(struct conntrack *ct, struct > dp_packet *pkt, > const struct nat_action_info_t *nat_action_info) > OVS_REQUIRES(ct->buckets[*bucket].lock) > { > - if ((ctx_in->key.dl_type == htons(ETH_TYPE_IP) && > - !pkt->md.ct_orig_tuple.ipv4.ipv4_proto) || > - (ctx_in->key.dl_type == htons(ETH_TYPE_IPV6) && > - !pkt->md.ct_orig_tuple.ipv6.ipv6_proto) || > - !(pkt->md.ct_state & (CS_SRC_NAT | CS_DST_NAT)) || > - nat_action_info) { > + struct conn_lookup_ctx ctx; > + uint16_t src_port; > + > + if (ctx_in->key.dl_type == htons(ETH_TYPE_IP) > + && !pkt->md.ct_orig_tuple.ipv4.ipv4_proto) { > + return false; > + } > + if (ctx_in->key.dl_type == htons(ETH_TYPE_IPV6) > + && !pkt->md.ct_orig_tuple.ipv6.ipv6_proto) { > + return false; > + } > + if (!(pkt->md.ct_state & (CS_SRC_NAT | CS_DST_NAT))) { > + return false; > + } > > I don’t like expanding this and this is not coding standard. > These conditions should be kept together > We have lots of examples in OVS of selection with multiple conditions. Code should be easy to read and I can quickly tell you what it is doing with the patch, but I am willing to drop this chunk if others think it's not good enough. > > > > + if (nat_action_info) { > return false; > } > > ct_lock_unlock(&ct->buckets[*bucket].lock); > - struct conn_lookup_ctx ctx; > memset(&ctx, 0 , sizeof ctx); > ctx.conn = NULL; > > @@ -1026,7 +1045,7 @@ check_orig_tuple(struct conntrack *ct, struct > dp_packet *pkt, > 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; > - uint16_t src_port = > ntohs(pkt->md.ct_orig_tuple.ipv4.src_port); > + src_port = ntohs(pkt->md.ct_orig_tuple.ipv4.src_port); > ctx.key.src.icmp_type = (uint8_t) src_port; > ctx.key.dst.icmp_type = > reverse_icmp_type(ctx.key.src.icmp_type); > } else { > @@ -1041,7 +1060,7 @@ check_orig_tuple(struct conntrack *ct, struct > dp_packet *pkt, > 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; > - uint16_t src_port = > ntohs(pkt->md.ct_orig_tuple.ipv6.src_port); > + src_port = ntohs(pkt->md.ct_orig_tuple.ipv6.src_port); > ctx.key.src.icmp_type = (uint8_t) src_port; > ctx.key.dst.icmp_type = > reverse_icmp6_type(ctx.key.src.icmp_type); > } else { > @@ -1053,7 +1072,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); > @@ -1077,8 +1095,17 @@ process_one(struct conntrack *ct, struct dp_packet > *pkt, > const struct nat_action_info_t *nat_action_info, > const char *helper) > { > + const struct alg_exp_node *alg_exp; > struct conn *conn; > + struct conn_lookup_ctx ctx2; > + struct conn conn_for_un_nat_copy; > + struct conn conn_for_expectation; > + conn_for_un_nat_copy.conn_type = CT_CONN_TYPE_DEFAULT; > unsigned bucket = hash_to_bucket(ctx->hash); > + bool create_new_conn; > + bool ftp_ctl; > + bool tftp_ctl; > > > same comment as above > > + > ct_lock_lock(&ct->buckets[bucket].lock); > conn_key_lookup(&ct->buckets[bucket], ctx, now); > conn = ctx->conn; > @@ -1091,17 +1118,12 @@ 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); > > @@ -1118,11 +1140,9 @@ process_one(struct conntrack *ct, struct dp_packet > *pkt, > } > } > > - bool create_new_conn = false; > - struct conn conn_for_un_nat_copy; > + create_new_conn = false; > conn_for_un_nat_copy.conn_type = CT_CONN_TYPE_DEFAULT; > - bool ftp_ctl = is_ftp_ctl(pkt); > - > + ftp_ctl = is_ftp_ctl(pkt); > if (OVS_LIKELY(conn)) { > if (ftp_ctl) { > /* Keep sequence tracking in sync with the source of the > @@ -1146,7 +1166,7 @@ process_one(struct conntrack *ct, struct dp_packet > *pkt, > handle_nat(pkt, conn, zone, ctx->reply, ctx->icmp_related); > } > > - }else if (check_orig_tuple(ct, pkt, ctx, now, &bucket, &conn, > + } else if (check_orig_tuple(ct, pkt, ctx, now, &bucket, &conn, > > ok, space was missing after “}” > > nat_action_info)) { > create_new_conn = conn_update_state(ct, pkt, ctx, &conn, now, > bucket); > @@ -1160,7 +1180,7 @@ process_one(struct conntrack *ct, struct dp_packet > *pkt, > } > } > > - const struct alg_exp_node *alg_exp = NULL; > + alg_exp = NULL; > if (OVS_UNLIKELY(create_new_conn)) { > struct alg_exp_node alg_exp_entry; > > @@ -1182,13 +1202,10 @@ process_one(struct conntrack *ct, struct > dp_packet *pkt, > if (conn && setmark) { > set_mark(pkt, conn, setmark[0], setmark[1]); > } > - > > don’t remove this space > > if (conn && setlabel) { > set_label(pkt, conn, &setlabel[0], &setlabel[1]); > } > - > - bool tftp_ctl = is_tftp_ctl(pkt); > - struct conn conn_for_expectation; > + tftp_ctl = is_tftp_ctl(pkt); > if (OVS_UNLIKELY((ftp_ctl || tftp_ctl) && conn)) { > conn_for_expectation = *conn; > } > @@ -1198,7 +1215,6 @@ process_one(struct conntrack *ct, struct dp_packet > *pkt, > if (is_un_nat_conn_valid(&conn_for_un_nat_copy)) { > create_un_nat_conn(ct, &conn_for_un_nat_copy, now, !!alg_exp); > } > - > > don’t remove this space > > /* FTP control packet handling with expectation creation. */ > if (OVS_UNLIKELY(ftp_ctl && conn)) { > handle_ftp_ctl(ct, ctx, pkt, &conn_for_expectation, > @@ -1285,9 +1301,14 @@ sweep_bucket(struct conntrack *ct, struct > conntrack_bucket *ctb, > OVS_REQUIRES(ctb->lock) > { > struct conn *conn, *next; > + struct alg_exp_node *alg_exp_node; > + struct alg_exp_node *alg_exp_node_next; > long long min_expiration = LLONG_MAX; > unsigned i; > size_t count = 0; > + size_t alg_exp_count; > + size_t max_to_expire; > + enum { MAX_ALG_EXP_TO_EXPIRE = 1000 }; > > for (i = 0; i < N_CT_TM; i++) { > LIST_FOR_EACH_SAFE (conn, next, exp_node, &ctb->exp_lists[i]) { > @@ -1307,13 +1328,11 @@ sweep_bucket(struct conntrack *ct, struct > conntrack_bucket *ctb, > } > } > > - enum { MAX_ALG_EXP_TO_EXPIRE = 1000 }; > - size_t alg_exp_count = hmap_count(&ct->alg_expectations); > + 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); > + max_to_expire = MAX(alg_exp_count/10, MAX_ALG_EXP_TO_EXPIRE); > count = 0; > ct_rwlock_wrlock(&ct->resources_lock); > - struct alg_exp_node *alg_exp_node, *alg_exp_node_next; > LIST_FOR_EACH_SAFE (alg_exp_node, alg_exp_node_next, > exp_node, &ct->alg_exp_list) { > if (now < alg_exp_node->expiration || count >= max_to_expire) { > @@ -1489,6 +1508,8 @@ extract_l3_ipv6(struct conn_key *key, const void > *data, size_t size, > const char **new_data) > { > const struct ovs_16aligned_ip6_hdr *ip6 = data; > + uint8_t nw_proto = ip6->ip6_nxt; > + uint8_t nw_frag = 0; > > if (new_data) { > if (OVS_UNLIKELY(size < sizeof *ip6)) { > @@ -1496,20 +1517,14 @@ 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; > - > if (!parse_ipv6_ext_hdrs(&data, &size, &nw_proto, &nw_frag)) { > return false; > } > - > if (new_data) { > *new_data = data; > } > - > if (nw_frag) { > return false; > } > @@ -1545,11 +1560,13 @@ check_l4_tcp(const struct conn_key *key, const > void *data, size_t size, > const void *l3, bool validate_checksum) > { > const struct tcp_header *tcp = data; > + size_t tcp_len; > + > if (size < sizeof *tcp) { > return false; > } > > - size_t tcp_len = TCP_OFFSET(tcp->tcp_ctl) * 4; > + tcp_len = TCP_OFFSET(tcp->tcp_ctl) * 4; > if (OVS_UNLIKELY(tcp_len < TCP_HEADER_LEN || tcp_len > size)) { > return false; > } > @@ -1562,11 +1579,13 @@ check_l4_udp(const struct conn_key *key, const > void *data, size_t size, > const void *l3, bool validate_checksum) > { > const struct udp_header *udp = data; > + size_t udp_len; > + > if (size < sizeof *udp) { > return false; > } > > - size_t udp_len = ntohs(udp->udp_len); > + udp_len = ntohs(udp->udp_len); > if (OVS_UNLIKELY(udp_len < UDP_HEADER_LEN || udp_len > size)) { > return false; > } > @@ -1901,10 +1920,12 @@ conn_key_extract(struct conntrack *ct, struct > dp_packet *pkt, ovs_be16 dl_type, > ctx->key.dl_type = dl_type; > if (ctx->key.dl_type == htons(ETH_TYPE_IP)) { > bool hwol_bad_l3_csum = dp_packet_ip_checksum_bad(pkt); > + > if (hwol_bad_l3_csum) { > ok = false; > } else { > bool hwol_good_l3_csum = dp_packet_ip_checksum_valid(pkt); > + > > No such coding standard; lots of code in OVS where comment is embedded in > contiguous lines > Why do you not like it here only ? This separates declarations from code, which is done in all other places. > > /* Validate the checksum only when hwol is not supported. */ > ok = extract_l3_ipv4(&ctx->key, l3, tail - (char *) l3, NULL, > !hwol_good_l3_csum); > @@ -1918,8 +1939,10 @@ conn_key_extract(struct conntrack *ct, struct > dp_packet *pkt, ovs_be16 dl_type, > > if (ok) { > bool hwol_bad_l4_csum = dp_packet_l4_checksum_bad(pkt); > + > if (!hwol_bad_l4_csum) { > bool hwol_good_l4_csum = dp_packet_l4_checksum_valid(pkt); > + > > No such coding standard; lots of code in OVS where comment is embedded in > contiguous lines > Why do you not like it here only ? > > /* Validate the checksum only when hwol is not supported. */ > if (extract_l4(&ctx->key, l4, tail - l4, &ctx->icmp_related, > l3, > !hwol_good_l4_csum)) { > @@ -1985,6 +2008,7 @@ 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); > + uint64_t diff; > > ovs_be64 addr6_64_min_hi; > ovs_be64 addr6_64_min_lo; > @@ -1996,7 +2020,6 @@ nat_ipv6_addrs_delta(struct in6_addr > *ipv6_aligned_min, > 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); > @@ -2023,6 +2046,7 @@ nat_ipv6_addr_increment(struct in6_addr > *ipv6_aligned, uint32_t increment) > { > uint8_t *ipv6_hi = &ipv6_aligned->s6_addr[0]; > uint8_t *ipv6_lo = &ipv6_aligned->s6_addr[0] + sizeof(ovs_be64); > + > > don’t add newline > > ovs_be64 addr6_64_hi; > ovs_be64 addr6_64_lo; > memcpy(&addr6_64_hi, ipv6_hi, sizeof addr6_64_hi); > @@ -2075,10 +2099,13 @@ nat_select_range_tuple(struct conntrack *ct, > const struct conn *conn, > enum { MIN_NAT_EPHEMERAL_PORT = 1024, > MAX_NAT_EPHEMERAL_PORT = 65535 }; > > + struct ct_addr ct_addr; > + struct ct_addr max_ct_addr; > uint16_t min_port; > uint16_t max_port; > uint16_t first_port; > - > + uint32_t address_index; > + uint32_t deltaa; > uint32_t hash = nat_range_hash(conn, ct->hash_basis); > > if ((conn->nat_info->nat_action & NAT_ACTION_SRC) && > @@ -2099,14 +2126,10 @@ nat_select_range_tuple(struct conntrack *ct, > const struct conn *conn, > max_port = conn->nat_info->max_port; > } > > - uint32_t deltaa = 0; > - uint32_t address_index; > - struct ct_addr ct_addr; > + deltaa = 0; > memset(&ct_addr, 0, sizeof ct_addr); > - struct ct_addr max_ct_addr; > memset(&max_ct_addr, 0, sizeof max_ct_addr); > max_ct_addr = conn->nat_info->max_addr; > - > if (conn->key.dl_type == htons(ETH_TYPE_IP)) { > deltaa = ntohl(conn->nat_info->max_addr.ipv4_aligned) - > ntohl(conn->nat_info->min_addr.ipv4_aligned); > @@ -2134,6 +2157,8 @@ nat_select_range_tuple(struct conntrack *ct, const > struct conn *conn, > struct ct_addr first_addr = ct_addr; > > while (true) { > + bool new_insert; > + > if (conn->nat_info->nat_action & NAT_ACTION_SRC) { > nat_conn->rev_key.dst.addr = ct_addr; > } else { > @@ -2149,8 +2174,8 @@ nat_select_range_tuple(struct conntrack *ct, const > struct conn *conn, > nat_conn->rev_key.src.port = htons(port); > } > > - bool new_insert = nat_conn_keys_insert(&ct->nat_conn_keys, > nat_conn, > - ct->hash_basis); > + new_insert = nat_conn_keys_insert(&ct->nat_conn_keys, nat_conn, > + ct->hash_basis); > if (new_insert) { > return true; > } else if (!all_ports_tried) { > @@ -2216,15 +2241,17 @@ static bool > nat_conn_keys_insert(struct hmap *nat_conn_keys, const struct conn > *nat_conn, > uint32_t basis) > { > - struct nat_conn_key_node *nat_conn_key_node = > - nat_conn_keys_lookup(nat_conn_keys, &nat_conn->rev_key, basis); > + struct nat_conn_key_node *nat_conn_key_node; > + struct nat_conn_key_node *nat_conn_key; > + uint32_t nat_conn_key_hash; > > + 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 = xzalloc(sizeof *nat_conn_key); > nat_conn_key->key = nat_conn->rev_key; > nat_conn_key->value = nat_conn->key; > - uint32_t nat_conn_key_hash = conn_key_hash(&nat_conn_key->key, > - basis); > + nat_conn_key_hash = conn_key_hash(&nat_conn_key->key, basis); > hmap_insert(nat_conn_keys, &nat_conn_key->node, > nat_conn_key_hash); > return true; > } > @@ -2262,13 +2289,13 @@ conn_key_lookup(struct conntrack_bucket *ctb, > struct conn_lookup_ctx *ctx, > > HMAP_FOR_EACH_WITH_HASH (conn, node, hash, &ctb->connections) { > if (!conn_key_cmp(&conn->key, &ctx->key) > - && !conn_expired(conn, now)) { > + && !conn_expired(conn, now)) { > ctx->conn = conn; > ctx->reply = false; > break; > } > if (!conn_key_cmp(&conn->rev_key, &ctx->key) > - && !conn_expired(conn, now)) { > + && !conn_expired(conn, now)) { > ctx->conn = conn; > ctx->reply = true; > break; > @@ -2306,7 +2333,6 @@ new_conn(struct conntrack_bucket *ctb, struct > dp_packet *pkt, > struct conn *newconn; > > newconn = l4_protos[key->nw_proto]->new_conn(ctb, pkt, now); > - > if (newconn) { > newconn->key = *key; > } > @@ -2413,14 +2439,12 @@ conntrack_dump_next(struct conntrack_dump *dump, > struct ct_dpif_entry *entry) > { > struct conntrack *ct = dump->ct; > long long now = time_msec(); > + struct hmap_node *node; > + struct conn *conn; > > while (dump->bucket < CONNTRACK_BUCKETS) { > - struct hmap_node *node; > - > ct_lock_lock(&ct->buckets[dump->bucket].lock); > for (;;) { > - struct conn *conn; > - > > > same comment as earlier > > > node = > hmap_at_position(&ct->buckets[dump->bucket].connections, > &dump->bucket_pos); > if (!node) { > @@ -2457,14 +2481,16 @@ int > conntrack_flush(struct conntrack *ct, const uint16_t *zone) > { > unsigned i; > + struct conn *conn; > + struct conn *next; > + struct alg_exp_node *alg_exp_node; > + struct alg_exp_node *alg_exp_node_next; > > for (i = 0; i < CONNTRACK_BUCKETS; i++) { > - struct conn *conn, *next; > - > ct_lock_lock(&ct->buckets[i].lock); > HMAP_FOR_EACH_SAFE (conn, next, node, > &ct->buckets[i].connections) { > - if ((!zone || *zone == conn->key.zone) && > - (conn->conn_type == CT_CONN_TYPE_DEFAULT)) { > + if ((!zone || *zone == conn->key.zone) > + && (conn->conn_type == CT_CONN_TYPE_DEFAULT)) { > > same comment as earlier > > conn_clean(ct, conn, &ct->buckets[i]); > } > } > @@ -2472,9 +2498,8 @@ conntrack_flush(struct conntrack *ct, const > uint16_t *zone) > } > > ct_rwlock_wrlock(&ct->resources_lock); > - struct alg_exp_node *alg_exp_node, *alg_exp_node_next; > - HMAP_FOR_EACH_SAFE (alg_exp_node, alg_exp_node_next, > - node, &ct->alg_expectations) { > + HMAP_FOR_EACH_SAFE (alg_exp_node, alg_exp_node_next, node, > + &ct->alg_expectations) { > if (!zone || *zone == alg_exp_node->key.zone) { > ovs_list_remove(&alg_exp_node->exp_node); > hmap_remove(&ct->alg_expectations, &alg_exp_node->node); > @@ -2493,8 +2518,8 @@ 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, > alg_expectations) { > @@ -2515,6 +2540,9 @@ expectation_create(struct conntrack *ct, > struct ct_addr src_addr; > struct ct_addr dst_addr; > struct ct_addr alg_nat_repl_addr; > + struct alg_exp_node *alg_exp; > + struct alg_exp_node *alg_exp_node; > + uint32_t alg_exp_conn_key_hash; > > switch (mode) { > case CT_FTP_MODE_ACTIVE: > @@ -2532,8 +2560,7 @@ expectation_create(struct conntrack *ct, > OVS_NOT_REACHED(); > } > > - struct alg_exp_node *alg_exp_node = > - xzalloc(sizeof *alg_exp_node); > + alg_exp_node = xzalloc(sizeof *alg_exp_node); > alg_exp_node->key.dl_type = master_conn->key.dl_type; > alg_exp_node->key.nw_proto = master_conn->key.nw_proto; > alg_exp_node->key.zone = master_conn->key.zone; > @@ -2549,8 +2576,8 @@ expectation_create(struct conntrack *ct, > * likely that the lookup will fail and > * expectation_create() will be called below. */ > ct_rwlock_wrlock(&ct->resources_lock); > - struct alg_exp_node *alg_exp = expectation_lookup( > - &ct->alg_expectations, &alg_exp_node->key, ct->hash_basis); > + alg_exp = expectation_lookup(&ct->alg_expectations, > &alg_exp_node->key, > + ct->hash_basis); > if (alg_exp) { > free(alg_exp_node); > ct_rwlock_unlock(&ct->resources_lock); > @@ -2558,13 +2585,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); > - hmap_insert(&ct->alg_expectations, > - &alg_exp_node->node, > + 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); > - > alg_exp_init_expiration(ct, alg_exp_node, now); > ct_rwlock_unlock(&ct->resources_lock); > } > @@ -2594,34 +2617,43 @@ repl_ftp_v4_addr(struct dp_packet *pkt, ovs_be32 > v4_addr_rep, > { > enum { MAX_FTP_V4_NAT_DELTA = 8 }; > > + char *byte_str; > + int overall_delta; > + size_t remain_size; > + uint8_t i; > /* 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_V4_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", > allocated_size); > return 0; > } > > - 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; > + remain_size = tcp_payload_length(pkt) - > addr_offset_from_ftp_data_start; > + overall_delta = 0; > + byte_str = ftp_data_start + addr_offset_from_ftp_data_start; > > /* Replace the existing IPv4 address by the new one. */ > - for (uint8_t i = 0; i < 4; i++) { > + for (i = 0; i < 4; i++) { > + char *next_delim; > + char rep_str[4]; > + int substr_size; > + int replace_size; > + uint8_t rep_byte; > + > /* Find the end of the string for this octet. */ > - char *next_delim = memchr(byte_str, ',', 4); > + next_delim = memchr(byte_str, ',', 4); > ovs_assert(next_delim); > - int substr_size = next_delim - byte_str; > + substr_size = next_delim - byte_str; > remain_size -= substr_size; > > /* Compose the new string for this octet, and replace it. */ > - char rep_str[4]; > - uint8_t rep_byte = get_v4_byte_be(v4_addr_rep, i); > - int replace_size = sprintf(rep_str, "%d", rep_byte); > + rep_byte = get_v4_byte_be(v4_addr_rep, i); > + replace_size = sprintf(rep_str, "%d", rep_byte); > > same comment as above w.r.t variable declaration near usage > > replace_substring(byte_str, substr_size, remain_size, > rep_str, replace_size); > overall_delta += replace_size - substr_size; > @@ -2647,6 +2679,7 @@ static char * > terminate_number_str(char *str, uint8_t max_digits) > { > uint8_t digits_found = 0; > + > while (isdigit(*str) && digits_found <= max_digits) { > str++; > digits_found++; > @@ -2667,8 +2700,7 @@ get_ftp_ctl_msg(struct dp_packet *pkt, char > *ftp_msg) > LARGEST_FTP_MSG_OF_INTEREST); > size_t tcp_hdr_len = TCP_OFFSET(th->tcp_ctl) * 4; > > - ovs_strlcpy(ftp_msg, tcp_hdr + tcp_hdr_len, > - tcp_payload_of_interest); > + ovs_strlcpy(ftp_msg, tcp_hdr + tcp_hdr_len, tcp_payload_of_interest); > > ok > > } > > static enum ftp_ctl_pkt > @@ -2677,15 +2709,16 @@ detect_ftp_ctl_type(const struct conn_lookup_ctx > *ctx, > { > > 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)) { > - if (strncasecmp(ftp_msg, FTP_EPRT_CMD, strlen(FTP_EPRT_CMD)) && > - !strcasestr(ftp_msg, FTP_EPSV_REPLY)) { > + if (strncasecmp(ftp_msg, FTP_EPRT_CMD, strlen(FTP_EPRT_CMD)) > + && !strcasestr(ftp_msg, FTP_EPSV_REPLY)) { > return CT_FTP_CTL_OTHER; > > same comment as above > > } > } else { > - if (strncasecmp(ftp_msg, FTP_PORT_CMD, strlen(FTP_PORT_CMD)) && > - strncasecmp(ftp_msg, FTP_PASV_REPLY_CODE, > + if (strncasecmp(ftp_msg, FTP_PORT_CMD, strlen(FTP_PORT_CMD)) > + && strncasecmp(ftp_msg, FTP_PASV_REPLY_CODE, > strlen(FTP_PASV_REPLY_CODE))) { > > same comment as above > > return CT_FTP_CTL_OTHER; > } > @@ -2939,38 +2972,41 @@ repl_ftp_v6_addr(struct dp_packet *pkt, struct > ct_addr v6_addr_rep, > { > /* This is slightly bigger than really possible. */ > enum { MAX_FTP_V6_NAT_DELTA = 45 }; > + int overall_delta; > + uint16_t allocated_size; > + uint32_t orig_used_size; > + size_t replace_addr_size; > + size_t remain_size; > + const char *rc; > + char *pkt_addr_str; > + char v6_addr_str[IPV6_SCAN_LEN] = {0}; > > if (mode == CT_FTP_MODE_PASSIVE) { > return 0; > } > > /* 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); > + orig_used_size = dp_packet_size(pkt); > + 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", > allocated_size); > return 0; > } > > - 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); > + IPV6_SCAN_LEN - 1); > ovs_assert(rc != NULL); > > - size_t replace_addr_size = strlen(v6_addr_str); > - > - size_t remain_size = tcp_payload_length(pkt) - > - 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); > - > - int overall_delta = (int) replace_addr_size - (int) addr_size; > + replace_addr_size = strlen(v6_addr_str); > + remain_size = tcp_payload_length(pkt) - > addr_offset_from_ftp_data_start; > + 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); > > + overall_delta = (int) replace_addr_size - (int) addr_size; > dp_packet_set_size(pkt, orig_used_size + overall_delta); > return overall_delta; > } > > same comment as above regarding variable declarations near usage > > @@ -2984,8 +3020,10 @@ handle_ftp_ctl(struct conntrack *ct, const struct > conn_lookup_ctx *ctx, > struct ip_header *l3_hdr = dp_packet_l3(pkt); > ovs_be32 v4_addr_rep = 0; > struct ct_addr v6_addr_rep; > + struct ovs_16aligned_ip6_hdr *nh6; > size_t addr_offset_from_ftp_data_start; > size_t addr_size = 0; > + int64_t seq_skew; > char *ftp_data_start; > bool do_seq_skew_adj = true; > enum ct_alg_mode mode = CT_FTP_MODE_ACTIVE; > @@ -2998,29 +3036,32 @@ handle_ftp_ctl(struct conntrack *ct, const struct > conn_lookup_ctx *ctx, > do_seq_skew_adj = false; > } > > - struct ovs_16aligned_ip6_hdr *nh6 = dp_packet_l3(pkt); > - int64_t seq_skew = 0; > + nh6 = dp_packet_l3(pkt); > + seq_skew = 0; > > > same comment as above regarding variable declarations near usage > > 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) { > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, > 5); > + > VLOG_WARN_RL(&rl, "Invalid FTP control packet format"); > pkt->md.ct_state |= CS_TRACKED | CS_INVALID; > 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, > @@ -3055,7 +3096,6 @@ 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 +3110,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)) { > -- > 2.13.6 > > > _______________________________________________ > dev mailing list > d...@openvswitch.org > > https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=Zhm7pwk8KMdO8CG7F7pKvJSPxz41jlXPmXL5aVVtreA&s=Y1k0-poJEUjrDsbCziMxKMJHZRIpYxk12oXjvtrgwlQ&e= > > -- Flavio _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev