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

[Ben Pfaff]

On Fri, Nov 17, 2017 at 03:51:34PM -0200, Flavio Leitner wrote:
> No functional change, just fixing coding style.
> 
> Signed-off-by: Flavio Leitner 

I see from the thread that there's some controversy here.  I think that
there are two aspects to it: technical and nontechnical.  Please allow
me address them separately, starting with the technical side.

The code that later evolved to become OVS originated in 2007 at Nicira.
At the time, support for C99 features was far from universal among the
compilers that seemed potentially important.  Justin and I wrote the
initial coding style document to emphasize portability.  Therefore,
originally it forbid any mid-block declarations as well as other
features with limited portability (e.g. C++ style // comments).

Since then there have been two relevant changes.  First, C99 is becoming
more pervasive across the compiler world.  Second, it's become clear
that OVS really only needs to care about three particular compilers,
which are GCC, Clang, and MSVC, and all of these compilers now support
C99 (and an increasing fraction of C11).

As these changes have occurred, we have been evolving the OVS coding
style too.  Sometimes, the evolution has happened first in the coding
style document (and later migrated to coding practice), and other times,
it has happened in the code itself (and later migrated to the coding
style document).  I think that this slow co-evolution is natural, since
it's rare for someone to go through all the code at once and update the
style to match changes to the coding style document.

Sometimes, OVS has only half-embraced new coding style features enabled
by compiler advancements.  Mid-block declarations are an example.  I've
been writing C code since 1992 or so (and I hope I'm getting better at
it!) but I've only been able to rely on portable support for mid-block
declarations for a few years.  For better or for worse, when in 2014
Jarno proposed that we allow mid-block declarations in OVS, the fossil
in me, not used to seeing mid-block declarations in C, made me
reluctant.  You can see that, originally, Jarno proposed simply allowing
mid-block declarations in his message
https://mail.openvswitch.org/pipermail/ovs-dev/2014-May/284130.html.  In
a reply, Ethan and I were the one who pushed back, with my "I'm still
really nervous about this one (I think it often uglifies code)..." and
Ethan's "...The reason is that most of the code doesn't do these things,
and there's a value in consistency."

Since 2014, I feel that the balance has shifted.  We have increased the
amount of code that uses mid-block declarations.  My increasing
familiarity with the style has reduced my misgivings, and even caused me
to embrace them: I really like the reduced potential for use of
uninitialized data by being able, far more often, to declare and
initialize a variable at the same time.  Most importantly for me
personally, this save time in code review, since I spend less time
flicking my eyes and scrolling my window back and forth between
declaration and use to reassure myself that initialization precedes use.
I still see some potential for ugliness, but the practical virtues of
mid-block declarations usually outweigh them.

What I'm saying here is, "I was wrong."  I honestly forgot we had
anything in the coding style document that discouraged mid-block
declarations.  I like consistency between code and documentation but I
believe that the right thing to do here is to update the coding style
document, by removing anything that discourages mid-block declarations.
Now I use them pervasively and I want to encourages others to do so too.

That's my long, rambling defense of mid-block declarations.  The other
technical issue here is about whether && and || should be at the
beginning or end of a line.  The coding style document, intentionally,
does not take an explicit position on this, and I did not realize that
the examples in the document are biased toward putting them at the
beginning of the line.  OVS uses a mix of positions.  I personally tend
to put them at the beginning of a line because I have a long history of
writing code in the GNU coding style, which mandates this positioning,
but others feel exactly the opposite (for example I believe that kernel
style favors end-of-line positioning).  This is a debate that frankly I
don't care much for and I'd prefer to leave it as a matter to individual
taste.  (This is also why the coding style document gives some freedom
in the positioning of /* and */ in comment blocks: there is a range of
viewpoints on this and I think the answer doesn't matter.)  If
necessary, I favor adding an explicit statement to the document saying
that lines may be split before or after binary operators.  (On the other
hand I do prefer ? and : at the beginning of a line.)

I believe that there is also a nontechnical aspect here.  I think that
might take a different email to address.  Let me see what I can do.

[ovs-dev] [patch v1 3/4] conntrack: Disable algs by default.

[Darrell Ball]

Presently, alg processing is enabled by default to exercise testing.
This is similar to kernels before 4.7.  The recommended default
behavior in the kernel is to only process algs if a helper is
supplied in a conntrack rule.  The behavior is changed to match the
later kernels.

Signed-off-by: Darrell Ball 
---
 lib/conntrack.c | 35 +--
 1 file changed, 29 insertions(+), 6 deletions(-)

diff --git a/lib/conntrack.c b/lib/conntrack.c
index 7fbcfba..dea2fed 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -789,13 +789,34 @@ conn_clean(struct conntrack *ct, struct conn *conn,
 }
 }
 
+static bool
+ct_verify_helper(const char *helper, enum ct_alg_ctl_type ct_alg_ctl)
+{
+if (ct_alg_ctl == CT_ALG_CTL_NONE) {
+return true;
+} else if (helper) {
+if ((ct_alg_ctl == CT_ALG_CTL_FTP) &&
+ !strncmp(helper, "ftp", strlen("ftp"))) {
+return true;
+} else if ((ct_alg_ctl == CT_ALG_CTL_TFTP) &&
+   !strncmp(helper, "tftp", strlen("tftp"))) {
+return true;
+} else {
+return false;
+}
+} else {
+return false;
+}
+}
+
 /* This function is called with the bucket lock held. */
 static struct conn *
 conn_not_found(struct conntrack *ct, struct dp_packet *pkt,
struct conn_lookup_ctx *ctx, bool commit, long long now,
const struct nat_action_info_t *nat_action_info,
struct conn *conn_for_un_nat_copy,
-   const char *helper, const struct alg_exp_node *alg_exp)
+   const char *helper, const struct alg_exp_node *alg_exp,
+   enum ct_alg_ctl_type ct_alg_ctl)
 {
 struct conn *nc = NULL;
 
@@ -819,15 +840,16 @@ conn_not_found(struct conntrack *ct, struct dp_packet 
*pkt,
 return nc;
 }
 
+if (!ct_verify_helper(helper, ct_alg_ctl)) {
+return nc;
+}
+
 unsigned bucket = hash_to_bucket(ctx->hash);
 nc = new_conn(>buckets[bucket], pkt, >key, now);
 ctx->conn = nc;
 nc->rev_key = nc->key;
 conn_key_reverse(>rev_key);
-
-if (helper) {
-nc->alg = xstrdup(helper);
-}
+nc->alg = nullable_xstrdup(helper);
 
 if (alg_exp) {
 nc->alg_related = true;
@@ -1182,7 +1204,8 @@ process_one(struct conntrack *ct, struct dp_packet *pkt,
 }
 ct_rwlock_unlock(>resources_lock);
 conn = conn_not_found(ct, pkt, ctx, commit, now, nat_action_info,
-  _for_un_nat_copy, helper, alg_exp);
+  _for_un_nat_copy, helper, alg_exp,
+  ct_alg_ctl);
 }
 write_ct_md(pkt, zone, conn, >key, alg_exp);
 
