[ovs-dev] [PATCH 9/9] ofproto/trace: Support ct_mark and ct_label in --ct-next
Greg Rose
gvrose8192 at gmail.com
Fri Sep 1 21:13:57 UTC 2017
On 08/25/2017 03:51 PM, Yi-Hung Wei wrote:
> Previously, --ct-next option in ofproto/trace only supports specifying
> ct_state. This patch adds support of ct_mark and ct_label.
>
> Signed-off-by: Yi-Hung Wei <yihung.wei at gmail.com>
> ---
> lib/ct-dpif.c | 7 ++++
> lib/ct-dpif.h | 3 ++
> lib/netlink-conntrack.c | 5 +++
> lib/odp-util.c | 3 +-
> lib/odp-util.h | 1 +
> ofproto/ofproto-dpif-trace.c | 98 ++++++++++++++++++++++++++++----------------
> ofproto/ofproto-dpif-trace.h | 7 ++--
> ofproto/ofproto-unixctl.man | 16 +++++---
> tests/ofproto-dpif.at | 12 +++---
> tests/system-traffic.at | 6 +--
> 10 files changed, 103 insertions(+), 55 deletions(-)
>
> diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
> index 91388dd6681b..07c549707303 100644
> --- a/lib/ct-dpif.c
> +++ b/lib/ct-dpif.c
> @@ -144,6 +144,13 @@ ct_dpif_format_info(const struct ct_dpif_info *info, struct ds *output)
> format_flags(&s, ct_state_to_string, info->ct_state, '|');
> ds_put_format(output, "ct_state=%s", ds_cstr(&s));
> ds_destroy(&s);
> +
> + ds_put_format(output, ",ct_mark=%#"PRIx32, info->ct_mark);
> + if (info->have_label) {
> + ovs_be128 value = hton128(info->ct_label);
> + ds_put_cstr(output, ",ct_label=");
> + ds_put_hex(output, &value, sizeof value);
> + }
> }
>
> void
> diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h
> index 9122e8cd752b..9d50db920e6e 100644
> --- a/lib/ct-dpif.h
> +++ b/lib/ct-dpif.h
> @@ -175,6 +175,9 @@ struct ct_dpif_entry {
>
> struct ct_dpif_info {
> uint32_t ct_state;
> + uint32_t ct_mark;
> + bool have_label;
> + ovs_u128 ct_label;
> };
>
> struct ct_dpif_exp_entry {
> diff --git a/lib/netlink-conntrack.c b/lib/netlink-conntrack.c
> index de1e467dc61a..57da43f77135 100644
> --- a/lib/netlink-conntrack.c
> +++ b/lib/netlink-conntrack.c
> @@ -523,6 +523,11 @@ nl_ct_get_info(struct ct_dpif_tuple *tuple, uint16_t zone,
>
> nl_ct_get_ct_state(tuple, &entry, &info->ct_state);
> info->ct_state |= CS_TRACKED;
> + info->ct_mark = entry.mark;
> + if (entry.have_labels == true) {
> + info->have_label = true;
> + memcpy(&info->ct_label, &entry.labels, sizeof(info->ct_label));
> + }
>
> ofpbuf_delete(reply);
> return 0;
> diff --git a/lib/odp-util.c b/lib/odp-util.c
> index 4f1499ed4a47..c9669ca4572a 100644
> --- a/lib/odp-util.c
> +++ b/lib/odp-util.c
> @@ -88,7 +88,6 @@ static struct nlattr *generate_all_wildcard_mask(const struct attr_len_tbl tbl[]
> const struct nlattr *key);
> static void format_u128(struct ds *d, const ovs_32aligned_u128 *key,
> const ovs_32aligned_u128 *mask, bool verbose);
> -static int scan_u128(const char *s, ovs_u128 *value, ovs_u128 *mask);
>
> static int parse_odp_action(const char *s, const struct simap *port_names,
> struct ofpbuf *actions);
> @@ -3527,7 +3526,7 @@ format_u128(struct ds *ds, const ovs_32aligned_u128 *key,
> * If either the value or mask is larger than 64 bits, the string must
> * be in hexadecimal.
> */
> -static int
> +int
> scan_u128(const char *s_, ovs_u128 *value, ovs_u128 *mask)
> {
> char *s = CONST_CAST(char *, s_);
> diff --git a/lib/odp-util.h b/lib/odp-util.h
> index 27c2ab4f0111..c2633797284b 100644
> --- a/lib/odp-util.h
> +++ b/lib/odp-util.h
> @@ -344,4 +344,5 @@ void odp_put_push_eth_action(struct ofpbuf *odp_actions,
> const struct eth_addr *eth_src,
> const struct eth_addr *eth_dst);
>
> +int scan_u128(const char *s, ovs_u128 *value, ovs_u128 *mask);
> #endif /* odp-util.h */
> diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c
> index d00d41a89cab..a6844b205f56 100644
> --- a/ofproto/ofproto-dpif-trace.c
> +++ b/ofproto/ofproto-dpif-trace.c
> @@ -29,7 +29,7 @@
> static void ofproto_trace(struct ofproto_dpif *, const struct flow *,
> const struct dp_packet *packet,
> const struct ofpact[], size_t ofpacts_len,
> - struct ovs_list *next_ct_states,
> + struct ovs_list *next_cts,
> struct ds *);
> static void oftrace_node_destroy(struct oftrace_node *);
>
> @@ -124,20 +124,19 @@ oftrace_recirc_node_destroy(struct oftrace_recirc_node *node)
> }
>
> static void
> -oftrace_push_ct_state(struct ovs_list *next_ct_states, uint32_t ct_state)
> +oftrace_push_ct_info(struct ovs_list *next_cts, struct ct_dpif_info info)
> {
> - struct oftrace_next_ct_state *next_ct_state =
> - xmalloc(sizeof *next_ct_state);
> - next_ct_state->state = ct_state;
> - ovs_list_push_back(next_ct_states, &next_ct_state->node);
> + struct oftrace_next_ct *next_ct = xmalloc(sizeof *next_ct);
> + next_ct->info = info;
> + ovs_list_push_back(next_cts, &next_ct->node);
> }
>
> static void
> -oftrace_pop_ct_state(struct ovs_list *next_ct_states, uint32_t *ct_state)
> +oftrace_pop_ct_info(struct ovs_list *next_cts, struct ct_dpif_info *info)
> {
> - struct oftrace_next_ct_state *s;
> - LIST_FOR_EACH_POP (s, node, next_ct_states) {
> - *ct_state = s->state;
> + struct oftrace_next_ct *s;
> + LIST_FOR_EACH_POP (s, node, next_cts) {
> + *info = s->info;
> free(s);
> return;
> }
I can't say I like how this push and pop is done. I know I'm nitpicking but in
oftrace_push_ct_info() there is a call to ovs_list_push_back() to add the list item
but then in the oftrace_pop_ct_info() function the call is to LIST_FOR_EACH_POP which
looks technically correct but makes the code ugly and you have to use that OVS_NOT_REACHED
macro which really just makes me sad. Why isn't there a ovs_list_pop_back()?
I know that you're just working with what was in place but I think we can improve
this while we're working in this space.
Other than that the rest of this patch looks fine to me. As a whole series I think
it's looking pretty good and the additional reporting features are really nice.
Thanks for working on this!!
Regards,
- Greg
> @@ -187,10 +186,12 @@ oftrace_node_print_details(struct ds *output,
>
> static char * OVS_WARN_UNUSED_RESULT
> parse_oftrace_options(int argc, const char *argv[],
> - struct ovs_list *next_ct_states)
> + struct ovs_list *next_cts)
> {
> int k;
> + char *name, *value, *arg;
> struct ds ds = DS_EMPTY_INITIALIZER;
> + struct ct_dpif_info info;
>
> for (k = 0; k < argc; k++) {
> if (!strncmp(argv[k], "--ct-next", 9)) {
> @@ -198,14 +199,36 @@ parse_oftrace_options(int argc, const char *argv[],
> return xasprintf("Missing argument for option %s", argv[k]);
> }
>
> - uint32_t ct_state;
> - if (!parse_ct_state(argv[++k], 0, "|", &ct_state, &ds)) {
> - return ds_steal_cstr(&ds);
> - }
> - if (!validate_ct_state(ct_state, &ds)) {
> - return ds_steal_cstr(&ds);
> + memset(&info, 0, sizeof(info));
> + arg = CONST_CAST(char *, argv[++k]);
> +
> + while (ofputil_parse_key_value(&arg, &name, &value)) {
> + if (!*value) {
> + return xasprintf("Field %s missing value", name);
> + }
> +
> + if (!strcmp(name, "ct_state")) {
> + if (!parse_ct_state(value, 0, "|", &info.ct_state, &ds)) {
> + return ds_steal_cstr(&ds);
> + }
> + if (!validate_ct_state(info.ct_state, &ds)) {
> + return ds_steal_cstr(&ds);
> + }
> + } else if (!strcmp(name, "ct_mark")) {
> + char *err = str_to_u32(value, &info.ct_mark);
> + if (err) {
> + free(err);
> + return xasprintf("Failed to parse ct_mark");
> + }
> + } else if (!strcmp(name, "ct_label")) {
> + int err = scan_u128(value, &info.ct_label, NULL);
> + if (err < 0) {
> + return xasprintf("Failed to parse ct_label");
> + }
> + info.have_label = true;
> + }
> }
> - oftrace_push_ct_state(next_ct_states, ct_state);
> + oftrace_push_ct_info(next_cts, info);
> } else {
> return xasprintf("Invalid option %s", argv[k]);
> }
> @@ -227,7 +250,7 @@ static char * OVS_WARN_UNUSED_RESULT
> parse_flow_and_packet(int argc, const char *argv[],
> struct ofproto_dpif **ofprotop, struct flow *flow,
> struct dp_packet **packetp,
> - struct ovs_list *next_ct_states)
> + struct ovs_list *next_cts)
> {
> const struct dpif_backer *backer = NULL;
> const char *error = NULL;
> @@ -268,7 +291,7 @@ parse_flow_and_packet(int argc, const char *argv[],
> }
>
> m_err = parse_oftrace_options(argc - first_option, argv + first_option,
> - next_ct_states);
> + next_cts);
> if (m_err) {
> goto exit;
> }
> @@ -407,11 +430,11 @@ exit:
> }
>
> static void
> -free_ct_states(struct ovs_list *ct_states)
> +free_cts(struct ovs_list *cts)
> {
> - while (!ovs_list_is_empty(ct_states)) {
> - uint32_t state;
> - oftrace_pop_ct_state(ct_states, &state);
> + while (!ovs_list_is_empty(cts)) {
> + struct ct_dpif_info info;
> + oftrace_pop_ct_info(cts, &info);
> }
> }
>
> @@ -423,16 +446,15 @@ ofproto_unixctl_trace(struct unixctl_conn *conn, int argc, const char *argv[],
> struct dp_packet *packet;
> char *error;
> struct flow flow;
> - struct ovs_list next_ct_states = OVS_LIST_INITIALIZER(&next_ct_states);
> + struct ovs_list next_cts = OVS_LIST_INITIALIZER(&next_cts);
>
> error = parse_flow_and_packet(argc, argv, &ofproto, &flow, &packet,
> - &next_ct_states);
> + &next_cts);
> if (!error) {
> struct ds result;
>
> ds_init(&result);
> - ofproto_trace(ofproto, &flow, packet, NULL, 0, &next_ct_states,
> - &result);
> + ofproto_trace(ofproto, &flow, packet, NULL, 0, &next_cts, &result);
> unixctl_command_reply(conn, ds_cstr(&result));
> ds_destroy(&result);
> dp_packet_delete(packet);
> @@ -440,7 +462,7 @@ ofproto_unixctl_trace(struct unixctl_conn *conn, int argc, const char *argv[],
> unixctl_command_reply_error(conn, error);
> free(error);
> }
> - free_ct_states(&next_ct_states);
> + free_cts(&next_cts);
> }
>
> static void
> @@ -455,7 +477,7 @@ ofproto_unixctl_trace_actions(struct unixctl_conn *conn, int argc,
> struct ds result;
> struct match match;
> uint16_t in_port;
> - struct ovs_list next_ct_states = OVS_LIST_INITIALIZER(&next_ct_states);
> + struct ovs_list next_cts = OVS_LIST_INITIALIZER(&next_cts);
>
> /* Three kinds of error return values! */
> enum ofperr retval;
> @@ -486,7 +508,7 @@ ofproto_unixctl_trace_actions(struct unixctl_conn *conn, int argc,
> }
>
> error = parse_flow_and_packet(argc, argv, &ofproto, &match.flow, &packet,
> - &next_ct_states);
> + &next_cts);
> if (error) {
> unixctl_command_reply_error(conn, error);
> free(error);
> @@ -534,14 +556,14 @@ ofproto_unixctl_trace_actions(struct unixctl_conn *conn, int argc,
> }
>
> ofproto_trace(ofproto, &match.flow, packet,
> - ofpacts.data, ofpacts.size, &next_ct_states, &result);
> + ofpacts.data, ofpacts.size, &next_cts, &result);
> unixctl_command_reply(conn, ds_cstr(&result));
>
> exit:
> ds_destroy(&result);
> dp_packet_delete(packet);
> ofpbuf_uninit(&ofpacts);
> - free_ct_states(&next_ct_states);
> + free_cts(&next_cts);
> }
>
> static void
> @@ -633,7 +655,7 @@ static void
> ofproto_trace(struct ofproto_dpif *ofproto, const struct flow *flow,
> const struct dp_packet *packet,
> const struct ofpact ofpacts[], size_t ofpacts_len,
> - struct ovs_list *next_ct_states, struct ds *output)
> + struct ovs_list *next_cts, struct ds *output)
> {
> struct ovs_list recirc_queue = OVS_LIST_INITIALIZER(&recirc_queue);
> ofproto_trace__(ofproto, flow, packet, &recirc_queue,
> @@ -650,7 +672,7 @@ ofproto_trace(struct ofproto_dpif *ofproto, const struct flow *flow,
> struct ct_dpif_info ct_info;
> memset(&ct_info, 0, sizeof(ct_info));
>
> - if (ovs_list_is_empty(next_ct_states)) {
> + if (ovs_list_is_empty(next_cts)) {
> struct ds errs = DS_EMPTY_INITIALIZER;
> struct ct_dpif_tuple tuple;
> int err, indent_size;
> @@ -681,10 +703,14 @@ ofproto_trace(struct ofproto_dpif *ofproto, const struct flow *flow,
> ds_put_format(output, " (use --ct-next to customize)");
> ds_destroy(&errs);
> } else {
> - oftrace_pop_ct_state(next_ct_states, &ct_info.ct_state);
> + oftrace_pop_ct_info(next_cts, &ct_info);
> ct_dpif_format_info(&ct_info, output);
> }
> recirc_node->flow.ct_state = ct_info.ct_state;
> + recirc_node->flow.ct_mark = ct_info.ct_mark;
> + if (ct_info.have_label) {
> + recirc_node->flow.ct_label = ct_info.ct_label;
> + }
> }
> ds_put_char(output, '\n');
> ds_put_char_multiple(output, '=', 79);
> diff --git a/ofproto/ofproto-dpif-trace.h b/ofproto/ofproto-dpif-trace.h
> index 5e51771b19b0..1c5ef645aa6d 100644
> --- a/ofproto/ofproto-dpif-trace.h
> +++ b/ofproto/ofproto-dpif-trace.h
> @@ -28,6 +28,7 @@
> * each action (OFT_ACTION) executed in the table.
> */
>
> +#include "ct-dpif.h"
> #include "openvswitch/compiler.h"
> #include "openvswitch/list.h"
> #include "flow.h"
> @@ -72,10 +73,10 @@ struct oftrace_recirc_node {
> struct dp_packet *packet;
> };
>
> -/* A node within a next_ct_states list. */
> -struct oftrace_next_ct_state {
> +/* A node within a next_cts list. */
> +struct oftrace_next_ct {
> struct ovs_list node; /* In next_ct_states. */
> - uint32_t state;
> + struct ct_dpif_info info;
> };
>
> void ofproto_dpif_trace_init(void);
> diff --git a/ofproto/ofproto-unixctl.man b/ofproto/ofproto-unixctl.man
> index abc9481f0faf..c966beba1e57 100644
> --- a/ofproto/ofproto-unixctl.man
> +++ b/ofproto/ofproto-unixctl.man
> @@ -51,12 +51,16 @@ wildcards.) \fIbridge\fR names of the bridge through which
> .RS
> \fBofproto/trace\fR supports the following options:
> .
> -.IP "--ct-next \fIflags\fR"
> +.IP "--ct-next \fIfield\fR=\fIvalue\fR[,\fIfield\fR=\fIvalue\fR]"
> When the traced flow triggers conntrack actions, \fBofproto/trace\fR
> will automatically trace the forked packet processing pipeline with
> -user specified ct_state. This option sets the ct_state flags that the
> -conntrack module will report. The \fIflags\fR must be a vertical
> -bar separated list of the following connection tracking flags:
> +user specified conntrack fields. Currently, the supported conntrack
> +fields include ct_state, ct_mark, and ct_label.
> +.
> +.IP
> +\fBct_state\fR: Sets the ct_state flags that the conntrack module will
> +report. The \fIvalue\fR of ct_state must be a vertical bar separated
> +list of the following connection tracking flags:
> .
> .RS
> .IP \(bu
> @@ -89,7 +93,9 @@ changed.
> .
> .IP
> When --ct-next is unspecified, or when there are fewer --ct-next options than
> -ct actions, the \fIflags\fR default to trk|new.
> +ct actions, ofproto/trace queries the conntrack fields from the datapath. If
> +the conntrack fields can not be queried, then the \fIct_state\fR default to
> +trk|new, and the \fIct_mark\fR and \fIct_label\fR default to zero.
> .
> .RE
> .
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index 7353cbd6289a..32b837e5ea75 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -9834,14 +9834,14 @@ table=1,priority=1,action=drop
> dnl
> dnl Table 2
> dnl
> -table=2,priority=10,in_port=1,tcp,ct_zone=1,ct_state=+trk+new,tcp,action=ct(commit,zone=1),ct(table=3,zone=2)
> -table=2,priority=10,in_port=1,tcp,ct_zone=1,ct_state=+trk+est,tcp,action=ct(table=3,zone=2)
> +table=2,priority=10,in_port=1,tcp,ct_zone=1,ct_state=+trk+new,tcp,action=ct(commit,zone=1,exec(set_field:0x11->ct_mark)),ct(table=3,zone=2)
> +table=2,priority=10,in_port=1,tcp,ct_zone=1,ct_state=+trk+est,ct_mark=0x11,tcp,action=ct(table=3,zone=2)
> table=2,priority=1,action=drop
> dnl
> dnl Table 3
> dnl
> -table=3,priority=10,in_port=1,tcp,ct_zone=2,ct_state=+trk+new,tcp,action=ct(commit,zone=2),4
> -table=3,priority=10,in_port=1,tcp,ct_zone=2,ct_state=+trk+est,tcp,action=3
> +table=3,priority=10,in_port=1,tcp,ct_zone=2,ct_state=+trk+new,tcp,action=ct(commit,zone=2,exec(set_field:0x22->ct_mark),exec(set_field:0x112233445566778899->ct_label)),4
> +table=3,priority=10,in_port=1,tcp,ct_zone=2,ct_state=+trk+est,ct_mark=0x22,ct_label=0x112233445566778899,tcp,action=3
> table=2,priority=1,action=drop
> ])
>
> @@ -9859,10 +9859,10 @@ AT_CHECK([tail -1 stdout], [0],
>
> AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=1,tcp'], [0], [stdout])
> AT_CHECK([tail -1 stdout], [0],
> - [Datapath actions: ct(commit,zone=2),4
> + [Datapath actions: ct(commit,zone=2,mark=0x22/0xffffffff,label=0x112233445566778899),4
> ])
>
> -AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=1,tcp' --ct-next 'trk|est' --ct-next 'trk|est' ], [0], [stdout])
> +AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=1,tcp' --ct-next 'ct_state=trk|est,ct_mark=0x11' --ct-next 'ct_state=trk|est,ct_mark=0x22,ct_label=0x112233445566778899'], [0], [stdout])
> AT_CHECK([tail -1 stdout], [0],
> [Datapath actions: 3
> ])
> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> index 727f97f7a7dd..e9ebcd3e2688 100644
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -4013,8 +4013,8 @@ table=0,priority=100,arp,action=normal
> table=0,priority=10,in_port=1,tcp,action=ct(table=1,zone=1)
> table=0,priority=10,in_port=2,tcp,ct_state=-trk,action=ct(table=1,zone=1)
>
> -table=1,priority=10,in_port=1,tcp,ct_state=+trk+new,action=ct(commit,zone=1),2
> -table=1,priority=10,in_port=1,tcp,ct_state=+trk+est,action=2
> +table=1,priority=10,in_port=1,tcp,ct_state=+trk+new,action=ct(commit,zone=1,exec(set_field:0x77->ct_mark),exec(set_field:0x11223344556677889900->ct_label)),2
> +table=1,priority=10,in_port=1,tcp,ct_state=+trk+est,ct_mark=0x77,ct_label=0x11223344556677889900,action=2
> table=1,priority=10,in_port=2,tcp,ct_state=+trk+new,action=drop
> table=1,priority=10,in_port=2,tcp,ct_state=+trk+est+rpl,action=1
> ])
> @@ -4029,7 +4029,7 @@ on_exit 'rm -f payload100.bin'
> dnl Run ofproto/trace before connection is established.
> AT_CHECK([ovs-appctl ofproto/trace br0 "in_port=1,dl_type=0x800,nw_src=10.1.1.1,nw_dst=10.1.1.2,nw_proto=6,tcp_src=33333,tcp_dst=22222"], [0], [stdout])
> AT_CHECK([tail -1 stdout], [0],
> - [Datapath actions: ct(commit,zone=1),3
> + [Datapath actions: ct(commit,zone=1,mark=0x77/0xffffffff,label=0x11223344556677889900),3
> ])
> AT_CHECK([grep -c 'ct_state=new|trk' stdout], [0], [2
> ])
>
More information about the dev
mailing list