[ovs-dev] [PATCH] conntrack: coding style fixes.
Darrell Ball
dball at vmware.com
Fri Nov 17 19:34:49 UTC 2017
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 at openvswitch.org on behalf of Flavio Leitner" <ovs-dev-bounces at openvswitch.org on behalf of fbl at sysclose.org> wrote:
No functional change, just fixing coding style.
Signed-off-by: Flavio Leitner <fbl at 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 at 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=
More information about the dev
mailing list