-- 
1.9.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[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, 

[ovs-dev] [patch v1 4/4] conntrack: Change established state to match kernel.

[Darrell Ball]

Presently, the userpace connection tracker 'established' packet
state diverges from the kernel and this patch brings them in line.
The behavior is now that 'established' is only possible after a
reply packet is seen.
The previous behavior is hard to notice when rules are written to
commit a connection in the trusted direction, which is recommended.

A test is added to verify this.

The documentation is updated to describe the new behavior of
'established' and also clarify 'new'.

Signed-off-by: Darrell Ball 
---
 lib/conntrack-private.h |  1 +
 lib/conntrack.c | 21 -
 lib/meta-flow.xml   | 10 +++---
 tests/system-traffic.at | 35 +++
 4 files changed, 59 insertions(+), 8 deletions(-)

diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h
index ac0198f..1f6a107 100644
--- a/lib/conntrack-private.h
+++ b/lib/conntrack-private.h
@@ -107,6 +107,7 @@ struct conn {
 uint8_t seq_skew_dir;
 /* True if alg data connection. */
 uint8_t alg_related;
+uint8_t reply_seen;
 };
 
 enum ct_update_res {
diff --git a/lib/conntrack.c b/lib/conntrack.c
index dea2fed..323114a 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -928,6 +928,21 @@ nat_res_exhaustion:
 return NULL;
 }
 
+/* This function is called with the bucket lock held. */
+static void
+conn_handle_reply(struct dp_packet *pkt, const struct conn_lookup_ctx *ctx,
+  struct conn **conn)
+{
+if (ctx->reply) {
+pkt->md.ct_state |= CS_REPLY_DIR;
+(*conn)->reply_seen = true;
+}
+if ((*conn)->reply_seen) {
+ pkt->md.ct_state |= CS_ESTABLISHED;
+ pkt->md.ct_state &= ~CS_NEW;
+}
+}
+
 static bool
 conn_update_state(struct conntrack *ct, struct dp_packet *pkt,
   struct conn_lookup_ctx *ctx, struct conn **conn,
@@ -950,11 +965,7 @@ conn_update_state(struct conntrack *ct, struct dp_packet 
*pkt,
 
 switch (res) {
 case CT_UPDATE_VALID:
-pkt->md.ct_state |= CS_ESTABLISHED;
-pkt->md.ct_state &= ~CS_NEW;
-if (ctx->reply) {
-pkt->md.ct_state |= CS_REPLY_DIR;
-}
+conn_handle_reply(pkt, ctx, conn);
 break;
 case CT_UPDATE_INVALID:
 pkt->md.ct_state = CS_INVALID;
diff --git a/lib/meta-flow.xml b/lib/meta-flow.xml
index 08ee0ec..33a2ad6 100644
--- a/lib/meta-flow.xml
+++ b/lib/meta-flow.xml
@@ -2493,13 +2493,17 @@ actions=clone(load:0->NXM_OF_IN_PORT[],output:123)
   
 new (0x01)
 
-  A new connection.  Set to 1 if this is an uncommitted connection.
+  A new connection.  Set to 1 if this is an uncommitted connection
+  or a committed connection that has not seen a reply yet.
 
 
 est (0x02)
 
-  Part of an existing connection.  Set to 1 if this is a committed
-  connection.
+  There are two requirements for a packet to be established, namely,
+  the associated connection must be committed and a reply must have
+  been seen.  The reply packet that creates this condition will be
+  marked as established as well as subsequent packets in either
+  direction that are associated with the same conntrack entry.
 
 
 rel (0x04)
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index fd7b612..cd55406 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -871,6 +871,41 @@ NS_CHECK_EXEC([at_ns1], [ping -q -c 3 -i 0.3 -w 2 10.1.1.1 
| FORMAT_PING], [0],
 OVS_TRAFFIC_VSWITCHD_STOP
 AT_CLEANUP
 
+AT_SETUP([conntrack - IPv4 ping Check Est state])
+CHECK_CONNTRACK()
+OVS_TRAFFIC_VSWITCHD_START()
+
+ADD_NAMESPACES(at_ns0, at_ns1)
+
+ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
+ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
+
+dnl Allow any traffic from ns0->ns1. Only allow nd, return traffic from 
ns1->ns0.
+dnl Check that packet is not established before a reply.
+AT_DATA([flows.txt], [dnl
+priority=1,action=drop
+priority=10,arp,action=normal
+table=0,priority=10,in_port=2,icmp,ct_state=-trk,action=ct(table=0)
+table=0,priority=10,in_port=2,icmp,ct_state=+trk+est actions=output:1
+table=0,priority=10,in_port=1,icmp actions=ct(commit,table=1)
+table=1,priority=10,in_port=1,icmp,ct_state=+trk+new actions=ct(table=2)
+table=2,priority=10,in_port=1,icmp,ct_state=+trk+new actions=output:2
+])
+
+AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt])
+
+dnl Pings from ns0->ns1 should work fine.
+NS_CHECK_EXEC([at_ns0], [ping -q -c 1 -i 0.3 -w 2 10.1.1.2 | FORMAT_PING], 
[0], [dnl
+1 packets transmitted, 1 received, 0% packet loss, time 0ms
+])
+
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2)], [0], [dnl
+icmp,orig=(src=10.1.1.1,dst=10.1.1.2,id=,type=8,code=0),reply=(src=10.1.1.2,dst=10.1.1.1,id=,type=0,code=0)
+])
+
+OVS_TRAFFIC_VSWITCHD_STOP
+AT_CLEANUP
+
 AT_SETUP([conntrack - IPv6 ping])
 CHECK_CONNTRACK()
 

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

