Re: [ovs-dev] [patch v1 1/4] conntrack: Some formatting improvements.

[Darrell Ball]

On 12/28/17, 5:28 AM, "ovs-dev-boun...@openvswitch.org on behalf of Flavio 
Leitner"  wrote:


Hi Darrell,


I am coming back from vacations, sorry the delay.

The patch doesn't apply anymore, but I reviewed anyways.

Thank you Flavio !

This patch was superceded and now outdated, but let me fold your comments for 
V3,
since there is now a request to combine some weakly related patches including 
this one into a series.

Cheers Darrell


On Sun, Nov 19, 2017 at 01:02:19PM -0800, Darrell Ball wrote:
> Fix up some instances where variable declarations were not close
> enough to their use, as these were missed before.  This is the
> preferred art in OVS code and flagged heavily in code reviews.
> This is highly desirable due to code clarity reasons.
> 
> There are also some cases where newlines were not needed by prior art
> and some cases where they were needed but missed.
> 
> There was one case where there was a missing space after "}".
> 
> There were a few cases where for loop index decalrations could be
  

typo.

Thanks

 
> folded into the loop.
> 
> Signed-off-by: Darrell Ball 
> ---
>  lib/conntrack.c | 190 
+---
>  1 file changed, 85 insertions(+), 105 deletions(-)
> 
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index f5a3aa9..b8d0e77 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -295,10 +295,10 @@ conntrack_init(struct conntrack *ct)
>  
>  for (i = 0; i < CONNTRACK_BUCKETS; i++) {
>  struct conntrack_bucket *ctb = >buckets[i];
> -
>  ct_lock_init(>lock);
>  ct_lock_lock(>lock);
>  hmap_init(>connections);
> +
>  for (j = 0; j < ARRAY_SIZE(ctb->exp_lists); j++) {
>  ovs_list_init(>exp_lists[j]);
>  }
> @@ -319,17 +319,16 @@ conntrack_init(struct conntrack *ct)
>  void
>  conntrack_destroy(struct conntrack *ct)
>  {
> -unsigned i;
> -
>  latch_set(>clean_thread_exit);
>  pthread_join(ct->clean_thread, NULL);
>  latch_destroy(>clean_thread_exit);
> -for (i = 0; i < CONNTRACK_BUCKETS; i++) {
> -struct conntrack_bucket *ctb = >buckets[i];
> -struct conn *conn;
>  
> +for (unsigned i = 0; i < CONNTRACK_BUCKETS; i++) {
> +struct conntrack_bucket *ctb = >buckets[i];
>  ovs_mutex_destroy(>cleanup_mutex);
>  ct_lock_lock(>lock);
> +
> +struct conn *conn;
>  HMAP_FOR_EACH_POP (conn, node, >connections) {
>  if (conn->conn_type == CT_CONN_TYPE_DEFAULT) {
>  atomic_count_dec(>n_conn);
> @@ -390,7 +389,6 @@ write_ct_md(struct dp_packet *pkt, uint16_t zone, 
const struct conn *conn,

Add empty line here?
>  pkt->md.ct_orig_tuple_ipv6 = false;

and here?

Thanks, for both

>  if (key) {
>  if (key->dl_type == htons(ETH_TYPE_IP)) {
> -
>  pkt->md.ct_orig_tuple.ipv4 = (struct ovs_key_ct_tuple_ipv4) {
>  key->src.addr.ipv4_aligned,
>  key->dst.addr.ipv4_aligned,
> @@ -447,14 +445,13 @@ is_ftp_ctl(const struct dp_packet *pkt)
>  return (ip_proto == IPPROTO_TCP &&
>  (th->tcp_src == htons(CT_IPPORT_FTP) ||
>   th->tcp_dst == htons(CT_IPPORT_FTP)));
> -
>  }
>  
>  static bool
>  is_tftp_ctl(const struct dp_packet *pkt)
>  {
> -uint8_t ip_proto = get_ip_proto(pkt);
> -struct udp_header *uh = dp_packet_l4(pkt);
> + uint8_t ip_proto = get_ip_proto(pkt);
> + struct udp_header *uh = dp_packet_l4(pkt);
>  
>  /* CT_IPPORT_TFTP is used because IPPORT_TFTP in not defined in OSX,
>   * at least in in.h. Since this value will never change, remove
> @@ -462,7 +459,6 @@ is_tftp_ctl(const struct dp_packet *pkt)
>  #define CT_IPPORT_TFTP 69
>  return (ip_proto == IPPROTO_UDP &&
>  uh->udp_dst == htons(CT_IPPORT_TFTP));
> -
>  }
>  
>  static void
> @@ -599,7 +595,6 @@ reverse_nat_packet(struct dp_packet *pkt, const 
struct conn *conn)
>  struct ip_header *inner_l3 = (struct ip_header *) (icmp + 1);
>  extract_l3_ipv4(_key, inner_l3, tail - ((char *)inner_l3) 
- pad,
>  _l4, false);
> -
>  pkt->l3_ofs += (char *) inner_l3 - (char *) nh;
>  pkt->l4_ofs += inner_l4 - (char *) icmp;
>  
> @@ -610,6 +605,7 @@ reverse_nat_packet(struct dp_packet *pkt, const 
struct conn *conn)
>  

Re: [ovs-dev] [patch v1 1/4] conntrack: Some formatting improvements.

[Flavio Leitner]

Hi Darrell,


I am coming back from vacations, sorry the delay.

The patch doesn't apply anymore, but I reviewed anyways.

On Sun, Nov 19, 2017 at 01:02:19PM -0800, Darrell Ball wrote:
> Fix up some instances where variable declarations were not close
> enough to their use, as these were missed before.  This is the
> preferred art in OVS code and flagged heavily in code reviews.
> This is highly desirable due to code clarity reasons.
> 
> There are also some cases where newlines were not needed by prior art
> and some cases where they were needed but missed.
> 
> There was one case where there was a missing space after "}".
> 
> There were a few cases where for loop index decalrations could be
  

typo.

 
> folded into the loop.
> 
> Signed-off-by: Darrell Ball 
> ---
>  lib/conntrack.c | 190 
> +---
>  1 file changed, 85 insertions(+), 105 deletions(-)
> 
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index f5a3aa9..b8d0e77 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -295,10 +295,10 @@ conntrack_init(struct conntrack *ct)
>  
>  for (i = 0; i < CONNTRACK_BUCKETS; i++) {
>  struct conntrack_bucket *ctb = >buckets[i];
> -
>  ct_lock_init(>lock);
>  ct_lock_lock(>lock);
>  hmap_init(>connections);
> +
>  for (j = 0; j < ARRAY_SIZE(ctb->exp_lists); j++) {
>  ovs_list_init(>exp_lists[j]);
>  }
> @@ -319,17 +319,16 @@ conntrack_init(struct conntrack *ct)
>  void
>  conntrack_destroy(struct conntrack *ct)
>  {
> -unsigned i;
> -
>  latch_set(>clean_thread_exit);
>  pthread_join(ct->clean_thread, NULL);
>  latch_destroy(>clean_thread_exit);
> -for (i = 0; i < CONNTRACK_BUCKETS; i++) {
> -struct conntrack_bucket *ctb = >buckets[i];
> -struct conn *conn;
>  
> +for (unsigned i = 0; i < CONNTRACK_BUCKETS; i++) {
> +struct conntrack_bucket *ctb = >buckets[i];
>  ovs_mutex_destroy(>cleanup_mutex);
>  ct_lock_lock(>lock);
> +
> +struct conn *conn;
>  HMAP_FOR_EACH_POP (conn, node, >connections) {
>  if (conn->conn_type == CT_CONN_TYPE_DEFAULT) {
>  atomic_count_dec(>n_conn);
> @@ -390,7 +389,6 @@ write_ct_md(struct dp_packet *pkt, uint16_t zone, const 
> struct conn *conn,

Add empty line here?
>  pkt->md.ct_orig_tuple_ipv6 = false;

and here?

>  if (key) {
>  if (key->dl_type == htons(ETH_TYPE_IP)) {
> -
>  pkt->md.ct_orig_tuple.ipv4 = (struct ovs_key_ct_tuple_ipv4) {
>  key->src.addr.ipv4_aligned,
>  key->dst.addr.ipv4_aligned,
> @@ -447,14 +445,13 @@ is_ftp_ctl(const struct dp_packet *pkt)
>  return (ip_proto == IPPROTO_TCP &&
>  (th->tcp_src == htons(CT_IPPORT_FTP) ||
>   th->tcp_dst == htons(CT_IPPORT_FTP)));
> -
>  }
>  
>  static bool
>  is_tftp_ctl(const struct dp_packet *pkt)
>  {
> -uint8_t ip_proto = get_ip_proto(pkt);
> -struct udp_header *uh = dp_packet_l4(pkt);
> + uint8_t ip_proto = get_ip_proto(pkt);
> + struct udp_header *uh = dp_packet_l4(pkt);
>  
>  /* CT_IPPORT_TFTP is used because IPPORT_TFTP in not defined in OSX,
>   * at least in in.h. Since this value will never change, remove
> @@ -462,7 +459,6 @@ is_tftp_ctl(const struct dp_packet *pkt)
>  #define CT_IPPORT_TFTP 69
>  return (ip_proto == IPPROTO_UDP &&
>  uh->udp_dst == htons(CT_IPPORT_TFTP));
> -
>  }
>  
>  static void
> @@ -599,7 +595,6 @@ reverse_nat_packet(struct dp_packet *pkt, const struct 
> conn *conn)
>  struct ip_header *inner_l3 = (struct ip_header *) (icmp + 1);
>  extract_l3_ipv4(_key, inner_l3, tail - ((char *)inner_l3) - 
> pad,
>  _l4, false);
> -
>  pkt->l3_ofs += (char *) inner_l3 - (char *) nh;
>  pkt->l4_ofs += inner_l4 - (char *) icmp;
>  
> @@ -610,6 +605,7 @@ reverse_nat_packet(struct dp_packet *pkt, const struct 
> conn *conn)
>  packet_set_ipv4_addr(pkt, _l3->ip_dst,
>   conn->key.dst.addr.ipv4_aligned);
>  }
> +
>  reverse_pat_packet(pkt, conn);
>  icmp->icmp_csum = 0;
>  icmp->icmp_csum = csum(icmp, tail - (char *) icmp - pad);
> @@ -712,6 +708,7 @@ conn_seq_skew_set(struct conntrack *ct, const struct 
> conn_key *key,
>  unsigned bucket = hash_to_bucket(hash);
>  ct_lock_lock(>buckets[bucket].lock);
>  struct conn *conn = conn_lookup(ct, key, now);
> +
>  if (conn && seq_skew) {
>  conn->seq_skew = seq_skew;
>  conn->seq_skew_dir = seq_skew_dir;
> @@ -724,7 +721,6 @@ nat_clean(struct conntrack *ct, struct conn *conn,
>struct conntrack_bucket *ctb)
>  OVS_REQUIRES(ctb->lock)
>  {
> -long long now = time_msec();
>  ct_rwlock_wrlock(>resources_lock);
>  

[ovs-dev] [patch v1 1/4] conntrack: Some formatting improvements.

[Darrell Ball]

Fix up some instances where variable declarations were not close
enough to their use, as these were missed before.  This is the
preferred art in OVS code and flagged heavily in code reviews.
This is highly desirable due to code clarity reasons.

There are also some cases where newlines were not needed by prior art
and some cases where they were needed but missed.

There was one case where there was a missing space after "}".

There were a few cases where for loop index decalrations could be
folded into the loop.

Signed-off-by: Darrell Ball 
---
 lib/conntrack.c | 190 +---
 1 file changed, 85 insertions(+), 105 deletions(-)

diff --git a/lib/conntrack.c b/lib/conntrack.c
index f5a3aa9..b8d0e77 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -295,10 +295,10 @@ conntrack_init(struct conntrack *ct)
 
 for (i = 0; i < CONNTRACK_BUCKETS; i++) {
 struct conntrack_bucket *ctb = >buckets[i];
-
 ct_lock_init(>lock);
 ct_lock_lock(>lock);
 hmap_init(>connections);
+
 for (j = 0; j < ARRAY_SIZE(ctb->exp_lists); j++) {
 ovs_list_init(>exp_lists[j]);
 }
@@ -319,17 +319,16 @@ conntrack_init(struct conntrack *ct)
 void
 conntrack_destroy(struct conntrack *ct)
 {
-unsigned i;
-
 latch_set(>clean_thread_exit);
 pthread_join(ct->clean_thread, NULL);
 latch_destroy(>clean_thread_exit);
-for (i = 0; i < CONNTRACK_BUCKETS; i++) {
-struct conntrack_bucket *ctb = >buckets[i];
-struct conn *conn;
 
+for (unsigned i = 0; i < CONNTRACK_BUCKETS; i++) {
+struct conntrack_bucket *ctb = >buckets[i];
 ovs_mutex_destroy(>cleanup_mutex);
 ct_lock_lock(>lock);
+
+struct conn *conn;
 HMAP_FOR_EACH_POP (conn, node, >connections) {
 if (conn->conn_type == CT_CONN_TYPE_DEFAULT) {
 atomic_count_dec(>n_conn);
@@ -390,7 +389,6 @@ write_ct_md(struct dp_packet *pkt, uint16_t zone, const 
struct conn *conn,
 pkt->md.ct_orig_tuple_ipv6 = false;
 if (key) {
 if (key->dl_type == htons(ETH_TYPE_IP)) {
-
 pkt->md.ct_orig_tuple.ipv4 = (struct ovs_key_ct_tuple_ipv4) {
 key->src.addr.ipv4_aligned,
 key->dst.addr.ipv4_aligned,
@@ -447,14 +445,13 @@ is_ftp_ctl(const struct dp_packet *pkt)
 return (ip_proto == IPPROTO_TCP &&
 (th->tcp_src == htons(CT_IPPORT_FTP) ||
  th->tcp_dst == htons(CT_IPPORT_FTP)));
-
 }
 
 static bool
 is_tftp_ctl(const struct dp_packet *pkt)
 {
-uint8_t ip_proto = get_ip_proto(pkt);
-struct udp_header *uh = dp_packet_l4(pkt);
+ uint8_t ip_proto = get_ip_proto(pkt);
+ struct udp_header *uh = dp_packet_l4(pkt);
 
 /* CT_IPPORT_TFTP is used because IPPORT_TFTP in not defined in OSX,
  * at least in in.h. Since this value will never change, remove
@@ -462,7 +459,6 @@ is_tftp_ctl(const struct dp_packet *pkt)
 #define CT_IPPORT_TFTP 69
 return (ip_proto == IPPROTO_UDP &&
 uh->udp_dst == htons(CT_IPPORT_TFTP));
-
 }
 
 static void
@@ -599,7 +595,6 @@ reverse_nat_packet(struct dp_packet *pkt, const struct conn 
*conn)
 struct ip_header *inner_l3 = (struct ip_header *) (icmp + 1);
 extract_l3_ipv4(_key, inner_l3, tail - ((char *)inner_l3) - pad,
 _l4, false);
-
 pkt->l3_ofs += (char *) inner_l3 - (char *) nh;
 pkt->l4_ofs += inner_l4 - (char *) icmp;
 
@@ -610,6 +605,7 @@ reverse_nat_packet(struct dp_packet *pkt, const struct conn 
*conn)
 packet_set_ipv4_addr(pkt, _l3->ip_dst,
  conn->key.dst.addr.ipv4_aligned);
 }
+
 reverse_pat_packet(pkt, conn);
 icmp->icmp_csum = 0;
 icmp->icmp_csum = csum(icmp, tail - (char *) icmp - pad);
@@ -712,6 +708,7 @@ conn_seq_skew_set(struct conntrack *ct, const struct 
conn_key *key,
 unsigned bucket = hash_to_bucket(hash);
 ct_lock_lock(>buckets[bucket].lock);
 struct conn *conn = conn_lookup(ct, key, now);
+
 if (conn && seq_skew) {
 conn->seq_skew = seq_skew;
 conn->seq_skew_dir = seq_skew_dir;
@@ -724,7 +721,6 @@ nat_clean(struct conntrack *ct, struct conn *conn,
   struct conntrack_bucket *ctb)
 OVS_REQUIRES(ctb->lock)
 {
-long long now = time_msec();
 ct_rwlock_wrlock(>resources_lock);
 nat_conn_keys_remove(>nat_conn_keys, >rev_key, ct->hash_basis);
 ct_rwlock_unlock(>resources_lock);
@@ -736,8 +732,8 @@ nat_clean(struct conntrack *ct, struct conn *conn,
 ct_lock_lock(>buckets[bucket_rev_conn].lock);
 ct_rwlock_wrlock(>resources_lock);
 
+long long now = time_msec();
 struct conn *rev_conn = conn_lookup(ct, >rev_key, now);
-
 struct nat_conn_key_node *nat_conn_key_node =
 nat_conn_keys_lookup(>nat_conn_keys, >rev_key,
  ct->hash_basis);
@@ -751,8 +747,8 @@ nat_clean(struct conntrack *ct,