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

Flavio Leitner fbl at sysclose.org
Sun Nov 19 18:25:17 UTC 2017


On Sat, 18 Nov 2017 00:41:27 +0000
Darrell Ball <dball at vmware.com> wrote:

> On 11/17/17, 3:01 PM, "Flavio Leitner" <fbl at sysclose.org> wrote:
> 
>     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.
> 
> The comment means you can go both ways depending on the situation.
> It is left up to judgement and there are significant advantages to keeping declarations near usage for clarity.
> In all cases that I have been exposed to, most people favor keeping the benefits of keeping the declarations
> near usage and I have accepted those comments.
> That is what I agree with and will follow.

You just said that it depends on the situation.

For very long functions, which I think there are at least two, I agree
with you. IMO, those functions are too long and most probably deserve
to be broken down into smaller ones, if possible, but then they are not
part of this patch.

There is no need to do that in short functions. I am willing to update
the patch removing the changes we agree it's not needed.


>     
>     > Most of this patch is related to this.
>     > I’ll point out a few to provide examples, but I don’t like this – nack.
>     > 
>     > 2/In many instances where this patch moves “&&” at beginning of next line rather than at end of line
>     > There is no coding style for this and different OVS code uses both styles.
>     > I prefer to have the operator at the end of line as it makes it clear there is a continuation.  
>     
>     It's your preference again.  Note the examples from 
>     Documentation/internals/contributing/coding-style.rst
>     
>     """
>     Do not parenthesize the operands of ``&&`` and ``||`` unless operator
>     precedence makes it necessary, or unless the operands are themselves
>     expressions that use ``&&`` and ``||``. Thus:
>     
>     ::
>     
>         if (!isdigit((unsigned char)s[0])
>             || !isdigit((unsigned char)s[1])
>             || !isdigit((unsigned char)s[2])) {
>             printf("string %s does not start with 3-digit code\n", s);
>         }
>     
>     but
>     
>     ::
>     
>         if (rule && (!best || rule->priority > best->priority)) {
>             best = rule;
>         }
>     
>     Do parenthesize a subexpression that must be split across more than one line,
>     e.g.:
>     
>     ::
>     
>         *idxp = ((l1_idx << PORT_ARRAY_L1_SHIFT)
>                  | (l2_idx << PORT_ARRAY_L2_SHIFT)
>                  | (l3_idx << PORT_ARRAY_L3_SHIFT));
>     """
> 
> 
> These are examples only; there is no requirement to follow these examples verbatim
> and we have code that does both. There has also been comments on the alias
> to leave this flexibility. I agree with this flexibility and like the operator at the end
> for the reason, I mentioned. I will leave as is.

So you're saying the examples in the coding style document are bad? :)

Boss, I appreciate the time spent reviewing it and it is okay if you
don't like parts of patch or even all of it. The assumption is that all
ML members do care about the project and so everyone's opinion matter.

We _must_ friendly discuss every patch and come down to an agreement that
can be applying as-is, change portions of it or even drop the patch
completely. I don't care about the final decision as long as we do the
review properly, with technical arguments for the benefit of the _whole
project_ and not just based on your personal preference.

Just think the other way around, if I tell you that your follow up
patch sucks and I am going to write it in my way, the only correct way.
What kind of message does this send to others in the group?  I am sure
it's not a message that will make the project succeed in the long term.
Therefore, it is absolutely _NOT_ okay to just say you're going to
rewrite others' patches and post them.

Again, I appreciate the time spend reviewing it and I am willing to
send out a V2 removing the changes that now I am convinced we don't
need them. But I expect the next review to be much less about your
personal preferences, and a lot more about how this can bring the
project one step forward. Does that sound fair to you?

Thanks,
fbl

>     
>     
>     > There is a missing space after “}” in one instance - thanks  
>     
>     Could you point it? I will gladly spin a v2.
>     
>     > There are also use of full 79 line lengths in a few places, which looked ok, but I did not check carefully.
>     > There is some missed extra newlines after declarations, which generally looks ok; I’ll check more however.
>     > I also see some extra newlines removed which looked ok, but I’ll check more.
>     > 
>     > I’ll submit my own patch since I don’t agree with “1” and “2”.
>     > 
>     > 
>     > On 11/17/17, 9:52 AM, "ovs-dev-bounces 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
>     
>     
> 



-- 
Flavio



More information about the dev mailing list