[Ben Pfaff]

On Fri, Nov 17, 2017 at 07:34:49PM +, Darrell Ball 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.
> Most of this patch is related to this.
> I’ll point out a few to provide examples, but I don’t like this – nack.

This reads to me as more aggressive than necessary.  The OVS coding
style says to mix declarations and code within a block "judiciously" and
to "keep declarations nicely grouped together in the beginning of a
block if possible."  I think that this now inaccurately describes
current coding practice in OVS, as well as best practice for modern C,
but it is still what the document says, so it is understandable that
anyone reading the document would want to fix the code to match.

I tend to take this patch as a kind of bug report against the coding
style document.  The right response to that might be to explain why the
coding style document is wrong and the history behind it, and to thank
the submitter (thank you, Flavio!).  I suggest that we should update the
coding style document to match our current practice (and perhaps to
motivate current practice with rationale).

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

This reads as somewhat aggressive too, especially following #1.

I believe that Flavio interpreted the examples in the coding style
document as normative.  That is understandable, because the document
does not say anything explicitly.  When we wrote the document in 2007, I
don't remember Justin and I even discussing whether lines should be
split before or after binary operators, and we've never (as far as I
know) interpreted those particular examples as normative in that aspect.

Again, I would prefer to interpret this part of the patch as a bug
report against the coding standards.  Thank you, Flavio, for bringing
this to our attention.  I think that it would be a good idea to modify
at least one of the examples to show a line break after a binary
operator, and perhaps to add an explicit statement that there is no norm
for positioning binary operators before or after a line break.  (Maybe
there is someone out there who wants to campaign for particular
positioning, but I'll leave that to whoever that is.)

> There is a missing space after “}” in one instance - thanks
> There are also use of full 79 line lengths in a few places, which looked ok, 
> but I did not check carefully.
> There is some missed extra newlines after declarations, which generally looks 
> ok; I’ll check more however.
> I also see some extra newlines removed which looked ok, but I’ll check more.
> 
> I’ll submit my own patch since I don’t agree with “1” and “2”.

