On 11/17/17, 3:01 PM, "Flavio Leitner" <[email protected]> wrote:
On Fri, 17 Nov 2017 19:34:49 +0000
Darrell Ball <[email protected]> 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, "[email protected] on behalf of
Flavio Leitner" <[email protected] on behalf of
[email protected]> wrote:
>
> No functional change, just fixing coding style.
>
> Signed-off-by: Flavio Leitner <[email protected]>
> ---
> 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
> [email protected]
>
https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=Zhm7pwk8KMdO8CG7F7pKvJSPxz41jlXPmXL5aVVtreA&s=Y1k0-poJEUjrDsbCziMxKMJHZRIpYxk12oXjvtrgwlQ&e=
>
>
--
Flavio
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
