| Message ID | 1498068412-89824-2-git-send-email-yihung.wei@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On 6/21/17, 11:06 AM, "ovs-dev-bounces@openvswitch.org on behalf of Yi-Hung Wei" <ovs-dev-bounces@openvswitch.org on behalf of yihung.wei@gmail.com> wrote:
This patch moves conntrack state parsing function from ovn-trace.c to
lib/conntrack.c, because it will be used by ofproto/trace unixctl command
later on. It also updates the ct_state checking logic, since we no longer
assume CS_TRACKED is enable by default.
Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>
---
lib/conntrack.c | 40 ++++++++++++++++++++++++++++++++++++++++
lib/conntrack.h | 1 +
This file is userspace datapath specific; the function being moved can probably
go to packet.c as we define the CT states already in packet.h.
ovn/utilities/ovn-trace.c | 30 ++----------------------------
3 files changed, 43 insertions(+), 28 deletions(-)
diff --git a/lib/conntrack.c b/lib/conntrack.c
index 90b154a87a74..901003ae0801 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -2030,3 +2030,43 @@ conntrack_flush(struct conntrack *ct, const uint16_t *zone)
}
return 0;
}
+
+uint32_t
+parse_ct_state(const char *state_s_, uint32_t default_state)
+{
+ uint32_t state = default_state;
+
+ char *state_s = xstrdup(state_s_);
+ char *save_ptr = NULL;
+ for (char *cs = strtok_r(state_s, ", ", &save_ptr); cs;
+ cs = strtok_r(NULL, ", ", &save_ptr)) {
+ uint32_t bit = ct_state_from_string(cs);
+ if (!bit) {
+ ovs_fatal(0, "%s: unknown connection tracking state flag", cs);
+ }
+ state |= bit;
+ }
+ free(state_s);
+
+ /* Check constraints listed in ovs-fields(7). */
+ if (state && !(state & CS_TRACKED)) {
+ VLOG_WARN("%s: invalid connection state: "
+ "If \"trk\" is unset, no other flags are set",
+ state_s_);
+ }
+ if (state & CS_INVALID && state & ~(CS_TRACKED | CS_INVALID)) {
+ VLOG_WARN("%s: invalid connection state: "
+ "when \"inv\" is set, only \"trk\" may also be set",
+ state_s_);
+ }
+ if (state & CS_NEW && state & CS_ESTABLISHED) {
+ VLOG_WARN("%s: invalid connection state: "
+ "\"new\" and \"est\" are mutually exclusive", state_s_);
+ }
+ if (state & CS_NEW && state & CS_REPLY_DIR) {
+ VLOG_WARN("%s: invalid connection state: "
+ "\"new\" and \"rpy\" are mutually exclusive", state_s_);
+ }
+
+ return state;
+}
diff --git a/lib/conntrack.h b/lib/conntrack.h
index 243aebb642f7..c623d79c1803 100644
--- a/lib/conntrack.h
+++ b/lib/conntrack.h
@@ -113,6 +113,7 @@ int conntrack_dump_next(struct conntrack_dump *, struct ct_dpif_entry *);
int conntrack_dump_done(struct conntrack_dump *);
int conntrack_flush(struct conntrack *, const uint16_t *zone);
+uint32_t parse_ct_state(const char *ct_states, uint32_t default_state);
?
/* 'struct ct_lock' is a wrapper for an adaptive mutex. It's useful to try
* different types of locks (e.g. spinlocks) */
diff --git a/ovn/utilities/ovn-trace.c b/ovn/utilities/ovn-trace.c
index 8bdb7d8a304d..7bec26e27d2b 100644
--- a/ovn/utilities/ovn-trace.c
+++ b/ovn/utilities/ovn-trace.c
@@ -20,6 +20,7 @@
#include "command-line.h"
#include "compiler.h"
+#include "conntrack.h"
#include "daemon.h"
#include "dirs.h"
#include "fatal-signal.h"
@@ -169,34 +170,7 @@ default_ovs(void)
static void
parse_ct_option(const char *state_s_)
{
- uint32_t state = CS_TRACKED;
-
- char *state_s = xstrdup(state_s_);
- char *save_ptr = NULL;
- for (char *cs = strtok_r(state_s, ", ", &save_ptr); cs;
- cs = strtok_r(NULL, ", ", &save_ptr)) {
- uint32_t bit = ct_state_from_string(cs);
- if (!bit) {
- ovs_fatal(0, "%s: unknown connection tracking state flag", cs);
- }
- state |= bit;
- }
- free(state_s);
-
- /* Check constraints listed in ovs-fields(7). */
- if (state & CS_INVALID && state & ~(CS_TRACKED | CS_INVALID)) {
- VLOG_WARN("%s: invalid connection state: "
- "when \"inv\" is set, only \"trk\" may also be set",
- state_s_);
- }
- if (state & CS_NEW && state & CS_ESTABLISHED) {
- VLOG_WARN("%s: invalid connection state: "
- "\"new\" and \"est\" are mutually exclusive", state_s_);
- }
- if (state & CS_NEW && state & CS_REPLY_DIR) {
- VLOG_WARN("%s: invalid connection state: "
- "\"new\" and \"rpy\" are mutually exclusive", state_s_);
- }
+ uint32_t state = parse_ct_state(state_s_, CS_TRACKED);
ct_states = xrealloc(ct_states, (n_ct_states + 1) * sizeof *ct_states);
ct_states[n_ct_states++] = state;
--
2.7.4
_______________________________________________
dev mailing list
dev@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=re4JKD28cBRk_Jel2gAJThuouBDVdP6TphYh7zF7HvM&s=QR34jGwxJS3QONr4nmdwH-CKj9sWHAd5NJMoit_CqzQ&e=
On Wed, Jun 21, 2017 at 11:19 AM, Darrell Ball <dball@vmware.com> wrote: > > > On 6/21/17, 11:06 AM, "ovs-dev-bounces@openvswitch.org on behalf of Yi-Hung Wei" <ovs-dev-bounces@openvswitch.org on behalf of yihung.wei@gmail.com> wrote: > > This patch moves conntrack state parsing function from ovn-trace.c to > lib/conntrack.c, because it will be used by ofproto/trace unixctl command > later on. It also updates the ct_state checking logic, since we no longer > assume CS_TRACKED is enable by default. > > Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com> > --- > lib/conntrack.c | 40 ++++++++++++++++++++++++++++++++++++++++ > lib/conntrack.h | 1 + > > This file is userspace datapath specific; the function being moved can probably > go to packet.c as we define the CT states already in packet.h. Thanks for review. I agree that lib/conntrack.c is not that appropriate to put this function. After further investigation, given that ct_state_from_string() is in lib/flow.c, I move the ct state parsing function to flow.c. > > > ovn/utilities/ovn-trace.c | 30 ++---------------------------- > 3 files changed, 43 insertions(+), 28 deletions(-) > > diff --git a/lib/conntrack.c b/lib/conntrack.c > index 90b154a87a74..901003ae0801 100644 > --- a/lib/conntrack.c > +++ b/lib/conntrack.c > @@ -2030,3 +2030,43 @@ conntrack_flush(struct conntrack *ct, const uint16_t *zone) > } > return 0; > } > + > +uint32_t > +parse_ct_state(const char *state_s_, uint32_t default_state) > +{ > + uint32_t state = default_state; > + > + char *state_s = xstrdup(state_s_); > + char *save_ptr = NULL; > + for (char *cs = strtok_r(state_s, ", ", &save_ptr); cs; > + cs = strtok_r(NULL, ", ", &save_ptr)) { > + uint32_t bit = ct_state_from_string(cs); > + if (!bit) { > + ovs_fatal(0, "%s: unknown connection tracking state flag", cs); > + } > + state |= bit; > + } > + free(state_s); > + > + /* Check constraints listed in ovs-fields(7). */ > + if (state && !(state & CS_TRACKED)) { > + VLOG_WARN("%s: invalid connection state: " > + "If \"trk\" is unset, no other flags are set", > + state_s_); > + } > + if (state & CS_INVALID && state & ~(CS_TRACKED | CS_INVALID)) { > + VLOG_WARN("%s: invalid connection state: " > + "when \"inv\" is set, only \"trk\" may also be set", > + state_s_); > + } > + if (state & CS_NEW && state & CS_ESTABLISHED) { > + VLOG_WARN("%s: invalid connection state: " > + "\"new\" and \"est\" are mutually exclusive", state_s_); > + } > + if (state & CS_NEW && state & CS_REPLY_DIR) { > + VLOG_WARN("%s: invalid connection state: " > + "\"new\" and \"rpy\" are mutually exclusive", state_s_); > + } > + > + return state; > +} > diff --git a/lib/conntrack.h b/lib/conntrack.h > index 243aebb642f7..c623d79c1803 100644 > --- a/lib/conntrack.h > +++ b/lib/conntrack.h > @@ -113,6 +113,7 @@ int conntrack_dump_next(struct conntrack_dump *, struct ct_dpif_entry *); > int conntrack_dump_done(struct conntrack_dump *); > > int conntrack_flush(struct conntrack *, const uint16_t *zone); > +uint32_t parse_ct_state(const char *ct_states, uint32_t default_state); > ? > /* 'struct ct_lock' is a wrapper for an adaptive mutex. It's useful to try > * different types of locks (e.g. spinlocks) */ > diff --git a/ovn/utilities/ovn-trace.c b/ovn/utilities/ovn-trace.c > index 8bdb7d8a304d..7bec26e27d2b 100644 > --- a/ovn/utilities/ovn-trace.c > +++ b/ovn/utilities/ovn-trace.c > @@ -20,6 +20,7 @@ > > #include "command-line.h" > #include "compiler.h" > +#include "conntrack.h" > #include "daemon.h" > #include "dirs.h" > #include "fatal-signal.h" > @@ -169,34 +170,7 @@ default_ovs(void) > static void > parse_ct_option(const char *state_s_) > { > - uint32_t state = CS_TRACKED; > - > - char *state_s = xstrdup(state_s_); > - char *save_ptr = NULL; > - for (char *cs = strtok_r(state_s, ", ", &save_ptr); cs; > - cs = strtok_r(NULL, ", ", &save_ptr)) { > - uint32_t bit = ct_state_from_string(cs); > - if (!bit) { > - ovs_fatal(0, "%s: unknown connection tracking state flag", cs); > - } > - state |= bit; > - } > - free(state_s); > - > - /* Check constraints listed in ovs-fields(7). */ > - if (state & CS_INVALID && state & ~(CS_TRACKED | CS_INVALID)) { > - VLOG_WARN("%s: invalid connection state: " > - "when \"inv\" is set, only \"trk\" may also be set", > - state_s_); > - } > - if (state & CS_NEW && state & CS_ESTABLISHED) { > - VLOG_WARN("%s: invalid connection state: " > - "\"new\" and \"est\" are mutually exclusive", state_s_); > - } > - if (state & CS_NEW && state & CS_REPLY_DIR) { > - VLOG_WARN("%s: invalid connection state: " > - "\"new\" and \"rpy\" are mutually exclusive", state_s_); > - } > + uint32_t state = parse_ct_state(state_s_, CS_TRACKED); > > ct_states = xrealloc(ct_states, (n_ct_states + 1) * sizeof *ct_states); > ct_states[n_ct_states++] = state; > -- > 2.7.4 > > _______________________________________________ > dev mailing list > dev@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=re4JKD28cBRk_Jel2gAJThuouBDVdP6TphYh7zF7HvM&s=QR34jGwxJS3QONr4nmdwH-CKj9sWHAd5NJMoit_CqzQ&e= > >
On 6/26/17, 10:18 AM, "Yi-Hung Wei" <yihung.wei@gmail.com> wrote:
On Wed, Jun 21, 2017 at 11:19 AM, Darrell Ball <dball@vmware.com> wrote:
>
>
> On 6/21/17, 11:06 AM, "ovs-dev-bounces@openvswitch.org on behalf of Yi-Hung Wei" <ovs-dev-bounces@openvswitch.org on behalf of yihung.wei@gmail.com> wrote:
>
> This patch moves conntrack state parsing function from ovn-trace.c to
> lib/conntrack.c, because it will be used by ofproto/trace unixctl command
> later on. It also updates the ct_state checking logic, since we no longer
> assume CS_TRACKED is enable by default.
>
> Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>
> ---
> lib/conntrack.c | 40 ++++++++++++++++++++++++++++++++++++++++
> lib/conntrack.h | 1 +
>
> This file is userspace datapath specific; the function being moved can probably
> go to packet.c as we define the CT states already in packet.h.
Thanks for review. I agree that lib/conntrack.c is not that
appropriate to put this function. After further investigation, given
that ct_state_from_string() is in lib/flow.c, I move the ct state
parsing function to flow.c.
yes, flow.c looks more related too.
>
>
> ovn/utilities/ovn-trace.c | 30 ++----------------------------
> 3 files changed, 43 insertions(+), 28 deletions(-)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 90b154a87a74..901003ae0801 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -2030,3 +2030,43 @@ conntrack_flush(struct conntrack *ct, const uint16_t *zone)
> }
> return 0;
> }
> +
> +uint32_t
> +parse_ct_state(const char *state_s_, uint32_t default_state)
> +{
> + uint32_t state = default_state;
> +
> + char *state_s = xstrdup(state_s_);
> + char *save_ptr = NULL;
> + for (char *cs = strtok_r(state_s, ", ", &save_ptr); cs;
> + cs = strtok_r(NULL, ", ", &save_ptr)) {
> + uint32_t bit = ct_state_from_string(cs);
> + if (!bit) {
> + ovs_fatal(0, "%s: unknown connection tracking state flag", cs);
> + }
> + state |= bit;
> + }
> + free(state_s);
> +
> + /* Check constraints listed in ovs-fields(7). */
> + if (state && !(state & CS_TRACKED)) {
> + VLOG_WARN("%s: invalid connection state: "
> + "If \"trk\" is unset, no other flags are set",
> + state_s_);
> + }
> + if (state & CS_INVALID && state & ~(CS_TRACKED | CS_INVALID)) {
> + VLOG_WARN("%s: invalid connection state: "
> + "when \"inv\" is set, only \"trk\" may also be set",
> + state_s_);
> + }
> + if (state & CS_NEW && state & CS_ESTABLISHED) {
> + VLOG_WARN("%s: invalid connection state: "
> + "\"new\" and \"est\" are mutually exclusive", state_s_);
> + }
> + if (state & CS_NEW && state & CS_REPLY_DIR) {
> + VLOG_WARN("%s: invalid connection state: "
> + "\"new\" and \"rpy\" are mutually exclusive", state_s_);
> + }
> +
> + return state;
> +}
> diff --git a/lib/conntrack.h b/lib/conntrack.h
> index 243aebb642f7..c623d79c1803 100644
> --- a/lib/conntrack.h
> +++ b/lib/conntrack.h
> @@ -113,6 +113,7 @@ int conntrack_dump_next(struct conntrack_dump *, struct ct_dpif_entry *);
> int conntrack_dump_done(struct conntrack_dump *);
>
> int conntrack_flush(struct conntrack *, const uint16_t *zone);
> +uint32_t parse_ct_state(const char *ct_states, uint32_t default_state);
> ?
> /* 'struct ct_lock' is a wrapper for an adaptive mutex. It's useful to try
> * different types of locks (e.g. spinlocks) */
> diff --git a/ovn/utilities/ovn-trace.c b/ovn/utilities/ovn-trace.c
> index 8bdb7d8a304d..7bec26e27d2b 100644
> --- a/ovn/utilities/ovn-trace.c
> +++ b/ovn/utilities/ovn-trace.c
> @@ -20,6 +20,7 @@
>
> #include "command-line.h"
> #include "compiler.h"
> +#include "conntrack.h"
> #include "daemon.h"
> #include "dirs.h"
> #include "fatal-signal.h"
> @@ -169,34 +170,7 @@ default_ovs(void)
> static void
> parse_ct_option(const char *state_s_)
> {
> - uint32_t state = CS_TRACKED;
> -
> - char *state_s = xstrdup(state_s_);
> - char *save_ptr = NULL;
> - for (char *cs = strtok_r(state_s, ", ", &save_ptr); cs;
> - cs = strtok_r(NULL, ", ", &save_ptr)) {
> - uint32_t bit = ct_state_from_string(cs);
> - if (!bit) {
> - ovs_fatal(0, "%s: unknown connection tracking state flag", cs);
> - }
> - state |= bit;
> - }
> - free(state_s);
> -
> - /* Check constraints listed in ovs-fields(7). */
> - if (state & CS_INVALID && state & ~(CS_TRACKED | CS_INVALID)) {
> - VLOG_WARN("%s: invalid connection state: "
> - "when \"inv\" is set, only \"trk\" may also be set",
> - state_s_);
> - }
> - if (state & CS_NEW && state & CS_ESTABLISHED) {
> - VLOG_WARN("%s: invalid connection state: "
> - "\"new\" and \"est\" are mutually exclusive", state_s_);
> - }
> - if (state & CS_NEW && state & CS_REPLY_DIR) {
> - VLOG_WARN("%s: invalid connection state: "
> - "\"new\" and \"rpy\" are mutually exclusive", state_s_);
> - }
> + uint32_t state = parse_ct_state(state_s_, CS_TRACKED);
>
> ct_states = xrealloc(ct_states, (n_ct_states + 1) * sizeof *ct_states);
> ct_states[n_ct_states++] = state;
> --
> 2.7.4
>
> _______________________________________________
> dev mailing list
> dev@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=re4JKD28cBRk_Jel2gAJThuouBDVdP6TphYh7zF7HvM&s=QR34jGwxJS3QONr4nmdwH-CKj9sWHAd5NJMoit_CqzQ&e=
>
>
diff --git a/lib/conntrack.c b/lib/conntrack.c index 90b154a87a74..901003ae0801 100644 --- a/lib/conntrack.c +++ b/lib/conntrack.c @@ -2030,3 +2030,43 @@ conntrack_flush(struct conntrack *ct, const uint16_t *zone) } return 0; } + +uint32_t +parse_ct_state(const char *state_s_, uint32_t default_state) +{ + uint32_t state = default_state; + + char *state_s = xstrdup(state_s_); + char *save_ptr = NULL; + for (char *cs = strtok_r(state_s, ", ", &save_ptr); cs; + cs = strtok_r(NULL, ", ", &save_ptr)) { + uint32_t bit = ct_state_from_string(cs); + if (!bit) { + ovs_fatal(0, "%s: unknown connection tracking state flag", cs); + } + state |= bit; + } + free(state_s); + + /* Check constraints listed in ovs-fields(7). */ + if (state && !(state & CS_TRACKED)) { + VLOG_WARN("%s: invalid connection state: " + "If \"trk\" is unset, no other flags are set", + state_s_); + } + if (state & CS_INVALID && state & ~(CS_TRACKED | CS_INVALID)) { + VLOG_WARN("%s: invalid connection state: " + "when \"inv\" is set, only \"trk\" may also be set", + state_s_); + } + if (state & CS_NEW && state & CS_ESTABLISHED) { + VLOG_WARN("%s: invalid connection state: " + "\"new\" and \"est\" are mutually exclusive", state_s_); + } + if (state & CS_NEW && state & CS_REPLY_DIR) { + VLOG_WARN("%s: invalid connection state: " + "\"new\" and \"rpy\" are mutually exclusive", state_s_); + } + + return state; +} diff --git a/lib/conntrack.h b/lib/conntrack.h index 243aebb642f7..c623d79c1803 100644 --- a/lib/conntrack.h +++ b/lib/conntrack.h @@ -113,6 +113,7 @@ int conntrack_dump_next(struct conntrack_dump *, struct ct_dpif_entry *); int conntrack_dump_done(struct conntrack_dump *); int conntrack_flush(struct conntrack *, const uint16_t *zone); +uint32_t parse_ct_state(const char *ct_states, uint32_t default_state); /* 'struct ct_lock' is a wrapper for an adaptive mutex. It's useful to try * different types of locks (e.g. spinlocks) */ diff --git a/ovn/utilities/ovn-trace.c b/ovn/utilities/ovn-trace.c index 8bdb7d8a304d..7bec26e27d2b 100644 --- a/ovn/utilities/ovn-trace.c +++ b/ovn/utilities/ovn-trace.c @@ -20,6 +20,7 @@ #include "command-line.h" #include "compiler.h" +#include "conntrack.h" #include "daemon.h" #include "dirs.h" #include "fatal-signal.h" @@ -169,34 +170,7 @@ default_ovs(void) static void parse_ct_option(const char *state_s_) { - uint32_t state = CS_TRACKED; - - char *state_s = xstrdup(state_s_); - char *save_ptr = NULL; - for (char *cs = strtok_r(state_s, ", ", &save_ptr); cs; - cs = strtok_r(NULL, ", ", &save_ptr)) { - uint32_t bit = ct_state_from_string(cs); - if (!bit) { - ovs_fatal(0, "%s: unknown connection tracking state flag", cs); - } - state |= bit; - } - free(state_s); - - /* Check constraints listed in ovs-fields(7). */ - if (state & CS_INVALID && state & ~(CS_TRACKED | CS_INVALID)) { - VLOG_WARN("%s: invalid connection state: " - "when \"inv\" is set, only \"trk\" may also be set", - state_s_); - } - if (state & CS_NEW && state & CS_ESTABLISHED) { - VLOG_WARN("%s: invalid connection state: " - "\"new\" and \"est\" are mutually exclusive", state_s_); - } - if (state & CS_NEW && state & CS_REPLY_DIR) { - VLOG_WARN("%s: invalid connection state: " - "\"new\" and \"rpy\" are mutually exclusive", state_s_); - } + uint32_t state = parse_ct_state(state_s_, CS_TRACKED); ct_states = xrealloc(ct_states, (n_ct_states + 1) * sizeof *ct_states); ct_states[n_ct_states++] = state;
This patch moves conntrack state parsing function from ovn-trace.c to lib/conntrack.c, because it will be used by ofproto/trace unixctl command later on. It also updates the ct_state checking logic, since we no longer assume CS_TRACKED is enable by default. Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com> --- lib/conntrack.c | 40 ++++++++++++++++++++++++++++++++++++++++ lib/conntrack.h | 1 + ovn/utilities/ovn-trace.c | 30 ++---------------------------- 3 files changed, 43 insertions(+), 28 deletions(-)