Sometimes it is helpful to propose a competing patch, but usually that
would follow something closer to coming to consensus, or sometimes after
discussion makes it clear that the new proposer understands some issue
better than the original poster or is otherwise better situated to
help.  In this case, it reads more as a kind of aggression.

I hope that we can resolve this, technically and nontechnically, to
everyone's satisfaction.

Thanks

Ben.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [ovs-discuss] Conntrack issue in OVS (2.6)+DPDK

[Darrell Ball]

Hi Jan

As discussed and agreed at OVSCON, I submitted a patch to bring the userspace 
connection tracker established state
In line with that of the kernel.
I used a similar patch to what I earlier suggested earlier in this thread, 
adding a test and also made some documentation updates.

Some of the discussion in this thread was somewhat orthogonal to bringing 
userspace ‘established’ in line with kernel
‘established’, but it appears to have been useful as some new recommendations 
may come out of it with respect to
recommended practices, for conntrack pipeline design.

Thanks Darrell


From: Jan Scheurich 
Date: Saturday, November 4, 2017 at 4:54 AM
To: Darrel Ball , Rohith Basavaraja 

Cc: "d...@openvswitch.org" 
Subject: RE: [ovs-discuss] Conntrack issue in OVS (2.6)+DPDK

Hi Darrel,

The example pipeline I crafted was not meant to be a realistic conntrack 
application but to demonstrate the semantic differences between userspace and 
kernel implementation and to discuss our problems with the current 
documentation.

