| Message ID | 20171117175134.16617-1-fbl@sysclose.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] conntrack: coding style fixes. | expand |
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.
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.
There is a missing space after “}” in one instance - thanks
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-bounces@openvswitch.org on behalf of Flavio Leitner" <ovs-dev-bounces@openvswitch.org on behalf of fbl@sysclose.org> wrote:
No functional change, just fixing coding style.
Signed-off-by: Flavio Leitner <fbl@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
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 ?
}
@@ -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.
+ 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 ?
/* 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
dev@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=
On Fri, 17 Nov 2017 19:34:49 +0000 Darrell Ball <dball@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-bounces@openvswitch.org on behalf of Flavio Leitner" <ovs-dev-bounces@openvswitch.org on behalf of fbl@sysclose.org> wrote: > > No functional change, just fixing coding style. > > Signed-off-by: Flavio Leitner <fbl@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 > dev@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= > >
On 11/17/17, 3:01 PM, "Flavio Leitner" <fbl@sysclose.org> wrote:
On Fri, 17 Nov 2017 19:34:49 +0000
Darrell Ball <dball@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.
The comment means you can go both ways depending on the situation.
It is left up to judgement and there are significant advantages to keeping declarations near usage for clarity.
In all cases that I have been exposed to, most people favor keeping the benefits of keeping the declarations
near usage and I have accepted those comments.
That is what I agree with and will follow.
> 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));
"""
These are examples only; there is no requirement to follow these examples verbatim
and we have code that does both. There has also been comments on the alias
to leave this flexibility. I agree with this flexibility and like the operator at the end
for the reason, I mentioned. I will leave as is.
> 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-bounces@openvswitch.org on behalf of Flavio Leitner" <ovs-dev-bounces@openvswitch.org on behalf of fbl@sysclose.org> wrote:
>
> No functional change, just fixing coding style.
>
> Signed-off-by: Flavio Leitner <fbl@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
> dev@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
On Sat, 18 Nov 2017 00:41:27 +0000 Darrell Ball <dball@vmware.com> wrote: > On 11/17/17, 3:01 PM, "Flavio Leitner" <fbl@sysclose.org> wrote: > > On Fri, 17 Nov 2017 19:34:49 +0000 > Darrell Ball <dball@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. > > The comment means you can go both ways depending on the situation. > It is left up to judgement and there are significant advantages to keeping declarations near usage for clarity. > In all cases that I have been exposed to, most people favor keeping the benefits of keeping the declarations > near usage and I have accepted those comments. > That is what I agree with and will follow. You just said that it depends on the situation. For very long functions, which I think there are at least two, I agree with you. IMO, those functions are too long and most probably deserve to be broken down into smaller ones, if possible, but then they are not part of this patch. There is no need to do that in short functions. I am willing to update the patch removing the changes we agree it's not needed. > > > 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)); > """ > > > These are examples only; there is no requirement to follow these examples verbatim > and we have code that does both. There has also been comments on the alias > to leave this flexibility. I agree with this flexibility and like the operator at the end > for the reason, I mentioned. I will leave as is. So you're saying the examples in the coding style document are bad? :) Boss, I appreciate the time spent reviewing it and it is okay if you don't like parts of patch or even all of it. The assumption is that all ML members do care about the project and so everyone's opinion matter. We _must_ friendly discuss every patch and come down to an agreement that can be applying as-is, change portions of it or even drop the patch completely. I don't care about the final decision as long as we do the review properly, with technical arguments for the benefit of the _whole project_ and not just based on your personal preference. Just think the other way around, if I tell you that your follow up patch sucks and I am going to write it in my way, the only correct way. What kind of message does this send to others in the group? I am sure it's not a message that will make the project succeed in the long term. Therefore, it is absolutely _NOT_ okay to just say you're going to rewrite others' patches and post them. Again, I appreciate the time spend reviewing it and I am willing to send out a V2 removing the changes that now I am convinced we don't need them. But I expect the next review to be much less about your personal preferences, and a lot more about how this can bring the project one step forward. Does that sound fair to you? Thanks, fbl > > > > 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-bounces@openvswitch.org on behalf of Flavio Leitner" <ovs-dev-bounces@openvswitch.org on behalf of fbl@sysclose.org> wrote: > > > > No functional change, just fixing coding style. > > > > Signed-off-by: Flavio Leitner <fbl@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 > > dev@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 > > >
On Fri, Nov 17, 2017 at 03:51:34PM -0200, Flavio Leitner wrote: > No functional change, just fixing coding style. > > Signed-off-by: Flavio Leitner <fbl@sysclose.org> I see from the thread that there's some controversy here. I think that there are two aspects to it: technical and nontechnical. Please allow me address them separately, starting with the technical side. The code that later evolved to become OVS originated in 2007 at Nicira. At the time, support for C99 features was far from universal among the compilers that seemed potentially important. Justin and I wrote the initial coding style document to emphasize portability. Therefore, originally it forbid any mid-block declarations as well as other features with limited portability (e.g. C++ style // comments). Since then there have been two relevant changes. First, C99 is becoming more pervasive across the compiler world. Second, it's become clear that OVS really only needs to care about three particular compilers, which are GCC, Clang, and MSVC, and all of these compilers now support C99 (and an increasing fraction of C11). As these changes have occurred, we have been evolving the OVS coding style too. Sometimes, the evolution has happened first in the coding style document (and later migrated to coding practice), and other times, it has happened in the code itself (and later migrated to the coding style document). I think that this slow co-evolution is natural, since it's rare for someone to go through all the code at once and update the style to match changes to the coding style document. Sometimes, OVS has only half-embraced new coding style features enabled by compiler advancements. Mid-block declarations are an example. I've been writing C code since 1992 or so (and I hope I'm getting better at it!) but I've only been able to rely on portable support for mid-block declarations for a few years. For better or for worse, when in 2014 Jarno proposed that we allow mid-block declarations in OVS, the fossil in me, not used to seeing mid-block declarations in C, made me reluctant. You can see that, originally, Jarno proposed simply allowing mid-block declarations in his message https://mail.openvswitch.org/pipermail/ovs-dev/2014-May/284130.html. In a reply, Ethan and I were the one who pushed back, with my "I'm still really nervous about this one (I think it often uglifies code)..." and Ethan's "...The reason is that most of the code doesn't do these things, and there's a value in consistency." Since 2014, I feel that the balance has shifted. We have increased the amount of code that uses mid-block declarations. My increasing familiarity with the style has reduced my misgivings, and even caused me to embrace them: I really like the reduced potential for use of uninitialized data by being able, far more often, to declare and initialize a variable at the same time. Most importantly for me personally, this save time in code review, since I spend less time flicking my eyes and scrolling my window back and forth between declaration and use to reassure myself that initialization precedes use. I still see some potential for ugliness, but the practical virtues of mid-block declarations usually outweigh them. What I'm saying here is, "I was wrong." I honestly forgot we had anything in the coding style document that discouraged mid-block declarations. I like consistency between code and documentation but I believe that the right thing to do here is to update the coding style document, by removing anything that discourages mid-block declarations. Now I use them pervasively and I want to encourages others to do so too. That's my long, rambling defense of mid-block declarations. The other technical issue here is about whether && and || should be at the beginning or end of a line. The coding style document, intentionally, does not take an explicit position on this, and I did not realize that the examples in the document are biased toward putting them at the beginning of the line. OVS uses a mix of positions. I personally tend to put them at the beginning of a line because I have a long history of writing code in the GNU coding style, which mandates this positioning, but others feel exactly the opposite (for example I believe that kernel style favors end-of-line positioning). This is a debate that frankly I don't care much for and I'd prefer to leave it as a matter to individual taste. (This is also why the coding style document gives some freedom in the positioning of /* and */ in comment blocks: there is a range of viewpoints on this and I think the answer doesn't matter.) If necessary, I favor adding an explicit statement to the document saying that lines may be split before or after binary operators. (On the other hand I do prefer ? and : at the beginning of a line.) I believe that there is also a nontechnical aspect here. I think that might take a different email to address. Let me see what I can do.
On Fri, Nov 17, 2017 at 07:34:49PM +0000, Darrell Ball 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. > Most of this patch is related to this. > I’ll point out a few to provide examples, but I don’t like this – nack. This reads to me as more aggressive than necessary. The OVS coding style says to mix declarations and code within a block "judiciously" and to "keep declarations nicely grouped together in the beginning of a block if possible." I think that this now inaccurately describes current coding practice in OVS, as well as best practice for modern C, but it is still what the document says, so it is understandable that anyone reading the document would want to fix the code to match. I tend to take this patch as a kind of bug report against the coding style document. The right response to that might be to explain why the coding style document is wrong and the history behind it, and to thank the submitter (thank you, Flavio!). I suggest that we should update the coding style document to match our current practice (and perhaps to motivate current practice with rationale). > 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. This reads as somewhat aggressive too, especially following #1. I believe that Flavio interpreted the examples in the coding style document as normative. That is understandable, because the document does not say anything explicitly. When we wrote the document in 2007, I don't remember Justin and I even discussing whether lines should be split before or after binary operators, and we've never (as far as I know) interpreted those particular examples as normative in that aspect. Again, I would prefer to interpret this part of the patch as a bug report against the coding standards. Thank you, Flavio, for bringing this to our attention. I think that it would be a good idea to modify at least one of the examples to show a line break after a binary operator, and perhaps to add an explicit statement that there is no norm for positioning binary operators before or after a line break. (Maybe there is someone out there who wants to campaign for particular positioning, but I'll leave that to whoever that is.) > There is a missing space after “}” in one instance - thanks > 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”. Sometimes it is helpful to propose a competing patch, but usually that would follow something closer to coming to consensus, or sometimes after discussion makes it clear that the new proposer understands some issue better than the original poster or is otherwise better situated to help. In this case, it reads more as a kind of aggression. I hope that we can resolve this, technically and nontechnically, to everyone's satisfaction. Thanks Ben.
On 11/19/17, 10:25 AM, "Flavio Leitner" <fbl@sysclose.org> wrote:
On Sat, 18 Nov 2017 00:41:27 +0000
Darrell Ball <dball@vmware.com> wrote:
> On 11/17/17, 3:01 PM, "Flavio Leitner" <fbl@sysclose.org> wrote:
>
> On Fri, 17 Nov 2017 19:34:49 +0000
> Darrell Ball <dball@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.
>
> The comment means you can go both ways depending on the situation.
> It is left up to judgement and there are significant advantages to keeping declarations near usage for clarity.
> In all cases that I have been exposed to, most people favor keeping the benefits of keeping the declarations
> near usage and I have accepted those comments.
> That is what I agree with and will follow.
You just said that it depends on the situation.
For very long functions, which I think there are at least two, I agree
with you. IMO, those functions are too long and most probably deserve
to be broken down into smaller ones, if possible, but then they are not
part of this patch.
There is no need to do that in short functions. I am willing to update
the patch removing the changes we agree it's not needed.
>
> > 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));
> """
>
>
> These are examples only; there is no requirement to follow these examples verbatim
> and we have code that does both. There has also been comments on the alias
> to leave this flexibility. I agree with this flexibility and like the operator at the end
> for the reason, I mentioned. I will leave as is.
So you're saying the examples in the coding style document are bad? :)
Boss, I appreciate the time spent reviewing it and it is okay if you
don't like parts of patch or even all of it. The assumption is that all
ML members do care about the project and so everyone's opinion matter.
Just for the record:
The ALG patches were posted for 3 full months and had multiple very detailed reviews and reviewers.
I accepted almost all comments, even some that I felt were borderline.
Is 3 months not enough ?
The NAT patches were posted for 5 full months, had multiple reviewers, including yourself,
who acked them. I accepted almost all comments there as well.
Is 5 months not enough ?
The same holds for other features as well.
We _must_ friendly discuss every patch and come down to an agreement that
can be applying as-is, change portions of it or even drop the patch
completely. I don't care about the final decision as long as we do the
review properly, with technical arguments for the benefit of the _whole
project_ and not just based on your personal preference.
Just think the other way around, if I tell you that your follow up
patch sucks and I am going to write it in my way, the only correct way.
What kind of message does this send to others in the group? I am sure
it's not a message that will make the project succeed in the long term.
Therefore, it is absolutely _NOT_ okay to just say you're going to
rewrite others' patches and post them.
Again, I appreciate the time spend reviewing it and I am willing to
send out a V2 removing the changes that now I am convinced we don't
need them. But I expect the next review to be much less about your
personal preferences, and a lot more about how this can bring the
project one step forward. Does that sound fair to you?
Thanks,
fbl
>
>
> > 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-bounces@openvswitch.org on behalf of Flavio Leitner" <ovs-dev-bounces@openvswitch.org on behalf of fbl@sysclose.org> wrote:
> >
> > No functional change, just fixing coding style.
> >
> > Signed-off-by: Flavio Leitner <fbl@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
> > dev@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
>
>
>
--
Flavio
On 11/19/17, 11:18 AM, "Ben Pfaff" <blp@ovn.org> wrote:
On Fri, Nov 17, 2017 at 07:34:49PM +0000, Darrell Ball 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.
> Most of this patch is related to this.
> I’ll point out a few to provide examples, but I don’t like this – nack.
This reads to me as more aggressive than necessary. The OVS coding
style says to mix declarations and code within a block "judiciously" and
to "keep declarations nicely grouped together in the beginning of a
block if possible." I think that this now inaccurately describes
current coding practice in OVS, as well as best practice for modern C,
but it is still what the document says, so it is understandable that
anyone reading the document would want to fix the code to match.
I tend to take this patch as a kind of bug report against the coding
style document. The right response to that might be to explain why the
coding style document is wrong and the history behind it, and to thank
the submitter (thank you, Flavio!). I suggest that we should update the
coding style document to match our current practice (and perhaps to
motivate current practice with rationale).
> 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.
This reads as somewhat aggressive too, especially following #1.
Hmm, not intentionally then “and it's certainly not ad hominem” – I read that on another alias thread once
(it is easy to search for, btw) - “not ad hominem” is accurate here as well.
I believe that Flavio interpreted the examples in the coding style
document as normative. That is understandable, because the document
does not say anything explicitly. When we wrote the document in 2007, I
don't remember Justin and I even discussing whether lines should be
split before or after binary operators, and we've never (as far as I
know) interpreted those particular examples as normative in that aspect.
Again, I would prefer to interpret this part of the patch as a bug
report against the coding standards. Thank you, Flavio, for bringing
this to our attention. I think that it would be a good idea to modify
at least one of the examples to show a line break after a binary
operator, and perhaps to add an explicit statement that there is no norm
for positioning binary operators before or after a line break. (Maybe
there is someone out there who wants to campaign for particular
positioning, but I'll leave that to whoever that is.)
> There is a missing space after “}” in one instance - thanks
> 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”.
Sometimes it is helpful to propose a competing patch, but usually that
would follow something closer to coming to consensus, or sometimes after
discussion makes it clear that the new proposer understands some issue
better than the original poster or is otherwise better situated to
help. In this case, it reads more as a kind of aggression.
I was not referring to a competing patch, but rather a patch that fixes the code to match
the effective coding standard, as I understand it, in some missing cases, but also has a commit message
that lists each coding standard. The patch is different than Flavio’s but Flavio’s patch made me
look more while I was working on related patches.
https://patchwork.ozlabs.org/patch/839358/
I hope that we can resolve this, technically and nontechnically, to
everyone's satisfaction.
Thanks
Ben.
On Sun, 19 Nov 2017 11:18:25 -0800 Ben Pfaff <blp@ovn.org> wrote: [...] > I tend to take this patch as a kind of bug report against the coding > style document. The right response to that might be to explain why the > coding style document is wrong and the history behind it, and to thank > the submitter (thank you, Flavio!). I suggest that we should update the > coding style document to match our current practice (and perhaps to > motivate current practice with rationale). Sounds good to me. Please drop my patch then.
Hi Ben, On Sun, 19 Nov 2017 10:58:11 -0800 Ben Pfaff <blp@ovn.org> wrote: > On Fri, Nov 17, 2017 at 03:51:34PM -0200, Flavio Leitner wrote: > > No functional change, just fixing coding style. > > > > Signed-off-by: Flavio Leitner <fbl@sysclose.org> > > I see from the thread that there's some controversy here. I think that > there are two aspects to it: technical and nontechnical. Please allow > me address them separately, starting with the technical side. > > The code that later evolved to become OVS originated in 2007 at Nicira. > At the time, support for C99 features was far from universal among the > compilers that seemed potentially important. Justin and I wrote the > initial coding style document to emphasize portability. Therefore, > originally it forbid any mid-block declarations as well as other > features with limited portability (e.g. C++ style // comments). > > Since then there have been two relevant changes. First, C99 is becoming > more pervasive across the compiler world. Second, it's become clear > that OVS really only needs to care about three particular compilers, > which are GCC, Clang, and MSVC, and all of these compilers now support > C99 (and an increasing fraction of C11). > > As these changes have occurred, we have been evolving the OVS coding > style too. Sometimes, the evolution has happened first in the coding > style document (and later migrated to coding practice), and other times, > it has happened in the code itself (and later migrated to the coding > style document). I think that this slow co-evolution is natural, since > it's rare for someone to go through all the code at once and update the > style to match changes to the coding style document. > > Sometimes, OVS has only half-embraced new coding style features enabled > by compiler advancements. Mid-block declarations are an example. I've > been writing C code since 1992 or so (and I hope I'm getting better at > it!) but I've only been able to rely on portable support for mid-block > declarations for a few years. For better or for worse, when in 2014 > Jarno proposed that we allow mid-block declarations in OVS, the fossil > in me, not used to seeing mid-block declarations in C, made me > reluctant. You can see that, originally, Jarno proposed simply allowing > mid-block declarations in his message > https://mail.openvswitch.org/pipermail/ovs-dev/2014-May/284130.html. In > a reply, Ethan and I were the one who pushed back, with my "I'm still > really nervous about this one (I think it often uglifies code)..." and > Ethan's "...The reason is that most of the code doesn't do these things, > and there's a value in consistency." > > Since 2014, I feel that the balance has shifted. We have increased the > amount of code that uses mid-block declarations. My increasing > familiarity with the style has reduced my misgivings, and even caused me > to embrace them: I really like the reduced potential for use of > uninitialized data by being able, far more often, to declare and > initialize a variable at the same time. Most importantly for me > personally, this save time in code review, since I spend less time > flicking my eyes and scrolling my window back and forth between > declaration and use to reassure myself that initialization precedes use. > I still see some potential for ugliness, but the practical virtues of > mid-block declarations usually outweigh them. > > What I'm saying here is, "I was wrong." I honestly forgot we had > anything in the coding style document that discouraged mid-block > declarations. I like consistency between code and documentation but I > believe that the right thing to do here is to update the coding style > document, by removing anything that discourages mid-block declarations. > Now I use them pervasively and I want to encourages others to do so too. > > That's my long, rambling defense of mid-block declarations. The other > technical issue here is about whether && and || should be at the > beginning or end of a line. The coding style document, intentionally, > does not take an explicit position on this, and I did not realize that > the examples in the document are biased toward putting them at the > beginning of the line. OVS uses a mix of positions. I personally tend > to put them at the beginning of a line because I have a long history of > writing code in the GNU coding style, which mandates this positioning, > but others feel exactly the opposite (for example I believe that kernel > style favors end-of-line positioning). This is a debate that frankly I > don't care much for and I'd prefer to leave it as a matter to individual > taste. (This is also why the coding style document gives some freedom > in the positioning of /* and */ in comment blocks: there is a range of > viewpoints on this and I think the answer doesn't matter.) If > necessary, I favor adding an explicit statement to the document saying > that lines may be split before or after binary operators. (On the other > hand I do prefer ? and : at the beginning of a line.) It all makes sense except for the freedom in the coding style. Too much freedom means no standard. It is natural that the code evolves and maybe new things will impact on the current coding style guidelines. I agree we don't need to rewrite OVS every time that happens, but as it happened in this case, one could update the style being used in a file at some point, so that the coding style document rules the major portion of the code base. I came from the kernel and yet the patch uses the other style, which I figured was the preferable one. Even with that fossil in me a bit reluctant to adopt mid-block declarations, I honestly don't care as long as I can see a pattern ruling a good portion of the code. Yes, I realize the proposed patch (please drop it) has changes going in the other direction because it never occurred to me that we could update the coding style with regards to where to break lines. How to decide which one is preferable? Maybe take a snapshot of OVS today and see what is mostly used and take from there? Thanks,
On Mon, Nov 20, 2017 at 11:42:43AM -0200, Flavio Leitner wrote: > On Sun, 19 Nov 2017 10:58:11 -0800 Ben Pfaff <blp@ovn.org> wrote: > > The other technical issue here is about whether && and || should be > > at the beginning or end of a line. The coding style document, > > intentionally, does not take an explicit position on this, and I did > > not realize that the examples in the document are biased toward > > putting them at the beginning of the line. OVS uses a mix of > > positions. I personally tend to put them at the beginning of a line > > because I have a long history of writing code in the GNU coding > > style, which mandates this positioning, but others feel exactly the > > opposite (for example I believe that kernel style favors end-of-line > > positioning). This is a debate that frankly I don't care much for > > and I'd prefer to leave it as a matter to individual taste. (This > > is also why the coding style document gives some freedom in the > > positioning of /* and */ in comment blocks: there is a range of > > viewpoints on this and I think the answer doesn't matter.) If > > necessary, I favor adding an explicit statement to the document > > saying that lines may be split before or after binary operators. > > (On the other hand I do prefer ? and : at the beginning of a line.) > > It all makes sense except for the freedom in the coding style. Too much > freedom means no standard. > > It is natural that the code evolves and maybe new things will impact on > the current coding style guidelines. I agree we don't need to rewrite > OVS every time that happens, but as it happened in this case, one could > update the style being used in a file at some point, so that the coding > style document rules the major portion of the code base. > > I came from the kernel and yet the patch uses the other style, which > I figured was the preferable one. Even with that fossil in me a bit > reluctant to adopt mid-block declarations, I honestly don't care as > long as I can see a pattern ruling a good portion of the code. > > Yes, I realize the proposed patch (please drop it) has changes going > in the other direction because it never occurred to me that we could > update the coding style with regards to where to break lines. > > How to decide which one is preferable? Maybe take a snapshot of OVS > today and see what is mostly used and take from there? One reason that the coding style takes no position is that I can see an argument for each possibility. Positioning a binary operator before a line break gives the operands a more consistent look, e.g.: a = (b | c | d); but positioning after a line break better draws the eye to the binary operator, which makes it more obvious what's happening, e.g.: if (a && b && c) These days, I personally choose a style depending on which of those factors I consider more important in an individual situation. What do you think?
On Mon, 20 Nov 2017 08:48:10 -0800 Ben Pfaff <blp@ovn.org> wrote: > On Mon, Nov 20, 2017 at 11:42:43AM -0200, Flavio Leitner wrote: > > On Sun, 19 Nov 2017 10:58:11 -0800 Ben Pfaff <blp@ovn.org> wrote: > > > The other technical issue here is about whether && and || should be > > > at the beginning or end of a line. The coding style document, > > > intentionally, does not take an explicit position on this, and I did > > > not realize that the examples in the document are biased toward > > > putting them at the beginning of the line. OVS uses a mix of > > > positions. I personally tend to put them at the beginning of a line > > > because I have a long history of writing code in the GNU coding > > > style, which mandates this positioning, but others feel exactly the > > > opposite (for example I believe that kernel style favors end-of-line > > > positioning). This is a debate that frankly I don't care much for > > > and I'd prefer to leave it as a matter to individual taste. (This > > > is also why the coding style document gives some freedom in the > > > positioning of /* and */ in comment blocks: there is a range of > > > viewpoints on this and I think the answer doesn't matter.) If > > > necessary, I favor adding an explicit statement to the document > > > saying that lines may be split before or after binary operators. > > > (On the other hand I do prefer ? and : at the beginning of a line.) > > > > It all makes sense except for the freedom in the coding style. Too much > > freedom means no standard. > > > > It is natural that the code evolves and maybe new things will impact on > > the current coding style guidelines. I agree we don't need to rewrite > > OVS every time that happens, but as it happened in this case, one could > > update the style being used in a file at some point, so that the coding > > style document rules the major portion of the code base. > > > > I came from the kernel and yet the patch uses the other style, which > > I figured was the preferable one. Even with that fossil in me a bit > > reluctant to adopt mid-block declarations, I honestly don't care as > > long as I can see a pattern ruling a good portion of the code. > > > > Yes, I realize the proposed patch (please drop it) has changes going > > in the other direction because it never occurred to me that we could > > update the coding style with regards to where to break lines. > > > > How to decide which one is preferable? Maybe take a snapshot of OVS > > today and see what is mostly used and take from there? > > One reason that the coding style takes no position is that I can see an > argument for each possibility. Positioning a binary operator before a > line break gives the operands a more consistent look, e.g.: > > a = (b | > c | > d); > > but positioning after a line break better draws the eye to the binary > operator, which makes it more obvious what's happening, e.g.: > > if (a > && b > && c) > > These days, I personally choose a style depending on which of those > factors I consider more important in an individual situation. > > What do you think? Both have pros and cons, so I have no definitive answer to each one is better. But I find more difficult to read and code when we have mixed styles, don't you? Interesting that the kernel style favors the end-of-line positioning, so if we choose that, OVS would be one step closer to the kernel DP coding style. A quick stats shows OVS preference is more towards the other way though. $ git grep '^[ \t]\+[&|\|]\{2\}' *.c | wc -l 1346 (60%) $ git grep '[&|\|]\{2\}[ ]*$' *.c | wc -l 865 (40%) Perhaps it all boils down to the lack of documentation in the coding style saying what you just said above. Because I was trying to follow the style in each file, so every time I switch to another file, I have to stop and find out which style is being used again and again. That is annoying. Therefore, your criteria helps a lot, because now I know it's not per project and neither per file, it's about which one I consider more suitable in each individual situation. Thanks,
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; 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))); } @@ -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)); } @@ -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; } @@ -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; + } + 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; + 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, 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]); } - 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); } - /* 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); + /* 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); + /* 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); + 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; - 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)) { 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); 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); } 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; } } 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))) { 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; } @@ -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; 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)) {
No functional change, just fixing coding style. Signed-off-by: Flavio Leitner <fbl@sysclose.org> --- lib/conntrack.c | 335 +++++++++++++++++++++++++++++++------------------------- 1 file changed, 188 insertions(+), 147 deletions(-)