[ovs-dev] [PATCH] conntrack: coding style fixes.

Flavio Leitner fbl at sysclose.org
Fri Nov 17 23:01:21 UTC 2017


On Fri, 17 Nov 2017 19:34:49 +0000
Darrell Ball <dball at 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 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

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 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=
>     
> 



-- 
Flavio



More information about the dev mailing list