I fully agree with your proposal for a proper ICMP set of rules. It would work 
equally for kernel and userspace datapath. But there are other rule sets where 
there they behave differently and we believe this is not good.

The original pipeline brought up by Rohith in August is the implementation of 
OpenStack Security Groups in OpenDaylight. In general ODL does not commit 
connections in the untrusted direction. However, in the problematic scenario 
(two Neutron ports in the same Neutron Network but in different Security 
Groups, co-located on the same OVS instance) the connection was committed (as 
trusted) on the sending side. The packet should have been dropped on the 
receiving side but the ct() lookup for the first packet on egress hits the 
committed connection and passes because ODL uses one conntrack zone per Neutron 
Network rather than per Security Group. I think this is wrong and using one 
zone per Security Group would probably solve this specific issue.

But with the kernel datapath this issue never surfaced because the connection 
is not considered established prior to the first reply packet so that the 
second lookup of the first packet on egress still yields +new-est. So the ODL 
developers testing with kernel datapath assumed their design was suitable. You 
can argue that was a misunderstanding of the function but the discrepancy 
between documentation and kernel behavior certainly didn’t help.

Perhaps it is better we continue this discussion in person during the OVS 
conference?

Regards, Jan

From: Darrell Ball [mailto:db...@vmware.com]
Sent: Saturday, 04 November, 2017 01:47
To: Jan Scheurich ; Rohith Basavaraja 

Cc: d...@openvswitch.org
Subject: Re: [ovs-discuss] Conntrack issue in OVS (2.6)+DPDK



From: Jan Scheurich 
>
Date: Friday, November 3, 2017 at 6:22 AM
To: Darrel Ball >, Rohith Basavaraja 
>
Cc: "d...@openvswitch.org" 
>
Subject: RE: [ovs-discuss] Conntrack issue in OVS (2.6)+DPDK

Hi Darrel,

I have now been able to actually test the example pipelines I provided earlier. 
Turns out that the first one I sent was correct.

[Darrell] Sure, let us discuss the first example; let me know if you want to 
discuss the second example you gave as well.


Please note that it was not meant as realistic conntrack pipeline

Darrell] Again, I can agree your example is not realistic or recommended. No 
one would write rules like this.
  The rules would certainly be written properly so that the trusted 
direction (the one that does the commit) allows the first packet through;
  this is a fundamental principle of conntrack. There are an 
infinite number of ways to misuse conntrack rules and no one can prevent misuse.
  On a similar topic, another fundamental problem I saw with the 
original discussion (from Aug) is creating a conntrack pipeline that commits
  a connection in the untrusted direction. That is also not 
something we do or recommend others do. This ‘suboptimal design approach’ 
brought
  us to the question on when a packet gets labelled as ESTABLISHED. 
 Normally, the difference would not be noticed, since a connection would not be
  committed in the untrusted direction and hence EST would not be 
possible unless another rule correctly commits in the trusted direction.
  I’ll add more comments below.


but just to demonstrate the misalignment between userspace and kernel conntrack 
and the conflict of both with the documentation.

The following pipeline is now tested:

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

[Flavio Leitner]

On Sat, 18 Nov 2017 00:41:27 +
Darrell Ball  wrote:

> On 11/17/17, 3:01 PM, "Flavio Leitner"  wrote:
> 
> On Fri, 17 Nov 2017 19:34:49 +
> Darrell Ball  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 

[ovs-dev] Grüße

[James Kross]

Grüße

Ich bin Herr James Kross, ein Rechtsanwalt in der UK.I ich erreichen zu Ihnen 
in Bezug auf eine Investitionsmöglichkeit.

Ich erbitten Ihre Hilfe bei der Rückführung der Gelder und die Eigenschaften 
links von meinem verstorbenen Kunden, bevor sie un-brauchbar von der Bank, wo 
die riesigen Kaution hinterlegt und von der Regierung beschlagnahmt erklärt. 
Mein Kunde starb Intestate (ohne Will) und jeder Versuch, ein Mitglied seiner 
Großfamilie hat sich als Spuren erfolglos wie er mit seiner gesamten ersten 
Familie starb.

