Sorry, ignore my previous email. "delim" parameter of strtok_r will take any of the characters as delimiter, so either "," or " " will work when we pass ", ".
Apparently my C has got a bit rusty ;) The whole patch series looks good to me. Acked-By: Miguel Angel Ajo <[email protected]> On Thu, Apr 20, 2017 at 1:04 PM, Miguel Angel Ajo Pelayo < [email protected]> wrote: > Just a very nit opinion/comment. nice work. > > On Tue, Apr 18, 2017 at 9:47 PM, Ben Pfaff <[email protected]> wrote: > >> Signed-off-by: Ben Pfaff <[email protected]> >> --- >> NEWS | 1 + >> ovn/utilities/ovn-trace.8.xml | 58 +++++++++++++++++++++++++ >> ovn/utilities/ovn-trace.c | 99 ++++++++++++++++++++++++++++++ >> ++++++++++++- >> 3 files changed, 156 insertions(+), 2 deletions(-) >> >> diff --git a/NEWS b/NEWS >> index eb825ac72161..f0ada38b4019 100644 >> --- a/NEWS >> +++ b/NEWS >> @@ -19,6 +19,7 @@ Post-v2.7.0 >> * Gratuitous ARP for NAT addresses on a distributed logical router. >> * Allow ovn-controller SSL configuration to be obtained from >> vswitchd >> database. >> + * ovn-trace now has basic support for tracing distributed firewalls. >> - Add the command 'ovs-appctl stp/show' (see ovs-vswitchd(8)). >> - OpenFlow: >> * Increased support for OpenFlow 1.6 (draft). >> diff --git a/ovn/utilities/ovn-trace.8.xml b/ovn/utilities/ovn-trace.8.xm >> l >> index 78914240d88c..8bb329bfbd71 100644 >> --- a/ovn/utilities/ovn-trace.8.xml >> +++ b/ovn/utilities/ovn-trace.8.xml >> @@ -307,6 +307,64 @@ >> </li> >> </ul> >> </dd> >> + >> + <dt><code>--ct=<var>flags</var></code></dt> >> + <dd> >> + <p> >> + This option sets the <code>ct_state</code> flags that a >> + <code>ct_next</code> logical action will report. The >> <var>flags</var> >> + must be a comma- or space-separated list of the following >> connection >> + tracking flags: >> + </p> >> + >> + <ul> >> + <li> >> + <code>trk</code>: Include to indicate connection tracking has >> taken >> + place. (This bit is set automatically even if not listed in >> + <var>flags</var>. >> + </li> >> + <li><code>new</code>: Include to indicate a new flow.</li> >> + <li><code>est</code>: Include to indicate an established >> flow.</li> >> + <li><code>rel</code>: Include to indicate a related flow.</li> >> + <li><code>rpl</code>: Include to indicate a reply flow.</li> >> + <li><code>inv</code>: Include to indicate a connection entry in a >> + bad state.</li> >> + <li><code>dnat</code>: Include to indicate a packet whose >> + destination IP address has been changed.</li> >> + <li><code>snat</code>: Include to indicate a packet whose source >> IP >> + address has been changed.</li> >> + </ul> >> + >> + <p> >> + The <code>ct_next</code> action is used to implement the OVN >> + distributed firewall. For testing, useful flag combinations >> include: >> + </p> >> + >> + <ul> >> + <li><code>trk,new</code>: A packet in a flow in either direction >> + through a firewall that has not yet been committed (with >> + <code>ct_commit</code>).</li> >> + <li><code>trk,est</code>: A packet in an established flow going >> out >> + through a firewall.</li> >> + <li><code>trk,rpl</code>: A packet coming in through a firewall >> in >> + reply to an established flow.</li> >> + <li><code>trk,inv</code>: An invalid packet in either >> direction.</li> >> + </ul> >> + >> + <p> >> + A packet might pass through the connection tracker twice in one >> trip >> + through OVN: once following egress from a VM as it passes outward >> + through a firewall, and once preceding ingress to a second VM as >> it >> + passes inward through a firewall. Use multiple <code>--ct</code> >> + options to specify the flags for multiple <code>ct_next</code> >> actions. >> + </p> >> + >> + <p> >> + When <code>--ct</code> is unspecified, or when there are fewer >> + <code>--ct</code> options than <code>ct_next</code> actions, the >> + <var>flags</var> default to <code>trk,est</code>. >> + </p> >> + </dd> >> </dl> >> >> <h2>Daemon Options</h2> >> diff --git a/ovn/utilities/ovn-trace.c b/ovn/utilities/ovn-trace.c >> index 66844b11ac1d..e9463f02a70f 100644 >> --- a/ovn/utilities/ovn-trace.c >> +++ b/ovn/utilities/ovn-trace.c >> @@ -69,6 +69,11 @@ static bool minimal; >> static const char *ovs; >> static struct vconn *vconn; >> >> +/* --ct: Connection tracking state to use for ct_next() actions. */ >> +static uint32_t *ct_states; >> +static size_t n_ct_states; >> +static size_t ct_state_idx; >> + >> OVS_NO_RETURN static void usage(void); >> static void parse_options(int argc, char *argv[]); >> static char *trace(const char *datapath, const char *flow); >> @@ -156,6 +161,42 @@ 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; >> > > why do we enforce the use of spaces? ", ", would it > make sense to accept just "," as delimiter and then > strip/ignore spaces for usability reasons? > > >> + 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_); >> + } >> + >> + ct_states = xrealloc(ct_states, (n_ct_states + 1) * sizeof >> *ct_states); >> + ct_states[n_ct_states++] = state; >> +} >> + >> +static void >> parse_options(int argc, char *argv[]) >> { >> enum { >> @@ -166,6 +207,7 @@ parse_options(int argc, char *argv[]) >> OPT_MINIMAL, >> OPT_ALL, >> OPT_OVS, >> + OPT_CT, >> DAEMON_OPTION_ENUMS, >> SSL_OPTION_ENUMS, >> VLOG_OPTION_ENUMS >> @@ -178,6 +220,7 @@ parse_options(int argc, char *argv[]) >> {"minimal", no_argument, NULL, OPT_MINIMAL}, >> {"all", no_argument, NULL, OPT_ALL}, >> {"ovs", optional_argument, NULL, OPT_OVS}, >> + {"ct", required_argument, NULL, OPT_CT}, >> {"help", no_argument, NULL, 'h'}, >> {"version", no_argument, NULL, 'V'}, >> DAEMON_LONG_OPTIONS, >> @@ -225,6 +268,10 @@ parse_options(int argc, char *argv[]) >> ovs = optarg ? optarg : default_ovs(); >> break; >> >> + case OPT_CT: >> + parse_ct_option(optarg); >> + break; >> + >> case 'h': >> usage(); >> >> @@ -1429,6 +1476,40 @@ execute_next(const struct ovnact_next *next, >> } >> >> static void >> +execute_ct_next(const struct ovnact_ct_next *ct_next, >> + const struct ovntrace_datapath *dp, struct flow *uflow, >> + enum ovnact_pipeline pipeline, struct ovs_list *super) >> +{ >> + /* Figure out ct_state. */ >> + uint32_t state; >> + const char *comment; >> + if (ct_state_idx < n_ct_states) { >> + state = ct_states[ct_state_idx++]; >> + comment = ""; >> + } else { >> + state = CS_ESTABLISHED | CS_TRACKED; >> + comment = " /* default (use --ct to customize) */"; >> + } >> + >> + /* Make a sub-node for attaching the next table. */ >> + struct ds s = DS_EMPTY_INITIALIZER; >> + format_flags(&s, ct_state_to_string,an state, '|'); >> >> + struct ovntrace_node *node = ovntrace_node_append( >> + super, OVNTRACE_NODE_TRANSFORMATION, "ct_next(ct_state=%s%s)", >> + ds_cstr(&s), comment); >> + ds_destroy(&s); >> + >> + /* Trace the actions in the next table. */ >> + struct flow ct_flow = *uflow; >> + ct_flow.ct_state = state; >> + trace__(dp, &ct_flow, ct_next->ltable, pipeline, &node->subs); >> + >> + /* Upon return, we will trace the actions following the ct action in >> the >> + * original table. The pipeline forked, so we're using the original >> + * flow, not ct_flow. */ >> +} >> + >> +static void >> trace_actions(const struct ovnact *ovnacts, size_t ovnacts_len, >> const struct ovntrace_datapath *dp, struct flow *uflow, >> uint8_t table_id, enum ovnact_pipeline pipeline, >> @@ -1484,13 +1565,27 @@ trace_actions(const struct ovnact *ovnacts, >> size_t ovnacts_len, >> break; >> >> case OVNACT_CT_NEXT: >> + execute_ct_next(ovnact_get_CT_NEXT(a), dp, uflow, pipeline, >> super); >> + break; >> + >> case OVNACT_CT_COMMIT: >> + /* Nothing to do. */ >> + break; >> + >> case OVNACT_CT_DNAT: >> case OVNACT_CT_SNAT: >> + ovntrace_node_append(super, OVNTRACE_NODE_ERROR, >> + "*** ct_dnat and ct_snat actions " >> + "not implemented"); >> + break; >> + >> case OVNACT_CT_LB: >> - case OVNACT_CT_CLEAR: >> ovntrace_node_append(super, OVNTRACE_NODE_ERROR, >> - "*** ct_* actions not implemented"); >> + "*** ct_lb action not implemented"); >> + break; >> + >> + case OVNACT_CT_CLEAR: >> + flow_clear_conntrack(uflow); >> break; >> >> case OVNACT_CLONE: >> -- >> 2.10.2 >> >> _______________________________________________ >> dev mailing list >> [email protected] >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