Ich gebe Ihnen weitere Informationen auf Ihre Antwort. Wenn dieser Vorschlag 
für Sie akzeptabel ist, bitte senden Sie mir folgendeInformationen für eine 
weitere Bestätigung.

A. privaten Telefon-und Faxnummern.

B . Ihre Post / Wohnadresse.

Ich demütig freuen uns auf Ihre Antwort bald.

Mit freundlichen Grüßen,
James Kross Esq

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

[Darrell Ball]

On 11/19/17, 10:25 AM, "Flavio Leitner"  wrote:

On Sat, 18 Nov 2017 00:41:27 +
Darrell Ball  wrote:

> On 11/17/17, 3:01 PM, "Flavio Leitner"  wrote:
> 
> On Fri, 17 Nov 2017 19:34:49 +
> Darrell Ball  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.


Just for the record:

The ALG patches were posted for 3 full months and had multiple very detailed 
reviews and reviewers.
I accepted almost all comments, even some that I felt were borderline.
Is 3 months not enough ?

The NAT patches were posted for 5 full months, had multiple reviewers, 
including yourself,
who acked them. I accepted almost all comments there as well.
Is 5 months not enough ?

The same holds for other features as well.





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 

[ovs-dev] [patch v1 2/4] conntrack: Refactor algs.

[Darrell Ball]

Upcoming requirements for new algs make it necessary to split out
alg helper more cleanly.

Signed-off-by: Darrell Ball 
---
 lib/conntrack.c | 73 ++---
 1 file changed, 44 insertions(+), 29 deletions(-)

diff --git a/lib/conntrack.c b/lib/conntrack.c
index b8d0e77..7fbcfba 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -67,6 +67,12 @@ enum ct_alg_mode {
 CT_TFTP_MODE,
 };
 
+enum ct_alg_ctl_type {
+CT_ALG_CTL_NONE,
+CT_ALG_CTL_FTP,
+CT_ALG_CTL_TFTP,
+};
+
 static bool conn_key_extract(struct conntrack *, struct dp_packet *,
  ovs_be16 dl_type, struct conn_lookup_ctx *,
  uint16_t zone);
@@ -432,33 +438,47 @@ get_ip_proto(const struct dp_packet *pkt)
 }
 
 static bool
-is_ftp_ctl(const struct dp_packet *pkt)
+is_ftp_ctl(const enum ct_alg_ctl_type ct_alg_ctl)
+{
+return ct_alg_ctl == CT_ALG_CTL_FTP;
+}
+
+static enum ct_alg_ctl_type
+get_alg_ctl_type(const struct dp_packet *pkt)
 {
 uint8_t ip_proto = get_ip_proto(pkt);
+struct udp_header *uh = dp_packet_l4(pkt);
 struct tcp_header *th = dp_packet_l4(pkt);
 
-/* CT_IPPORT_FTP is used because IPPORT_FTP in not defined in OSX,
- * at least in in.h. Since this value will never change, just remove
+/* CT_IPPORT_FTP/TFTP is used because IPPORT_FTP/TFTP in not defined
+ * in OSX, at least in in.h. Since these values will never change, remove
  * the external dependency. */
-#define CT_IPPORT_FTP 21
+enum { CT_IPPORT_FTP = 21 };
+enum { CT_IPPORT_TFTP = 69 };
 
-return (ip_proto == IPPROTO_TCP &&
-(th->tcp_src == htons(CT_IPPORT_FTP) ||
- th->tcp_dst == htons(CT_IPPORT_FTP)));
+if (ip_proto == IPPROTO_UDP && uh->udp_dst == htons(CT_IPPORT_TFTP)) {
+return CT_ALG_CTL_TFTP;
+} else if (ip_proto == IPPROTO_TCP &&
+   (th->tcp_src == htons(CT_IPPORT_FTP) ||
+th->tcp_dst == htons(CT_IPPORT_FTP))) {
+return CT_ALG_CTL_FTP;
+}
+return CT_ALG_CTL_NONE;
 }
 
-static bool
-is_tftp_ctl(const struct dp_packet *pkt)
+static void
+handle_alg_ctl(struct conntrack *ct, const struct conn_lookup_ctx *ctx,
+   struct dp_packet *pkt, enum ct_alg_ctl_type ct_alg_ctl,
+   const struct conn *conn, long long now, bool nat,
+   const struct conn *conn_for_expectation)
 {
- 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
- * the external dependency. */
-#define CT_IPPORT_TFTP 69
-return (ip_proto == IPPROTO_UDP &&
-uh->udp_dst == htons(CT_IPPORT_TFTP));
+/* ALG control packet handling with expectation creation. */
+if (OVS_UNLIKELY((ct_alg_ctl == CT_ALG_CTL_FTP) && conn)) {
+handle_ftp_ctl(ct, ctx, pkt, conn_for_expectation,
+   now, CT_FTP_CTL_INTEREST, nat);
+} else if (OVS_UNLIKELY((ct_alg_ctl == CT_ALG_CTL_TFTP) && conn)) {
+handle_tftp_ctl(ct, conn_for_expectation, now);
+}
 }
 
 static void
@@ -1110,10 +1130,11 @@ process_one(struct conntrack *ct, struct dp_packet *pkt,
 bool create_new_conn = false;
 struct conn conn_for_un_nat_copy;
 conn_for_un_nat_copy.conn_type = CT_CONN_TYPE_DEFAULT;
-bool ftp_ctl = is_ftp_ctl(pkt);
+
+enum ct_alg_ctl_type ct_alg_ctl = get_alg_ctl_type(pkt);
 
 if (OVS_LIKELY(conn)) {
-if (ftp_ctl) {
+if (is_ftp_ctl(ct_alg_ctl)) {
 /* Keep sequence tracking in sync with the source of the
  * sequence skew. */
 if (ctx->reply != conn->seq_skew_dir) {
@@ -1173,9 +1194,8 @@ process_one(struct conntrack *ct, struct dp_packet *pkt,
 set_label(pkt, conn, [0], [1]);
 }
 
-bool tftp_ctl = is_tftp_ctl(pkt);
 struct conn conn_for_expectation;
-if (OVS_UNLIKELY((ftp_ctl || tftp_ctl) && conn)) {
+if (OVS_UNLIKELY((ct_alg_ctl != CT_ALG_CTL_NONE) && conn)) {
 conn_for_expectation = *conn;
 }
 
@@ -1185,13 +1205,8 @@ process_one(struct conntrack *ct, struct dp_packet *pkt,
 create_un_nat_conn(ct, _for_un_nat_copy, now, !!alg_exp);
 }
 
-/* FTP control packet handling with expectation creation. */
-if (OVS_UNLIKELY(ftp_ctl && conn)) {
-handle_ftp_ctl(ct, ctx, pkt, _for_expectation,
-   now, CT_FTP_CTL_INTEREST, !!nat_action_info);
-} else if (OVS_UNLIKELY(tftp_ctl && conn)) {
-handle_tftp_ctl(ct, _for_expectation, now);
-}
+handle_alg_ctl(ct, ctx, pkt, ct_alg_ctl, conn, now, !!nat_action_info,
+   _for_expectation);
 }
 
 /* Sends the packets in '*pkt_batch' through the connection tracker 'ct'.  All
-- 
1.9.1

___
dev mailing list
d...@openvswitch.org

[ovs-dev] [patch v1 0/4] conntrack: Improvements and fixes.

[Darrell Ball]

There are 3 main changes:
Bring the userpace connection tracker 'established' state in
line with the kernel.
Disable alg processing by default and only allow it if the alg
helper is supplied in the conntrack commit rule.
There is also some refactoring to more cleanly splice out alg
checking/helpers.

Darrell Ball (4):
  conntrack: Some formatting improvements.
  conntrack: Refactor algs.
  conntrack: Disable algs by default.
  conntrack: Change established state to match kernel.

 lib/conntrack-private.h |   1 +
 lib/conntrack.c | 313 ++--
 lib/meta-flow.xml   |  10 +-
 tests/system-traffic.at |  35 ++
 4 files changed, 214 insertions(+), 145 deletions(-)

-- 
1.9.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] odp-execute: Bug Fix - missing break statement for CLONE action

[Vishal Deep Ajmera]

Hi,

Do any one sees issues with this patch ? If let loose, it may create issues 
once we have implementation of METER.

Warm Regards,
Vishal

From: Vishal Deep Ajmera
Sent: Thursday, November 09, 2017 3:45 PM
To: d...@openvswitch.org
Subject: [PATCH] odp-execute: Bug Fix - missing break statement for CLONE action

Adding the break statement for CLONE action case.

Signed-off-by: Vishal Deep Ajmera 
>
---
lib/odp-execute.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/lib/odp-execute.c b/lib/odp-execute.c
index 3011479..2d20cd5 100644
--- a/lib/odp-execute.c
+++ b/lib/odp-execute.c
@@ -800,6 +800,7 @@ odp_execute_actions(void *dp, struct dp_packet_batch 
*batch, bool steal,
  * stolen them.  */
 return;
 }
+break;
 case OVS_ACTION_ATTR_METER:
 /* Not implemented yet. */
 break;

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

[Darrell Ball]

On 11/19/17, 11:18 AM, "Ben Pfaff"  wrote:

On Fri, Nov 17, 2017 at 07:34:49PM +, Darrell Ball 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.
> Most of this patch is related to this.
> I’ll point out a few to provide examples, but I don’t like this – nack.

This reads to me as more aggressive than necessary.  The OVS coding
style says to mix declarations and code within a block "judiciously" and
to "keep declarations nicely grouped together in the beginning of a
block if possible."  I think that this now inaccurately describes
current coding practice in OVS, as well as best practice for modern C,
but it is still what the document says, so it is understandable that
anyone reading the document would want to fix the code to match.


I tend to take this patch as a kind of bug report against the coding
style document.  The right response to that might be to explain why the
coding style document is wrong and the history behind it, and to thank
the submitter (thank you, Flavio!).  I suggest that we should update the
coding style document to match our current practice (and perhaps to
motivate current practice with rationale).

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

This reads as somewhat aggressive too, especially following #1.

Hmm, not intentionally then “and it's certainly not ad hominem” – I read that 
on another alias thread once
(it is easy to search for, btw) - “not ad hominem” is accurate here as well.


I believe that Flavio interpreted the examples in the coding style
document as normative.  That is understandable, because the document
does not say anything explicitly.  When we wrote the document in 2007, I
don't remember Justin and I even discussing whether lines should be
split before or after binary operators, and we've never (as far as I
know) interpreted those particular examples as normative in that aspect.

Again, I would prefer to interpret this part of the patch as a bug
report against the coding standards.  Thank you, Flavio, for bringing
this to our attention.  I think that it would be a good idea to modify
at least one of the examples to show a line break after a binary
operator, and perhaps to add an explicit statement that there is no norm
for positioning binary operators before or after a line break.  (Maybe
there is someone out there who wants to campaign for particular
positioning, but I'll leave that to whoever that is.)

> There is a missing space after “}” in one instance - thanks
> There are also use of full 79 line lengths in a few places, which looked 
ok, but I did not check carefully.
> There is some missed extra newlines after declarations, which generally 
looks ok; I’ll check more however.
> I also see some extra newlines removed which looked ok, but I’ll check 
more.
> 
> I’ll submit my own patch since I don’t agree with “1” and “2”.

Sometimes it is helpful to propose a competing patch, but usually that
would follow something closer to coming to consensus, or sometimes after
discussion makes it clear that the new proposer understands some issue
better than the original poster or is otherwise better situated to
help.  In this case, it reads more as a kind of aggression.

I was not referring to a competing patch, but rather a patch that fixes the 
code to match 
the effective coding standard, as I understand it, in some missing cases, but 
also has a commit message
that lists each coding standard. The patch is different than Flavio’s but 
Flavio’s patch made me
look more while I was working on related patches.

https://patchwork.ozlabs.org/patch/839358/



I hope that we can resolve this, technically and nontechnically, to
everyone's satisfaction.

Thanks

Ben.


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev