diff mbox

[ovs-dev,3/3] ofproto/trace: Add --ct-next option to ofproto/trace

Message ID 1498068412-89824-4-git-send-email-yihung.wei@gmail.com
State Changes Requested
Headers show

Commit Message

Yi-Hung Wei June 21, 2017, 6:06 p.m. UTC 
Previous patch enables ofproto/trace to automatically trace a flow
that involves multiple recirculation on conntrack. However, it always
sets the ct_state to trk|est when it processes recirculated conntrack flows.
With this patch, users can customize the expected next ct_state in the
aforementioned use case.

Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>
---
 NEWS                         |   2 +
 ofproto/ofproto-dpif-trace.c | 111 ++++++++++++++++++++++++++++++++++++-------
 ofproto/ofproto-dpif-trace.h |   6 +++
 ofproto/ofproto-unixctl.man  |  54 +++++++++++++++++++--
 tests/completion.at          |   8 ++--
 tests/ofproto-dpif.at        |  33 +++++++++----
 6 files changed, 182 insertions(+), 32 deletions(-)

Comments

Joe Stringer June 21, 2017, 8:02 p.m. UTC | #1 
On 21 June 2017 at 11:06, Yi-Hung Wei <yihung.wei@gmail.com> wrote:
> Previous patch enables ofproto/trace to automatically trace a flow
> that involves multiple recirculation on conntrack. However, it always
> sets the ct_state to trk|est when it processes recirculated conntrack flows.
> With this patch, users can customize the expected next ct_state in the
> aforementioned use case.
>
> Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>
> ---
>  NEWS                         |   2 +
>  ofproto/ofproto-dpif-trace.c | 111 ++++++++++++++++++++++++++++++++++++-------
>  ofproto/ofproto-dpif-trace.h |   6 +++
>  ofproto/ofproto-unixctl.man  |  54 +++++++++++++++++++--
>  tests/completion.at          |   8 ++--
>  tests/ofproto-dpif.at        |  33 +++++++++----
>  6 files changed, 182 insertions(+), 32 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 4ea7e628e1fc..1824ec9de744 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -58,6 +58,8 @@ Post-v2.7.0
>     - Fedora Packaging:
>       * OVN services are no longer restarted automatically after upgrade.
>     - Add --cleanup option to command 'ovs-appctl exit' (see ovs-vswitchd(8)).
> +   - Add --ct-next option to command 'ovs-appctl ofprot/trace' (see

ofproto/trace.

> +     ovs-vswitchd(8)).
>     - L3 tunneling:
>       * Add "layer3" options for tunnel ports that support non-Ethernet (L3)
>         payload (GRE, VXLAN-GPE).
> diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c
> index d8cdd92493cf..e75c62d464eb 100644
> --- a/ofproto/ofproto-dpif-trace.c
> +++ b/ofproto/ofproto-dpif-trace.c
> @@ -18,6 +18,7 @@
>
>  #include "ofproto-dpif-trace.h"
>
> +#include "conntrack.h"
>  #include "dpif.h"
>  #include "ofproto-dpif-xlate.h"
>  #include "openvswitch/ofp-parse.h"
> @@ -26,7 +27,7 @@
>  static void ofproto_trace(struct ofproto_dpif *, struct flow *,
>                            const struct dp_packet *packet,
>                            const struct ofpact[], size_t ofpacts_len,
> -                          struct ds *);
> +                          struct ds *, struct ovs_list *next_ct_states);
>  static void oftrace_node_destroy(struct oftrace_node *);
>
>  /* Creates a new oftrace_node, populates it with the given 'type' and a copy of
> @@ -119,6 +120,17 @@ oftrace_recirc_node_destroy(struct oftrace_recirc_node *node)
>      free(node);
>  }
>
> +static uint32_t
> +oftrace_next_ct_state(struct ovs_list *next_ct_states)
> +{
> +    struct oftrace_next_ct_state *next_ct_state;
> +    ASSIGN_CONTAINER(next_ct_state, ovs_list_pop_front(next_ct_states), node);
> +    uint32_t state = next_ct_state->state;
> +    free(next_ct_state);

It wouldn't hurt to expand this a bit for improved readability,
declaring the variables first, then getting the ovs_list_pop_front()
pointer, then assigning the container, getting state and freeing.

> +
> +    return state;
> +}
> +
>  static void
>  oftrace_node_print_details(struct ds *output,
>                             const struct ovs_list *nodes, int level)
> @@ -160,18 +172,47 @@ 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)
> +{
> +    char *error = NULL;
> +    int k;
> +
> +    for (k = 0; k < argc; k++) {
> +        if (!strncmp(argv[k], "--ct-next", 9)) {
> +            if (k + 1 > argc) {
> +                error = xasprintf("Missing argument for option %s", argv[k]);
> +                return error;

I guess these could just be return xasprintf(...) ?

> +            }
> +
> +            uint32_t ct_state = parse_ct_state(argv[++k], 0);

I'm a bit concerned that parse_oftrace_options() may be called from
ovs-vswitchd, and parse_ct_state() may, on parse failure, exit in a
fatal manner. If this is the case, perhaps we should refactor
parse_ct_state() into a version that should exit fatally for
commandline execution, and the version run from the main vswitchd
thread.

> +            struct oftrace_next_ct_state *next_state =
> +                xmalloc(sizeof *next_state);
> +            next_state->state = ct_state;
> +            ovs_list_push_back(next_ct_states, &next_state->node);

You might consider refactoring the above into a function like
oftrace_push_ct_state(next_ct_states, ct_state). Then the malloc/free
operations would be matched between this new function and the above
oftrace_next_ct_state().. which you may also consider renaming
something like oftrace_pop_ct_state(...).

> +        } else {
> +            error = xasprintf("Invalid option %s", argv[k]);
> +            return error;

Same as above xasprintf comment.

> +        }
> +    }
> +
> +    return error;
> +}
> +
>  /* Parses the 'argc' elements of 'argv', ignoring argv[0].  The following
>   * forms are supported:
>   *
> - *     - [dpname] odp_flow [-generate | packet]
> - *     - bridge br_flow [-generate | packet]
> + *     - [dpname] odp_flow [OPTIONS] [-generate | packet]
> + *     - bridge br_flow [OPTIONS] [-generate | packet]
>   *
>   * On success, initializes '*ofprotop' and 'flow' and returns NULL.  On failure
>   * returns a nonnull malloced error message. */
>  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 dp_packet **packetp,
> +                      struct ovs_list *next_ct_states)
>  {
>      const struct dpif_backer *backer = NULL;
>      const char *error = NULL;
> @@ -180,6 +221,7 @@ parse_flow_and_packet(int argc, const char *argv[],
>      struct dp_packet *packet;
>      struct ofpbuf odp_key;
>      struct ofpbuf odp_mask;
> +    int first_option;
>
>      ofpbuf_init(&odp_key, 0);
>      ofpbuf_init(&odp_mask, 0);
> @@ -199,6 +241,25 @@ parse_flow_and_packet(int argc, const char *argv[],
>          error = NULL;
>      }
>
> +    /* Parse options. */
> +    if (argc >= 4) {
> +        if (!strncmp(argv[2], "--", 2)) {
> +            first_option = 2;
> +        } else if (!strncmp(argv[3], "--", 2)) {
> +            first_option = 3;
> +        } else {
> +            error = "Syntax error";

Could you expand on this a bit? It can be confusing for users to get
stonewalled with "Syntax error" and not know why the syntax was wrong.
(I realise there's other examples later in the function where this
error is used, it'd be nice to improve that as well).

Thanks!
Darrell Ball June 21, 2017, 8:37 p.m. UTC | #2 
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:

    Previous patch enables ofproto/trace to automatically trace a flow
    that involves multiple recirculation on conntrack. However, it always
    sets the ct_state to trk|est when it processes recirculated conntrack flows.
    With this patch, users can customize the expected next ct_state in the
    aforementioned use case.
    
    Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>

    ---
     NEWS                         |   2 +
     ofproto/ofproto-dpif-trace.c | 111 ++++++++++++++++++++++++++++++++++++-------
     ofproto/ofproto-dpif-trace.h |   6 +++
     ofproto/ofproto-unixctl.man  |  54 +++++++++++++++++++--
     tests/completion.at          |   8 ++--
     tests/ofproto-dpif.at        |  33 +++++++++----
     6 files changed, 182 insertions(+), 32 deletions(-)
    
    diff --git a/NEWS b/NEWS
    index 4ea7e628e1fc..1824ec9de744 100644
    --- a/NEWS
    +++ b/NEWS
    @@ -58,6 +58,8 @@ Post-v2.7.0
        - Fedora Packaging:
          * OVN services are no longer restarted automatically after upgrade.
        - Add --cleanup option to command 'ovs-appctl exit' (see ovs-vswitchd(8)).
    +   - Add --ct-next option to command 'ovs-appctl ofprot/trace' (see
    +     ovs-vswitchd(8)).
        - L3 tunneling:
          * Add "layer3" options for tunnel ports that support non-Ethernet (L3)
            payload (GRE, VXLAN-GPE).
    diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c
    index d8cdd92493cf..e75c62d464eb 100644
    --- a/ofproto/ofproto-dpif-trace.c
    +++ b/ofproto/ofproto-dpif-trace.c
    @@ -18,6 +18,7 @@
     
     #include "ofproto-dpif-trace.h"
     
    +#include "conntrack.h"
     #include "dpif.h"
     #include "ofproto-dpif-xlate.h"
     #include "openvswitch/ofp-parse.h"
    @@ -26,7 +27,7 @@
     static void ofproto_trace(struct ofproto_dpif *, struct flow *,
                               const struct dp_packet *packet,
                               const struct ofpact[], size_t ofpacts_len,
    -                          struct ds *);
    +                          struct ds *, struct ovs_list *next_ct_states);
     static void oftrace_node_destroy(struct oftrace_node *);
     
     /* Creates a new oftrace_node, populates it with the given 'type' and a copy of
    @@ -119,6 +120,17 @@ oftrace_recirc_node_destroy(struct oftrace_recirc_node *node)
         free(node);
     }
     
    +static uint32_t
    +oftrace_next_ct_state(struct ovs_list *next_ct_states)
    +{
    +    struct oftrace_next_ct_state *next_ct_state;
    +    ASSIGN_CONTAINER(next_ct_state, ovs_list_pop_front(next_ct_states), node);
    +    uint32_t state = next_ct_state->state;
    +    free(next_ct_state);
    +
    +    return state;
    +}
    +
     static void
     oftrace_node_print_details(struct ds *output,
                                const struct ovs_list *nodes, int level)
    @@ -160,18 +172,47 @@ 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)
    +{
    +    char *error = NULL;
    +    int k;
    +
    +    for (k = 0; k < argc; k++) {
    +        if (!strncmp(argv[k], "--ct-next", 9)) {
    +            if (k + 1 > argc) {
    +                error = xasprintf("Missing argument for option %s", argv[k]);
    +                return error;
    +            }
    +
    +            uint32_t ct_state = parse_ct_state(argv[++k], 0);
    +            struct oftrace_next_ct_state *next_state =
    +                xmalloc(sizeof *next_state);
    +            next_state->state = ct_state;
    +            ovs_list_push_back(next_ct_states, &next_state->node);
    +        } else {
    +            error = xasprintf("Invalid option %s", argv[k]);
    +            return error;
    +        }
    +    }
    +
    +    return error;
    +}
    +
     /* Parses the 'argc' elements of 'argv', ignoring argv[0].  The following
      * forms are supported:
      *
    - *     - [dpname] odp_flow [-generate | packet]
    - *     - bridge br_flow [-generate | packet]
    + *     - [dpname] odp_flow [OPTIONS] [-generate | packet]
    + *     - bridge br_flow [OPTIONS] [-generate | packet]
      *
      * On success, initializes '*ofprotop' and 'flow' and returns NULL.  On failure
      * returns a nonnull malloced error message. */
     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 dp_packet **packetp,
    +                      struct ovs_list *next_ct_states)
     {
         const struct dpif_backer *backer = NULL;
         const char *error = NULL;
    @@ -180,6 +221,7 @@ parse_flow_and_packet(int argc, const char *argv[],
         struct dp_packet *packet;
         struct ofpbuf odp_key;
         struct ofpbuf odp_mask;
    +    int first_option;
     
         ofpbuf_init(&odp_key, 0);
         ofpbuf_init(&odp_mask, 0);
    @@ -199,6 +241,25 @@ parse_flow_and_packet(int argc, const char *argv[],
             error = NULL;
         }
     
    +    /* Parse options. */
    +    if (argc >= 4) {
    +        if (!strncmp(argv[2], "--", 2)) {
    +            first_option = 2;
    +        } else if (!strncmp(argv[3], "--", 2)) {
    +            first_option = 3;
    +        } else {
    +            error = "Syntax error";
    +            goto exit;
    +        }
    +
    +        m_err = parse_oftrace_options(argc - first_option, argv + first_option,
    +                                      next_ct_states);
    +        if (m_err) {
    +            goto exit;
    +        }
    +        argc = first_option;
    +    }
    +
         /* odp_flow can have its in_port specified as a name instead of port no.
          * We do not yet know whether a given flow is a odp_flow or a br_flow.
          * But, to know whether a flow is odp_flow through odp_flow_from_string(),
    @@ -338,13 +399,16 @@ 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);
     
    -    error = parse_flow_and_packet(argc, argv, &ofproto, &flow, &packet);
    +    error = parse_flow_and_packet(argc, argv, &ofproto, &flow, &packet,
    +                                  &next_ct_states);
         if (!error) {
             struct ds result;
     
             ds_init(&result);
    -        ofproto_trace(ofproto, &flow, packet, NULL, 0, &result);
    +        ofproto_trace(ofproto, &flow, packet, NULL, 0, &result,
    +                      &next_ct_states);
             unixctl_command_reply(conn, ds_cstr(&result));
             ds_destroy(&result);
             dp_packet_delete(packet);
    @@ -366,6 +430,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);
     
         /* Three kinds of error return values! */
         enum ofperr retval;
    @@ -395,7 +460,8 @@ ofproto_unixctl_trace_actions(struct unixctl_conn *conn, int argc,
             enforce_consistency = false;
         }
     
    -    error = parse_flow_and_packet(argc, argv, &ofproto, &match.flow, &packet);
    +    error = parse_flow_and_packet(argc, argv, &ofproto, &match.flow, &packet,
    +                                  &next_ct_states);
         if (error) {
             unixctl_command_reply_error(conn, error);
             free(error);
    @@ -443,7 +509,7 @@ ofproto_unixctl_trace_actions(struct unixctl_conn *conn, int argc,
         }
     
         ofproto_trace(ofproto, &match.flow, packet,
    -                  ofpacts.data, ofpacts.size, &result);
    +                  ofpacts.data, ofpacts.size, &result, &next_ct_states);
         unixctl_command_reply(conn, ds_cstr(&result));
     
     exit:
    @@ -541,7 +607,7 @@ static void
     ofproto_trace(struct ofproto_dpif *ofproto, struct flow *flow,
                   const struct dp_packet *packet,
                   const struct ofpact ofpacts[], size_t ofpacts_len,
    -              struct ds *output)
    +              struct ds *output, struct ovs_list *next_ct_states)
     {
         struct ovs_list recirc_queue = OVS_LIST_INITIALIZER(&recirc_queue);
         oftrace_add_recirc_node(&recirc_queue, OFT_INIT, flow, flow->recirc_id);
    @@ -553,9 +619,21 @@ ofproto_trace(struct ofproto_dpif *ofproto, struct flow *flow,
             ASSIGN_CONTAINER(recirc_node, node, node);
     
             if (recirc_node->type == OFT_CONNTRACK) {
    -            recirc_node->flow->ct_state = CS_ESTABLISHED | CS_TRACKED;
    -            ds_put_cstr(output, "\n\nResume conntrack prcessing with "
    -                                "default ct_state=trk|est.\n");
    +            if (ovs_list_is_empty(next_ct_states)) {
    +                recirc_node->flow->ct_state = CS_ESTABLISHED | CS_TRACKED;
    +                ds_put_cstr(output, "\n\nResume conntrack prcessing with "
    +                                    "default ct_state=trk|est. Use --ct-next "
    +                                    "to customize\n");
    +            } else {
    +                recirc_node->flow->ct_state =
    +                    oftrace_next_ct_state(next_ct_states);
    +                struct ds s = DS_EMPTY_INITIALIZER;
    +                format_flags(&s, ct_state_to_string,
    +                             recirc_node->flow->ct_state, '|');
    +                ds_put_format(output, "\n\nResume conntrack prcessing with "
    +                                      "ct_state=%s\n", ds_cstr(&s));
    +                ds_destroy(&s);
    +            }
                 ds_put_char_multiple(output, '-', 79);
                 ds_put_char(output, '\n');
             }
    @@ -576,10 +654,11 @@ ofproto_dpif_trace_init(void)
     
         unixctl_command_register(
             "ofproto/trace",
    -        "{[dp_name] odp_flow | bridge br_flow} [-generate|packet]",
    -        1, 3, ofproto_unixctl_trace, NULL);
    +        "{[dp_name] odp_flow | bridge br_flow} [OPTIONS...] "
    +        "[-generate|packet]", 1, INT_MAX, ofproto_unixctl_trace, NULL);
         unixctl_command_register(
             "ofproto/trace-packet-out",
    -        "[-consistent] {[dp_name] odp_flow | bridge br_flow} [-generate|packet] actions",
    -        2, 6, ofproto_unixctl_trace_actions, NULL);
    +        "[-consistent] {[dp_name] odp_flow | bridge br_flow} [OPTIONS...] "
    +        "[-generate|packet] actions",
    +        2, INT_MAX, ofproto_unixctl_trace_actions, NULL);
     }
    diff --git a/ofproto/ofproto-dpif-trace.h b/ofproto/ofproto-dpif-trace.h
    index f7299fd42848..9bb080534a7a 100644
    --- a/ofproto/ofproto-dpif-trace.h
    +++ b/ofproto/ofproto-dpif-trace.h
    @@ -73,6 +73,12 @@ struct oftrace_recirc_node {
         struct flow *flow;
     };
     
    +/* A node within a next_ct_state list. */
    +struct oftrace_next_ct_state {
    +    struct ovs_list node;       /* In next_ct_states. */
    +    uint32_t state;
    +};
    +
     void ofproto_dpif_trace_init(void);
     
     struct oftrace_node *oftrace_report(struct ovs_list *, enum oftrace_node_type,
    diff --git a/ofproto/ofproto-unixctl.man b/ofproto/ofproto-unixctl.man
    index 423837351910..e6c37894e980 100644
    --- a/ofproto/ofproto-unixctl.man
    +++ b/ofproto/ofproto-unixctl.man
    @@ -6,10 +6,10 @@ These commands manage the core OpenFlow switch implementation (called
     Lists the names of the running ofproto instances.  These are the names
     that may be used on \fBofproto/trace\fR.
     .
    -.IP "\fBofproto/trace\fR [\fIdpname\fR] \fIodp_flow\fR [\fB\-generate \fR| \fIpacket\fR]"
    -.IQ "\fBofproto/trace\fR \fIbridge\fR \fIbr_flow\fR [\fB\-generate \fR| \fIpacket\fR]"
    -.IQ "\fBofproto/trace\-packet\-out\fR [\fB\-consistent\fR] [\fIdpname\fR] \fIodp_flow\fR [\fB\-generate \fR| \fIpacket\fR] \fIactions\fR"
    -.IQ "\fBofproto/trace\-packet\-out\fR [\fB\-consistent\fR] \fIbridge\fR \fIbr_flow\fR [\fB\-generate \fR| \fIpacket\fR] \fIactions\fR"
    +.IP "\fBofproto/trace\fR [\fIdpname\fR] \fIodp_flow\fR [\fIOPTIONS\fR] [\fB\-generate \fR| \fIpacket\fR]"
    +.IQ "\fBofproto/trace\fR \fIbridge\fR \fIbr_flow\fR [\fIOPTIONS\fR] [\fB\-generate \fR| \fIpacket\fR]"
    +.IQ "\fBofproto/trace\-packet\-out\fR [\fB\-consistent\fR] [\fIdpname\fR] \fIodp_flow\fR [\fIOPTIONS\fR] [\fB\-generate \fR| \fIpacket\fR] \fIactions\fR"
    +.IQ "\fBofproto/trace\-packet\-out\fR [\fB\-consistent\fR] \fIbridge\fR \fIbr_flow\fR [\fIOPTIONS\fR] [\fB\-generate \fR| \fIpacket\fR] \fIactions\fR"
     Traces the path of an imaginary packet through \fIswitch\fR and
     reports the path that it took.  The initial treatment of the packet
     varies based on the command:
    @@ -48,6 +48,52 @@ wildcards.)  \fIbridge\fR names of the bridge through which
     .RE
     .
     .IP
    +.RS
    +\fBofproto/trace\fR supports the following options:
    +.
    +.IP "--ct-next \fIflags\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 comma- or
    +space-separated list of the following connection tracking flags:
    +.
    +.RS
    +.IP \(bu
    +\fBtrk\fR: Include to indicate connection tracking has taken place.
    +.
    +.IP \(bu
    +\fBnew\fR: Include to indicate a new flow.
    +.
    +.IP \(bu
    +\fBest\fR: Include to indicate an established flow.
    +.
    +.IP \(bu
    +\fBrel\fR: Include to indicate a related flow.
    +.
    +.IP \(bu
    +\fBrpl\fR: Include to indicate a reply flow.
    +.
    +.IP \(bu
    +\fBinv\fR: Include to indicate a connection entry in a bad state.
    +.
    +.IP \(bu
    +\fBdnat\fR: Include to indicate a packet whose destination IP address has been
    +changed.
    +.
    +.IP \(bu
    +\fBsnat\fR: Include to indicate a packet whose source IP address has been
    +changed.
    +.
    +.RE
    +.
    +.IP
    +When --ct-next is unspecified, or when there are fewer --ct-next options that
    +ct actions, the \fIflags\fR default to trk,est.

1/ I think you should drop ‘est’ as part of the default.
    Rarely, is the ‘est’ state being debugged.
    trk by itself will catch more, in case the user does not specify options, is not aware of
    the options or maybe does not even understand them.

2/ I think it is very surprising that if fewer –ct-next options are provided than
     ct actions, that the remaining ct actions are traced as the default. I would
     expect the last one specified to be used for any remaining ct actions, if any.
     I think the typical usage would be the user providing one –ct-next and wanting 
     it applying to any/all ct actions.

    +.
    +.RE
    +.
    +.IP
     Most commonly, one specifies only a flow, using one of the forms
     above, but sometimes one might need to specify an actual packet
     instead of just a flow:
    diff --git a/tests/completion.at b/tests/completion.at
    index e28c75239227..00e3a46b8bfa 100644
    --- a/tests/completion.at
    +++ b/tests/completion.at
    @@ -171,7 +171,7 @@ AT_CLEANUP
     
     
     # complex completion check - ofproto/trace
    -# ofproto/trace {[dp_name] odp_flow | bridge br_flow} [-generate|packet]
    +# ofproto/trace {[dp_name] odp_flow | bridge br_flow} [OPTIONS] [-generate|packet]
     # test expansion on 'dp|dp_name' and 'bridge'
     AT_SETUP([appctl-bashcomp - complex completion check 3])
     AT_SKIP_IF([test -z ${BASH_VERSION+x}])
    @@ -209,7 +209,8 @@ AT_CHECK_UNQUOTED([GET_AVAIL(${INPUT})], [0])
     # set odp_flow to some random string, should go to the next level.
     INPUT="$(bash ovs-appctl-bashcomp.bash debug ovs-appctl ofproto/trace ovs-dummy "in_port(123),mac(),ip,tcp" TAB 2>&1)"
     MATCH="$(GET_COMP_STR([-generate], [-generate])
    -GET_COMP_STR([packet], []))"
    +GET_COMP_STR([packet], [])
    +GET_COMP_STR([OPTIONS...], []))"
     AT_CHECK_UNQUOTED([GET_EXPAN(${INPUT})],
     [0], [dnl
     ${MATCH}
    @@ -242,7 +243,8 @@ AT_CHECK_UNQUOTED([GET_AVAIL(${INPUT})], [0])
     # set argument to some random string, should go to the odp_flow path.
     INPUT="$(bash ovs-appctl-bashcomp.bash debug ovs-appctl ofproto/trace "in_port(123),mac(),ip,tcp" TAB 2>&1)"
     MATCH="$(GET_COMP_STR([-generate], [-generate])
    -GET_COMP_STR([packet], []))"
    +GET_COMP_STR([packet], [])
    +GET_COMP_STR([OPTIONS...], []))"
     AT_CHECK_UNQUOTED([GET_EXPAN(${INPUT})],
     [0], [dnl
     ${MATCH}
    diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
    index c51c689b9f46..142322afd777 100644
    --- a/tests/ofproto-dpif.at
    +++ b/tests/ofproto-dpif.at
    @@ -5208,14 +5208,6 @@ Trailing garbage in packet data
     ovs-appctl: ovs-vswitchd: server returned an error
     ])
     
    -# Test incorrect command: ofproto/trace with 4 arguments
    -AT_CHECK([ovs-appctl ofproto/trace \
    -  arg1, arg2, arg3, arg4], [2], [stdout],[stderr])
    -AT_CHECK([tail -2 stderr], [0], [dnl
    -"ofproto/trace" command takes at most 3 arguments
    -ovs-appctl: ovs-vswitchd: server returned an error
    -])
    -
     # Test incorrect command: ofproto/trace with 0 argument
     AT_CHECK([ovs-appctl ofproto/trace ], [2], [stdout],[stderr])
     AT_CHECK([tail -2 stderr], [0], [dnl
    @@ -9713,13 +9705,14 @@ AT_CLEANUP
     AT_SETUP([ofproto-dpif - conntrack - ofproto/trace])
     OVS_VSWITCHD_START
     
    -add_of_ports br0 1 2
    +add_of_ports br0 1 2 3 4
     
     AT_DATA([flows.txt], [dnl
     dnl Table 0
     dnl
     table=0,priority=100,arp,action=normal
     table=0,priority=10,udp,action=ct(table=1,zone=0)
    +table=0,priority=10,tcp,action=ct(table=2,zone=1)
     table=0,priority=1,action=drop
     dnl
     dnl Table 1
    @@ -9728,6 +9721,18 @@ table=1,priority=10,in_port=1,ct_state=+trk+new,udp,action=ct(commit,zone=0),2
     table=1,priority=10,in_port=1,ct_state=+trk+est,udp,action=2
     table=1,priority=10,in_port=2,ct_state=+trk+est,udp,action=1
     table=1,priority=1,action=drop
    +dnl
    +dnl Table 2
    +dnl
    +table=2,priority=10,in_port=1,tcp,ct_state=+trk+new,tcp,action=ct(commit,zone=1),ct(table=3,zone=2)
    +table=2,priority=10,in_port=1,tcp,ct_state=+trk+est,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_state=+trk+new,tcp,action=ct(commit,zone=2),4
    +table=3,priority=10,in_port=1,tcp,ct_state=+trk+est,tcp,action=3
    +table=2,priority=1,action=drop
     ])
     
     AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
    @@ -9742,6 +9747,16 @@ AT_CHECK([tail -1 stdout], [0],
       [Datapath actions: 2
     ])
     
    +AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=1,tcp'], [0], [stdout])
    +AT_CHECK([tail -1 stdout], [0],
    +  [Datapath actions: 3
    +])
    +
    +AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=1,tcp' --ct-next 'trk,new' --ct-next 'trk,new' ], [0], [stdout])
    +AT_CHECK([tail -1 stdout], [0],
    +  [Datapath actions: ct(commit,zone=2),4
    +])
    +
     OVS_VSWITCHD_STOP
     AT_CLEANUP
     
    -- 
    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=JmH2jZDW_1WD9HRXGFuUOs7-4mI6AXUopn7gzbNPXo8&s=Qwjtc88CeYT74ugg8YfxZXQAjxYM_7fELmzeLlVWyVY&e=
Yi-Hung Wei June 26, 2017, 5:18 p.m. UTC | #3 
On Wed, Jun 21, 2017 at 1:37 PM, 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:
>
>     Previous patch enables ofproto/trace to automatically trace a flow
>     that involves multiple recirculation on conntrack. However, it always
>     sets the ct_state to trk|est when it processes recirculated conntrack flows.
>     With this patch, users can customize the expected next ct_state in the
>     aforementioned use case.
>
>     Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>
>     ---
>      NEWS                         |   2 +
>      ofproto/ofproto-dpif-trace.c | 111 ++++++++++++++++++++++++++++++++++++-------
>      ofproto/ofproto-dpif-trace.h |   6 +++
>      ofproto/ofproto-unixctl.man  |  54 +++++++++++++++++++--
>      tests/completion.at          |   8 ++--
>      tests/ofproto-dpif.at        |  33 +++++++++----
>      6 files changed, 182 insertions(+), 32 deletions(-)
>
>     diff --git a/NEWS b/NEWS
>     index 4ea7e628e1fc..1824ec9de744 100644
>     --- a/NEWS
>     +++ b/NEWS
>     @@ -58,6 +58,8 @@ Post-v2.7.0
>         - Fedora Packaging:
>           * OVN services are no longer restarted automatically after upgrade.
>         - Add --cleanup option to command 'ovs-appctl exit' (see ovs-vswitchd(8)).
>     +   - Add --ct-next option to command 'ovs-appctl ofprot/trace' (see
>     +     ovs-vswitchd(8)).
>         - L3 tunneling:
>           * Add "layer3" options for tunnel ports that support non-Ethernet (L3)
>             payload (GRE, VXLAN-GPE).
>     diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c
>     index d8cdd92493cf..e75c62d464eb 100644
>     --- a/ofproto/ofproto-dpif-trace.c
>     +++ b/ofproto/ofproto-dpif-trace.c
>     @@ -18,6 +18,7 @@
>
>      #include "ofproto-dpif-trace.h"
>
>     +#include "conntrack.h"
>      #include "dpif.h"
>      #include "ofproto-dpif-xlate.h"
>      #include "openvswitch/ofp-parse.h"
>     @@ -26,7 +27,7 @@
>      static void ofproto_trace(struct ofproto_dpif *, struct flow *,
>                                const struct dp_packet *packet,
>                                const struct ofpact[], size_t ofpacts_len,
>     -                          struct ds *);
>     +                          struct ds *, struct ovs_list *next_ct_states);
>      static void oftrace_node_destroy(struct oftrace_node *);
>
>      /* Creates a new oftrace_node, populates it with the given 'type' and a copy of
>     @@ -119,6 +120,17 @@ oftrace_recirc_node_destroy(struct oftrace_recirc_node *node)
>          free(node);
>      }
>
>     +static uint32_t
>     +oftrace_next_ct_state(struct ovs_list *next_ct_states)
>     +{
>     +    struct oftrace_next_ct_state *next_ct_state;
>     +    ASSIGN_CONTAINER(next_ct_state, ovs_list_pop_front(next_ct_states), node);
>     +    uint32_t state = next_ct_state->state;
>     +    free(next_ct_state);
>     +
>     +    return state;
>     +}
>     +
>      static void
>      oftrace_node_print_details(struct ds *output,
>                                 const struct ovs_list *nodes, int level)
>     @@ -160,18 +172,47 @@ 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)
>     +{
>     +    char *error = NULL;
>     +    int k;
>     +
>     +    for (k = 0; k < argc; k++) {
>     +        if (!strncmp(argv[k], "--ct-next", 9)) {
>     +            if (k + 1 > argc) {
>     +                error = xasprintf("Missing argument for option %s", argv[k]);
>     +                return error;
>     +            }
>     +
>     +            uint32_t ct_state = parse_ct_state(argv[++k], 0);
>     +            struct oftrace_next_ct_state *next_state =
>     +                xmalloc(sizeof *next_state);
>     +            next_state->state = ct_state;
>     +            ovs_list_push_back(next_ct_states, &next_state->node);
>     +        } else {
>     +            error = xasprintf("Invalid option %s", argv[k]);
>     +            return error;
>     +        }
>     +    }
>     +
>     +    return error;
>     +}
>     +
>      /* Parses the 'argc' elements of 'argv', ignoring argv[0].  The following
>       * forms are supported:
>       *
>     - *     - [dpname] odp_flow [-generate | packet]
>     - *     - bridge br_flow [-generate | packet]
>     + *     - [dpname] odp_flow [OPTIONS] [-generate | packet]
>     + *     - bridge br_flow [OPTIONS] [-generate | packet]
>       *
>       * On success, initializes '*ofprotop' and 'flow' and returns NULL.  On failure
>       * returns a nonnull malloced error message. */
>      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 dp_packet **packetp,
>     +                      struct ovs_list *next_ct_states)
>      {
>          const struct dpif_backer *backer = NULL;
>          const char *error = NULL;
>     @@ -180,6 +221,7 @@ parse_flow_and_packet(int argc, const char *argv[],
>          struct dp_packet *packet;
>          struct ofpbuf odp_key;
>          struct ofpbuf odp_mask;
>     +    int first_option;
>
>          ofpbuf_init(&odp_key, 0);
>          ofpbuf_init(&odp_mask, 0);
>     @@ -199,6 +241,25 @@ parse_flow_and_packet(int argc, const char *argv[],
>              error = NULL;
>          }
>
>     +    /* Parse options. */
>     +    if (argc >= 4) {
>     +        if (!strncmp(argv[2], "--", 2)) {
>     +            first_option = 2;
>     +        } else if (!strncmp(argv[3], "--", 2)) {
>     +            first_option = 3;
>     +        } else {
>     +            error = "Syntax error";
>     +            goto exit;
>     +        }
>     +
>     +        m_err = parse_oftrace_options(argc - first_option, argv + first_option,
>     +                                      next_ct_states);
>     +        if (m_err) {
>     +            goto exit;
>     +        }
>     +        argc = first_option;
>     +    }
>     +
>          /* odp_flow can have its in_port specified as a name instead of port no.
>           * We do not yet know whether a given flow is a odp_flow or a br_flow.
>           * But, to know whether a flow is odp_flow through odp_flow_from_string(),
>     @@ -338,13 +399,16 @@ 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);
>
>     -    error = parse_flow_and_packet(argc, argv, &ofproto, &flow, &packet);
>     +    error = parse_flow_and_packet(argc, argv, &ofproto, &flow, &packet,
>     +                                  &next_ct_states);
>          if (!error) {
>              struct ds result;
>
>              ds_init(&result);
>     -        ofproto_trace(ofproto, &flow, packet, NULL, 0, &result);
>     +        ofproto_trace(ofproto, &flow, packet, NULL, 0, &result,
>     +                      &next_ct_states);
>              unixctl_command_reply(conn, ds_cstr(&result));
>              ds_destroy(&result);
>              dp_packet_delete(packet);
>     @@ -366,6 +430,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);
>
>          /* Three kinds of error return values! */
>          enum ofperr retval;
>     @@ -395,7 +460,8 @@ ofproto_unixctl_trace_actions(struct unixctl_conn *conn, int argc,
>              enforce_consistency = false;
>          }
>
>     -    error = parse_flow_and_packet(argc, argv, &ofproto, &match.flow, &packet);
>     +    error = parse_flow_and_packet(argc, argv, &ofproto, &match.flow, &packet,
>     +                                  &next_ct_states);
>          if (error) {
>              unixctl_command_reply_error(conn, error);
>              free(error);
>     @@ -443,7 +509,7 @@ ofproto_unixctl_trace_actions(struct unixctl_conn *conn, int argc,
>          }
>
>          ofproto_trace(ofproto, &match.flow, packet,
>     -                  ofpacts.data, ofpacts.size, &result);
>     +                  ofpacts.data, ofpacts.size, &result, &next_ct_states);
>          unixctl_command_reply(conn, ds_cstr(&result));
>
>      exit:
>     @@ -541,7 +607,7 @@ static void
>      ofproto_trace(struct ofproto_dpif *ofproto, struct flow *flow,
>                    const struct dp_packet *packet,
>                    const struct ofpact ofpacts[], size_t ofpacts_len,
>     -              struct ds *output)
>     +              struct ds *output, struct ovs_list *next_ct_states)
>      {
>          struct ovs_list recirc_queue = OVS_LIST_INITIALIZER(&recirc_queue);
>          oftrace_add_recirc_node(&recirc_queue, OFT_INIT, flow, flow->recirc_id);
>     @@ -553,9 +619,21 @@ ofproto_trace(struct ofproto_dpif *ofproto, struct flow *flow,
>              ASSIGN_CONTAINER(recirc_node, node, node);
>
>              if (recirc_node->type == OFT_CONNTRACK) {
>     -            recirc_node->flow->ct_state = CS_ESTABLISHED | CS_TRACKED;
>     -            ds_put_cstr(output, "\n\nResume conntrack prcessing with "
>     -                                "default ct_state=trk|est.\n");
>     +            if (ovs_list_is_empty(next_ct_states)) {
>     +                recirc_node->flow->ct_state = CS_ESTABLISHED | CS_TRACKED;
>     +                ds_put_cstr(output, "\n\nResume conntrack prcessing with "
>     +                                    "default ct_state=trk|est. Use --ct-next "
>     +                                    "to customize\n");
>     +            } else {
>     +                recirc_node->flow->ct_state =
>     +                    oftrace_next_ct_state(next_ct_states);
>     +                struct ds s = DS_EMPTY_INITIALIZER;
>     +                format_flags(&s, ct_state_to_string,
>     +                             recirc_node->flow->ct_state, '|');
>     +                ds_put_format(output, "\n\nResume conntrack prcessing with "
>     +                                      "ct_state=%s\n", ds_cstr(&s));
>     +                ds_destroy(&s);
>     +            }
>                  ds_put_char_multiple(output, '-', 79);
>                  ds_put_char(output, '\n');
>              }
>     @@ -576,10 +654,11 @@ ofproto_dpif_trace_init(void)
>
>          unixctl_command_register(
>              "ofproto/trace",
>     -        "{[dp_name] odp_flow | bridge br_flow} [-generate|packet]",
>     -        1, 3, ofproto_unixctl_trace, NULL);
>     +        "{[dp_name] odp_flow | bridge br_flow} [OPTIONS...] "
>     +        "[-generate|packet]", 1, INT_MAX, ofproto_unixctl_trace, NULL);
>          unixctl_command_register(
>              "ofproto/trace-packet-out",
>     -        "[-consistent] {[dp_name] odp_flow | bridge br_flow} [-generate|packet] actions",
>     -        2, 6, ofproto_unixctl_trace_actions, NULL);
>     +        "[-consistent] {[dp_name] odp_flow | bridge br_flow} [OPTIONS...] "
>     +        "[-generate|packet] actions",
>     +        2, INT_MAX, ofproto_unixctl_trace_actions, NULL);
>      }
>     diff --git a/ofproto/ofproto-dpif-trace.h b/ofproto/ofproto-dpif-trace.h
>     index f7299fd42848..9bb080534a7a 100644
>     --- a/ofproto/ofproto-dpif-trace.h
>     +++ b/ofproto/ofproto-dpif-trace.h
>     @@ -73,6 +73,12 @@ struct oftrace_recirc_node {
>          struct flow *flow;
>      };
>
>     +/* A node within a next_ct_state list. */
>     +struct oftrace_next_ct_state {
>     +    struct ovs_list node;       /* In next_ct_states. */
>     +    uint32_t state;
>     +};
>     +
>      void ofproto_dpif_trace_init(void);
>
>      struct oftrace_node *oftrace_report(struct ovs_list *, enum oftrace_node_type,
>     diff --git a/ofproto/ofproto-unixctl.man b/ofproto/ofproto-unixctl.man
>     index 423837351910..e6c37894e980 100644
>     --- a/ofproto/ofproto-unixctl.man
>     +++ b/ofproto/ofproto-unixctl.man
>     @@ -6,10 +6,10 @@ These commands manage the core OpenFlow switch implementation (called
>      Lists the names of the running ofproto instances.  These are the names
>      that may be used on \fBofproto/trace\fR.
>      .
>     -.IP "\fBofproto/trace\fR [\fIdpname\fR] \fIodp_flow\fR [\fB\-generate \fR| \fIpacket\fR]"
>     -.IQ "\fBofproto/trace\fR \fIbridge\fR \fIbr_flow\fR [\fB\-generate \fR| \fIpacket\fR]"
>     -.IQ "\fBofproto/trace\-packet\-out\fR [\fB\-consistent\fR] [\fIdpname\fR] \fIodp_flow\fR [\fB\-generate \fR| \fIpacket\fR] \fIactions\fR"
>     -.IQ "\fBofproto/trace\-packet\-out\fR [\fB\-consistent\fR] \fIbridge\fR \fIbr_flow\fR [\fB\-generate \fR| \fIpacket\fR] \fIactions\fR"
>     +.IP "\fBofproto/trace\fR [\fIdpname\fR] \fIodp_flow\fR [\fIOPTIONS\fR] [\fB\-generate \fR| \fIpacket\fR]"
>     +.IQ "\fBofproto/trace\fR \fIbridge\fR \fIbr_flow\fR [\fIOPTIONS\fR] [\fB\-generate \fR| \fIpacket\fR]"
>     +.IQ "\fBofproto/trace\-packet\-out\fR [\fB\-consistent\fR] [\fIdpname\fR] \fIodp_flow\fR [\fIOPTIONS\fR] [\fB\-generate \fR| \fIpacket\fR] \fIactions\fR"
>     +.IQ "\fBofproto/trace\-packet\-out\fR [\fB\-consistent\fR] \fIbridge\fR \fIbr_flow\fR [\fIOPTIONS\fR] [\fB\-generate \fR| \fIpacket\fR] \fIactions\fR"
>      Traces the path of an imaginary packet through \fIswitch\fR and
>      reports the path that it took.  The initial treatment of the packet
>      varies based on the command:
>     @@ -48,6 +48,52 @@ wildcards.)  \fIbridge\fR names of the bridge through which
>      .RE
>      .
>      .IP
>     +.RS
>     +\fBofproto/trace\fR supports the following options:
>     +.
>     +.IP "--ct-next \fIflags\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 comma- or
>     +space-separated list of the following connection tracking flags:
>     +.
>     +.RS
>     +.IP \(bu
>     +\fBtrk\fR: Include to indicate connection tracking has taken place.
>     +.
>     +.IP \(bu
>     +\fBnew\fR: Include to indicate a new flow.
>     +.
>     +.IP \(bu
>     +\fBest\fR: Include to indicate an established flow.
>     +.
>     +.IP \(bu
>     +\fBrel\fR: Include to indicate a related flow.
>     +.
>     +.IP \(bu
>     +\fBrpl\fR: Include to indicate a reply flow.
>     +.
>     +.IP \(bu
>     +\fBinv\fR: Include to indicate a connection entry in a bad state.
>     +.
>     +.IP \(bu
>     +\fBdnat\fR: Include to indicate a packet whose destination IP address has been
>     +changed.
>     +.
>     +.IP \(bu
>     +\fBsnat\fR: Include to indicate a packet whose source IP address has been
>     +changed.
>     +.
>     +.RE
>     +.
>     +.IP
>     +When --ct-next is unspecified, or when there are fewer --ct-next options that
>     +ct actions, the \fIflags\fR default to trk,est.
>
> 1/ I think you should drop ‘est’ as part of the default.
>     Rarely, is the ‘est’ state being debugged.
>     trk by itself will catch more, in case the user does not specify options, is not aware of
>     the options or maybe does not even understand them.
>
> 2/ I think it is very surprising that if fewer –ct-next options are provided than
>      ct actions, that the remaining ct actions are traced as the default. I would
>      expect the last one specified to be used for any remaining ct actions, if any.
>      I think the typical usage would be the user providing one –ct-next and wanting
>      it applying to any/all ct actions.

Thanks for your coments. I feel like what makes more sense maybe to
query the current ct_state from conntrack module, which will be the
next thing that I plan to do later on. I choose trk|est because
ovn-trace use it as default. Actually, if we look at currently
available conntrack rules in the testcase (git grep '=+trk+est,' *.at
| wc -l), "+trk+est" hits similar (slightly more) # of rules comparing
to "+trk".
I think ofporto/trace is for advanced users to debug, and people may
use this tool to debug some tricky coner cases. Instead of speculating
the default use cases for debugging, as long as we clearly state what
is the default behavor in the trace and how can the users customize
the option in the document, it may be good for now. We can always have
another patch to chagne the defalt behavior when we get more feedback
from the users later.
>
>     +.
>     +.RE
>     +.
>     +.IP
>      Most commonly, one specifies only a flow, using one of the forms
>      above, but sometimes one might need to specify an actual packet
>      instead of just a flow:
>     diff --git a/tests/completion.at b/tests/completion.at
>     index e28c75239227..00e3a46b8bfa 100644
>     --- a/tests/completion.at
>     +++ b/tests/completion.at
>     @@ -171,7 +171,7 @@ AT_CLEANUP
>
>
>      # complex completion check - ofproto/trace
>     -# ofproto/trace {[dp_name] odp_flow | bridge br_flow} [-generate|packet]
>     +# ofproto/trace {[dp_name] odp_flow | bridge br_flow} [OPTIONS] [-generate|packet]
>      # test expansion on 'dp|dp_name' and 'bridge'
>      AT_SETUP([appctl-bashcomp - complex completion check 3])
>      AT_SKIP_IF([test -z ${BASH_VERSION+x}])
>     @@ -209,7 +209,8 @@ AT_CHECK_UNQUOTED([GET_AVAIL(${INPUT})], [0])
>      # set odp_flow to some random string, should go to the next level.
>      INPUT="$(bash ovs-appctl-bashcomp.bash debug ovs-appctl ofproto/trace ovs-dummy "in_port(123),mac(),ip,tcp" TAB 2>&1)"
>      MATCH="$(GET_COMP_STR([-generate], [-generate])
>     -GET_COMP_STR([packet], []))"
>     +GET_COMP_STR([packet], [])
>     +GET_COMP_STR([OPTIONS...], []))"
>      AT_CHECK_UNQUOTED([GET_EXPAN(${INPUT})],
>      [0], [dnl
>      ${MATCH}
>     @@ -242,7 +243,8 @@ AT_CHECK_UNQUOTED([GET_AVAIL(${INPUT})], [0])
>      # set argument to some random string, should go to the odp_flow path.
>      INPUT="$(bash ovs-appctl-bashcomp.bash debug ovs-appctl ofproto/trace "in_port(123),mac(),ip,tcp" TAB 2>&1)"
>      MATCH="$(GET_COMP_STR([-generate], [-generate])
>     -GET_COMP_STR([packet], []))"
>     +GET_COMP_STR([packet], [])
>     +GET_COMP_STR([OPTIONS...], []))"
>      AT_CHECK_UNQUOTED([GET_EXPAN(${INPUT})],
>      [0], [dnl
>      ${MATCH}
>     diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
>     index c51c689b9f46..142322afd777 100644
>     --- a/tests/ofproto-dpif.at
>     +++ b/tests/ofproto-dpif.at
>     @@ -5208,14 +5208,6 @@ Trailing garbage in packet data
>      ovs-appctl: ovs-vswitchd: server returned an error
>      ])
>
>     -# Test incorrect command: ofproto/trace with 4 arguments
>     -AT_CHECK([ovs-appctl ofproto/trace \
>     -  arg1, arg2, arg3, arg4], [2], [stdout],[stderr])
>     -AT_CHECK([tail -2 stderr], [0], [dnl
>     -"ofproto/trace" command takes at most 3 arguments
>     -ovs-appctl: ovs-vswitchd: server returned an error
>     -])
>     -
>      # Test incorrect command: ofproto/trace with 0 argument
>      AT_CHECK([ovs-appctl ofproto/trace ], [2], [stdout],[stderr])
>      AT_CHECK([tail -2 stderr], [0], [dnl
>     @@ -9713,13 +9705,14 @@ AT_CLEANUP
>      AT_SETUP([ofproto-dpif - conntrack - ofproto/trace])
>      OVS_VSWITCHD_START
>
>     -add_of_ports br0 1 2
>     +add_of_ports br0 1 2 3 4
>
>      AT_DATA([flows.txt], [dnl
>      dnl Table 0
>      dnl
>      table=0,priority=100,arp,action=normal
>      table=0,priority=10,udp,action=ct(table=1,zone=0)
>     +table=0,priority=10,tcp,action=ct(table=2,zone=1)
>      table=0,priority=1,action=drop
>      dnl
>      dnl Table 1
>     @@ -9728,6 +9721,18 @@ table=1,priority=10,in_port=1,ct_state=+trk+new,udp,action=ct(commit,zone=0),2
>      table=1,priority=10,in_port=1,ct_state=+trk+est,udp,action=2
>      table=1,priority=10,in_port=2,ct_state=+trk+est,udp,action=1
>      table=1,priority=1,action=drop
>     +dnl
>     +dnl Table 2
>     +dnl
>     +table=2,priority=10,in_port=1,tcp,ct_state=+trk+new,tcp,action=ct(commit,zone=1),ct(table=3,zone=2)
>     +table=2,priority=10,in_port=1,tcp,ct_state=+trk+est,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_state=+trk+new,tcp,action=ct(commit,zone=2),4
>     +table=3,priority=10,in_port=1,tcp,ct_state=+trk+est,tcp,action=3
>     +table=2,priority=1,action=drop
>      ])
>
>      AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
>     @@ -9742,6 +9747,16 @@ AT_CHECK([tail -1 stdout], [0],
>        [Datapath actions: 2
>      ])
>
>     +AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=1,tcp'], [0], [stdout])
>     +AT_CHECK([tail -1 stdout], [0],
>     +  [Datapath actions: 3
>     +])
>     +
>     +AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=1,tcp' --ct-next 'trk,new' --ct-next 'trk,new' ], [0], [stdout])
>     +AT_CHECK([tail -1 stdout], [0],
>     +  [Datapath actions: ct(commit,zone=2),4
>     +])
>     +
>      OVS_VSWITCHD_STOP
>      AT_CLEANUP
>
>     --
>     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=JmH2jZDW_1WD9HRXGFuUOs7-4mI6AXUopn7gzbNPXo8&s=Qwjtc88CeYT74ugg8YfxZXQAjxYM_7fELmzeLlVWyVY&e=
>
>
Yi-Hung Wei June 26, 2017, 5:18 p.m. UTC | #4 
On Wed, Jun 21, 2017 at 1:02 PM, Joe Stringer <joe@ovn.org> wrote:
> On 21 June 2017 at 11:06, Yi-Hung Wei <yihung.wei@gmail.com> wrote:
>> Previous patch enables ofproto/trace to automatically trace a flow
>> that involves multiple recirculation on conntrack. However, it always
>> sets the ct_state to trk|est when it processes recirculated conntrack flows.
>> With this patch, users can customize the expected next ct_state in the
>> aforementioned use case.
>>
>> Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>
>> ---
>>  NEWS                         |   2 +
>>  ofproto/ofproto-dpif-trace.c | 111 ++++++++++++++++++++++++++++++++++++-------
>>  ofproto/ofproto-dpif-trace.h |   6 +++
>>  ofproto/ofproto-unixctl.man  |  54 +++++++++++++++++++--
>>  tests/completion.at          |   8 ++--
>>  tests/ofproto-dpif.at        |  33 +++++++++----
>>  6 files changed, 182 insertions(+), 32 deletions(-)
>>
>> diff --git a/NEWS b/NEWS
>> index 4ea7e628e1fc..1824ec9de744 100644
>> --- a/NEWS
>> +++ b/NEWS
>> @@ -58,6 +58,8 @@ Post-v2.7.0
>>     - Fedora Packaging:
>>       * OVN services are no longer restarted automatically after upgrade.
>>     - Add --cleanup option to command 'ovs-appctl exit' (see ovs-vswitchd(8)).
>> +   - Add --ct-next option to command 'ovs-appctl ofprot/trace' (see
>
> ofproto/trace.
Fixed in v3.

>
>> +     ovs-vswitchd(8)).
>>     - L3 tunneling:
>>       * Add "layer3" options for tunnel ports that support non-Ethernet (L3)
>>         payload (GRE, VXLAN-GPE).
>> diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c
>> index d8cdd92493cf..e75c62d464eb 100644
>> --- a/ofproto/ofproto-dpif-trace.c
>> +++ b/ofproto/ofproto-dpif-trace.c
>> @@ -18,6 +18,7 @@
>>
>>  #include "ofproto-dpif-trace.h"
>>
>> +#include "conntrack.h"
>>  #include "dpif.h"
>>  #include "ofproto-dpif-xlate.h"
>>  #include "openvswitch/ofp-parse.h"
>> @@ -26,7 +27,7 @@
>>  static void ofproto_trace(struct ofproto_dpif *, struct flow *,
>>                            const struct dp_packet *packet,
>>                            const struct ofpact[], size_t ofpacts_len,
>> -                          struct ds *);
>> +                          struct ds *, struct ovs_list *next_ct_states);
>>  static void oftrace_node_destroy(struct oftrace_node *);
>>
>>  /* Creates a new oftrace_node, populates it with the given 'type' and a copy of
>> @@ -119,6 +120,17 @@ oftrace_recirc_node_destroy(struct oftrace_recirc_node *node)
>>      free(node);
>>  }
>>
>> +static uint32_t
>> +oftrace_next_ct_state(struct ovs_list *next_ct_states)
>> +{
>> +    struct oftrace_next_ct_state *next_ct_state;
>> +    ASSIGN_CONTAINER(next_ct_state, ovs_list_pop_front(next_ct_states), node);
>> +    uint32_t state = next_ct_state->state;
>> +    free(next_ct_state);
>
> It wouldn't hurt to expand this a bit for improved readability,
> declaring the variables first, then getting the ovs_list_pop_front()
> pointer, then assigning the container, getting state and freeing.
Sure.

>
>> +
>> +    return state;
>> +}
>> +
>>  static void
>>  oftrace_node_print_details(struct ds *output,
>>                             const struct ovs_list *nodes, int level)
>> @@ -160,18 +172,47 @@ 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)
>> +{
>> +    char *error = NULL;
>> +    int k;
>> +
>> +    for (k = 0; k < argc; k++) {
>> +        if (!strncmp(argv[k], "--ct-next", 9)) {
>> +            if (k + 1 > argc) {
>> +                error = xasprintf("Missing argument for option %s", argv[k]);
>> +                return error;
>
> I guess these could just be return xasprintf(...) ?
Sure.

>
>> +            }
>> +
>> +            uint32_t ct_state = parse_ct_state(argv[++k], 0);
>
> I'm a bit concerned that parse_oftrace_options() may be called from
> ovs-vswitchd, and parse_ct_state() may, on parse failure, exit in a
> fatal manner. If this is the case, perhaps we should refactor
> parse_ct_state() into a version that should exit fatally for
> commandline execution, and the version run from the main vswitchd
> thread.
This is a good point. I break parse_ct_state() into parse_ct_state()
and validate_ct_sate(), and move the ovs_fatal() logic into the caller
of parse_ct_state(). Later on if ovs-vswitchd needs to use
parse_ct_state() it will not necessarily trigger ovs_fatal().


>
>> +            struct oftrace_next_ct_state *next_state =
>> +                xmalloc(sizeof *next_state);
>> +            next_state->state = ct_state;
>> +            ovs_list_push_back(next_ct_states, &next_state->node);
>
> You might consider refactoring the above into a function like
> oftrace_push_ct_state(next_ct_states, ct_state). Then the malloc/free
> operations would be matched between this new function and the above
> oftrace_next_ct_state().. which you may also consider renaming
> something like oftrace_pop_ct_state(...).
Thanks, that will be more clean. I refactor that in v2.

>
>> +        } else {
>> +            error = xasprintf("Invalid option %s", argv[k]);
>> +            return error;
>
> Same as above xasprintf comment.
Yes.

>
>> +        }
>> +    }
>> +
>> +    return error;
>> +}
>> +
>>  /* Parses the 'argc' elements of 'argv', ignoring argv[0].  The following
>>   * forms are supported:
>>   *
>> - *     - [dpname] odp_flow [-generate | packet]
>> - *     - bridge br_flow [-generate | packet]
>> + *     - [dpname] odp_flow [OPTIONS] [-generate | packet]
>> + *     - bridge br_flow [OPTIONS] [-generate | packet]
>>   *
>>   * On success, initializes '*ofprotop' and 'flow' and returns NULL.  On failure
>>   * returns a nonnull malloced error message. */
>>  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 dp_packet **packetp,
>> +                      struct ovs_list *next_ct_states)
>>  {
>>      const struct dpif_backer *backer = NULL;
>>      const char *error = NULL;
>> @@ -180,6 +221,7 @@ parse_flow_and_packet(int argc, const char *argv[],
>>      struct dp_packet *packet;
>>      struct ofpbuf odp_key;
>>      struct ofpbuf odp_mask;
>> +    int first_option;
>>
>>      ofpbuf_init(&odp_key, 0);
>>      ofpbuf_init(&odp_mask, 0);
>> @@ -199,6 +241,25 @@ parse_flow_and_packet(int argc, const char *argv[],
>>          error = NULL;
>>      }
>>
>> +    /* Parse options. */
>> +    if (argc >= 4) {
>> +        if (!strncmp(argv[2], "--", 2)) {
>> +            first_option = 2;
>> +        } else if (!strncmp(argv[3], "--", 2)) {
>> +            first_option = 3;
>> +        } else {
>> +            error = "Syntax error";
>
> Could you expand on this a bit? It can be confusing for users to get
> stonewalled with "Syntax error" and not know why the syntax was wrong.
> (I realise there's other examples later in the function where this
> error is used, it'd be nice to improve that as well).
I replace it with "Syntax error: invalid option"
>
> Thanks!
Darrell Ball June 26, 2017, 6:10 p.m. UTC | #5 
On 6/26/17, 10:18 AM, "Yi-Hung Wei" <yihung.wei@gmail.com> wrote:

    On Wed, Jun 21, 2017 at 1:37 PM, 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:

    >

    >     Previous patch enables ofproto/trace to automatically trace a flow

    >     that involves multiple recirculation on conntrack. However, it always

    >     sets the ct_state to trk|est when it processes recirculated conntrack flows.

    >     With this patch, users can customize the expected next ct_state in the

    >     aforementioned use case.

    >

    >     Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>

    >     ---

    >      NEWS                         |   2 +

    >      ofproto/ofproto-dpif-trace.c | 111 ++++++++++++++++++++++++++++++++++++-------

    >      ofproto/ofproto-dpif-trace.h |   6 +++

    >      ofproto/ofproto-unixctl.man  |  54 +++++++++++++++++++--

    >      tests/completion.at          |   8 ++--

    >      tests/ofproto-dpif.at        |  33 +++++++++----

    >      6 files changed, 182 insertions(+), 32 deletions(-)

    >

    >     diff --git a/NEWS b/NEWS

    >     index 4ea7e628e1fc..1824ec9de744 100644

    >     --- a/NEWS

    >     +++ b/NEWS

    >     @@ -58,6 +58,8 @@ Post-v2.7.0

    >         - Fedora Packaging:

    >           * OVN services are no longer restarted automatically after upgrade.

    >         - Add --cleanup option to command 'ovs-appctl exit' (see ovs-vswitchd(8)).

    >     +   - Add --ct-next option to command 'ovs-appctl ofprot/trace' (see

    >     +     ovs-vswitchd(8)).

    >         - L3 tunneling:

    >           * Add "layer3" options for tunnel ports that support non-Ethernet (L3)

    >             payload (GRE, VXLAN-GPE).

    >     diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c

    >     index d8cdd92493cf..e75c62d464eb 100644

    >     --- a/ofproto/ofproto-dpif-trace.c

    >     +++ b/ofproto/ofproto-dpif-trace.c

    >     @@ -18,6 +18,7 @@

    >

    >      #include "ofproto-dpif-trace.h"

    >

    >     +#include "conntrack.h"

    >      #include "dpif.h"

    >      #include "ofproto-dpif-xlate.h"

    >      #include "openvswitch/ofp-parse.h"

    >     @@ -26,7 +27,7 @@

    >      static void ofproto_trace(struct ofproto_dpif *, struct flow *,

    >                                const struct dp_packet *packet,

    >                                const struct ofpact[], size_t ofpacts_len,

    >     -                          struct ds *);

    >     +                          struct ds *, struct ovs_list *next_ct_states);

    >      static void oftrace_node_destroy(struct oftrace_node *);

    >

    >      /* Creates a new oftrace_node, populates it with the given 'type' and a copy of

    >     @@ -119,6 +120,17 @@ oftrace_recirc_node_destroy(struct oftrace_recirc_node *node)

    >          free(node);

    >      }

    >

    >     +static uint32_t

    >     +oftrace_next_ct_state(struct ovs_list *next_ct_states)

    >     +{

    >     +    struct oftrace_next_ct_state *next_ct_state;

    >     +    ASSIGN_CONTAINER(next_ct_state, ovs_list_pop_front(next_ct_states), node);

    >     +    uint32_t state = next_ct_state->state;

    >     +    free(next_ct_state);

    >     +

    >     +    return state;

    >     +}

    >     +

    >      static void

    >      oftrace_node_print_details(struct ds *output,

    >                                 const struct ovs_list *nodes, int level)

    >     @@ -160,18 +172,47 @@ 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)

    >     +{

    >     +    char *error = NULL;

    >     +    int k;

    >     +

    >     +    for (k = 0; k < argc; k++) {

    >     +        if (!strncmp(argv[k], "--ct-next", 9)) {

    >     +            if (k + 1 > argc) {

    >     +                error = xasprintf("Missing argument for option %s", argv[k]);

    >     +                return error;

    >     +            }

    >     +

    >     +            uint32_t ct_state = parse_ct_state(argv[++k], 0);

    >     +            struct oftrace_next_ct_state *next_state =

    >     +                xmalloc(sizeof *next_state);

    >     +            next_state->state = ct_state;

    >     +            ovs_list_push_back(next_ct_states, &next_state->node);

    >     +        } else {

    >     +            error = xasprintf("Invalid option %s", argv[k]);

    >     +            return error;

    >     +        }

    >     +    }

    >     +

    >     +    return error;

    >     +}

    >     +

    >      /* Parses the 'argc' elements of 'argv', ignoring argv[0].  The following

    >       * forms are supported:

    >       *

    >     - *     - [dpname] odp_flow [-generate | packet]

    >     - *     - bridge br_flow [-generate | packet]

    >     + *     - [dpname] odp_flow [OPTIONS] [-generate | packet]

    >     + *     - bridge br_flow [OPTIONS] [-generate | packet]

    >       *

    >       * On success, initializes '*ofprotop' and 'flow' and returns NULL.  On failure

    >       * returns a nonnull malloced error message. */

    >      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 dp_packet **packetp,

    >     +                      struct ovs_list *next_ct_states)

    >      {

    >          const struct dpif_backer *backer = NULL;

    >          const char *error = NULL;

    >     @@ -180,6 +221,7 @@ parse_flow_and_packet(int argc, const char *argv[],

    >          struct dp_packet *packet;

    >          struct ofpbuf odp_key;

    >          struct ofpbuf odp_mask;

    >     +    int first_option;

    >

    >          ofpbuf_init(&odp_key, 0);

    >          ofpbuf_init(&odp_mask, 0);

    >     @@ -199,6 +241,25 @@ parse_flow_and_packet(int argc, const char *argv[],

    >              error = NULL;

    >          }

    >

    >     +    /* Parse options. */

    >     +    if (argc >= 4) {

    >     +        if (!strncmp(argv[2], "--", 2)) {

    >     +            first_option = 2;

    >     +        } else if (!strncmp(argv[3], "--", 2)) {

    >     +            first_option = 3;

    >     +        } else {

    >     +            error = "Syntax error";

    >     +            goto exit;

    >     +        }

    >     +

    >     +        m_err = parse_oftrace_options(argc - first_option, argv + first_option,

    >     +                                      next_ct_states);

    >     +        if (m_err) {

    >     +            goto exit;

    >     +        }

    >     +        argc = first_option;

    >     +    }

    >     +

    >          /* odp_flow can have its in_port specified as a name instead of port no.

    >           * We do not yet know whether a given flow is a odp_flow or a br_flow.

    >           * But, to know whether a flow is odp_flow through odp_flow_from_string(),

    >     @@ -338,13 +399,16 @@ 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);

    >

    >     -    error = parse_flow_and_packet(argc, argv, &ofproto, &flow, &packet);

    >     +    error = parse_flow_and_packet(argc, argv, &ofproto, &flow, &packet,

    >     +                                  &next_ct_states);

    >          if (!error) {

    >              struct ds result;

    >

    >              ds_init(&result);

    >     -        ofproto_trace(ofproto, &flow, packet, NULL, 0, &result);

    >     +        ofproto_trace(ofproto, &flow, packet, NULL, 0, &result,

    >     +                      &next_ct_states);

    >              unixctl_command_reply(conn, ds_cstr(&result));

    >              ds_destroy(&result);

    >              dp_packet_delete(packet);

    >     @@ -366,6 +430,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);

    >

    >          /* Three kinds of error return values! */

    >          enum ofperr retval;

    >     @@ -395,7 +460,8 @@ ofproto_unixctl_trace_actions(struct unixctl_conn *conn, int argc,

    >              enforce_consistency = false;

    >          }

    >

    >     -    error = parse_flow_and_packet(argc, argv, &ofproto, &match.flow, &packet);

    >     +    error = parse_flow_and_packet(argc, argv, &ofproto, &match.flow, &packet,

    >     +                                  &next_ct_states);

    >          if (error) {

    >              unixctl_command_reply_error(conn, error);

    >              free(error);

    >     @@ -443,7 +509,7 @@ ofproto_unixctl_trace_actions(struct unixctl_conn *conn, int argc,

    >          }

    >

    >          ofproto_trace(ofproto, &match.flow, packet,

    >     -                  ofpacts.data, ofpacts.size, &result);

    >     +                  ofpacts.data, ofpacts.size, &result, &next_ct_states);

    >          unixctl_command_reply(conn, ds_cstr(&result));

    >

    >      exit:

    >     @@ -541,7 +607,7 @@ static void

    >      ofproto_trace(struct ofproto_dpif *ofproto, struct flow *flow,

    >                    const struct dp_packet *packet,

    >                    const struct ofpact ofpacts[], size_t ofpacts_len,

    >     -              struct ds *output)

    >     +              struct ds *output, struct ovs_list *next_ct_states)

    >      {

    >          struct ovs_list recirc_queue = OVS_LIST_INITIALIZER(&recirc_queue);

    >          oftrace_add_recirc_node(&recirc_queue, OFT_INIT, flow, flow->recirc_id);

    >     @@ -553,9 +619,21 @@ ofproto_trace(struct ofproto_dpif *ofproto, struct flow *flow,

    >              ASSIGN_CONTAINER(recirc_node, node, node);

    >

    >              if (recirc_node->type == OFT_CONNTRACK) {

    >     -            recirc_node->flow->ct_state = CS_ESTABLISHED | CS_TRACKED;

    >     -            ds_put_cstr(output, "\n\nResume conntrack prcessing with "

    >     -                                "default ct_state=trk|est.\n");

    >     +            if (ovs_list_is_empty(next_ct_states)) {

    >     +                recirc_node->flow->ct_state = CS_ESTABLISHED | CS_TRACKED;

    >     +                ds_put_cstr(output, "\n\nResume conntrack prcessing with "

    >     +                                    "default ct_state=trk|est. Use --ct-next "

    >     +                                    "to customize\n");

    >     +            } else {

    >     +                recirc_node->flow->ct_state =

    >     +                    oftrace_next_ct_state(next_ct_states);

    >     +                struct ds s = DS_EMPTY_INITIALIZER;

    >     +                format_flags(&s, ct_state_to_string,

    >     +                             recirc_node->flow->ct_state, '|');

    >     +                ds_put_format(output, "\n\nResume conntrack prcessing with "

    >     +                                      "ct_state=%s\n", ds_cstr(&s));

    >     +                ds_destroy(&s);

    >     +            }

    >                  ds_put_char_multiple(output, '-', 79);

    >                  ds_put_char(output, '\n');

    >              }

    >     @@ -576,10 +654,11 @@ ofproto_dpif_trace_init(void)

    >

    >          unixctl_command_register(

    >              "ofproto/trace",

    >     -        "{[dp_name] odp_flow | bridge br_flow} [-generate|packet]",

    >     -        1, 3, ofproto_unixctl_trace, NULL);

    >     +        "{[dp_name] odp_flow | bridge br_flow} [OPTIONS...] "

    >     +        "[-generate|packet]", 1, INT_MAX, ofproto_unixctl_trace, NULL);

    >          unixctl_command_register(

    >              "ofproto/trace-packet-out",

    >     -        "[-consistent] {[dp_name] odp_flow | bridge br_flow} [-generate|packet] actions",

    >     -        2, 6, ofproto_unixctl_trace_actions, NULL);

    >     +        "[-consistent] {[dp_name] odp_flow | bridge br_flow} [OPTIONS...] "

    >     +        "[-generate|packet] actions",

    >     +        2, INT_MAX, ofproto_unixctl_trace_actions, NULL);

    >      }

    >     diff --git a/ofproto/ofproto-dpif-trace.h b/ofproto/ofproto-dpif-trace.h

    >     index f7299fd42848..9bb080534a7a 100644

    >     --- a/ofproto/ofproto-dpif-trace.h

    >     +++ b/ofproto/ofproto-dpif-trace.h

    >     @@ -73,6 +73,12 @@ struct oftrace_recirc_node {

    >          struct flow *flow;

    >      };

    >

    >     +/* A node within a next_ct_state list. */

    >     +struct oftrace_next_ct_state {

    >     +    struct ovs_list node;       /* In next_ct_states. */

    >     +    uint32_t state;

    >     +};

    >     +

    >      void ofproto_dpif_trace_init(void);

    >

    >      struct oftrace_node *oftrace_report(struct ovs_list *, enum oftrace_node_type,

    >     diff --git a/ofproto/ofproto-unixctl.man b/ofproto/ofproto-unixctl.man

    >     index 423837351910..e6c37894e980 100644

    >     --- a/ofproto/ofproto-unixctl.man

    >     +++ b/ofproto/ofproto-unixctl.man

    >     @@ -6,10 +6,10 @@ These commands manage the core OpenFlow switch implementation (called

    >      Lists the names of the running ofproto instances.  These are the names

    >      that may be used on \fBofproto/trace\fR.

    >      .

    >     -.IP "\fBofproto/trace\fR [\fIdpname\fR] \fIodp_flow\fR [\fB\-generate \fR| \fIpacket\fR]"

    >     -.IQ "\fBofproto/trace\fR \fIbridge\fR \fIbr_flow\fR [\fB\-generate \fR| \fIpacket\fR]"

    >     -.IQ "\fBofproto/trace\-packet\-out\fR [\fB\-consistent\fR] [\fIdpname\fR] \fIodp_flow\fR [\fB\-generate \fR| \fIpacket\fR] \fIactions\fR"

    >     -.IQ "\fBofproto/trace\-packet\-out\fR [\fB\-consistent\fR] \fIbridge\fR \fIbr_flow\fR [\fB\-generate \fR| \fIpacket\fR] \fIactions\fR"

    >     +.IP "\fBofproto/trace\fR [\fIdpname\fR] \fIodp_flow\fR [\fIOPTIONS\fR] [\fB\-generate \fR| \fIpacket\fR]"

    >     +.IQ "\fBofproto/trace\fR \fIbridge\fR \fIbr_flow\fR [\fIOPTIONS\fR] [\fB\-generate \fR| \fIpacket\fR]"

    >     +.IQ "\fBofproto/trace\-packet\-out\fR [\fB\-consistent\fR] [\fIdpname\fR] \fIodp_flow\fR [\fIOPTIONS\fR] [\fB\-generate \fR| \fIpacket\fR] \fIactions\fR"

    >     +.IQ "\fBofproto/trace\-packet\-out\fR [\fB\-consistent\fR] \fIbridge\fR \fIbr_flow\fR [\fIOPTIONS\fR] [\fB\-generate \fR| \fIpacket\fR] \fIactions\fR"

    >      Traces the path of an imaginary packet through \fIswitch\fR and

    >      reports the path that it took.  The initial treatment of the packet

    >      varies based on the command:

    >     @@ -48,6 +48,52 @@ wildcards.)  \fIbridge\fR names of the bridge through which

    >      .RE

    >      .

    >      .IP

    >     +.RS

    >     +\fBofproto/trace\fR supports the following options:

    >     +.

    >     +.IP "--ct-next \fIflags\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 comma- or

    >     +space-separated list of the following connection tracking flags:

    >     +.

    >     +.RS

    >     +.IP \(bu

    >     +\fBtrk\fR: Include to indicate connection tracking has taken place.

    >     +.

    >     +.IP \(bu

    >     +\fBnew\fR: Include to indicate a new flow.

    >     +.

    >     +.IP \(bu

    >     +\fBest\fR: Include to indicate an established flow.

    >     +.

    >     +.IP \(bu

    >     +\fBrel\fR: Include to indicate a related flow.

    >     +.

    >     +.IP \(bu

    >     +\fBrpl\fR: Include to indicate a reply flow.

    >     +.

    >     +.IP \(bu

    >     +\fBinv\fR: Include to indicate a connection entry in a bad state.

    >     +.

    >     +.IP \(bu

    >     +\fBdnat\fR: Include to indicate a packet whose destination IP address has been

    >     +changed.

    >     +.

    >     +.IP \(bu

    >     +\fBsnat\fR: Include to indicate a packet whose source IP address has been

    >     +changed.

    >     +.

    >     +.RE

    >     +.

    >     +.IP

    >     +When --ct-next is unspecified, or when there are fewer --ct-next options that

    >     +ct actions, the \fIflags\fR default to trk,est.

    >

    > 1/ I think you should drop ‘est’ as part of the default.

    >     Rarely, is the ‘est’ state being debugged.

    >     trk by itself will catch more, in case the user does not specify options, is not aware of

    >     the options or maybe does not even understand them.

    >

    > 2/ I think it is very surprising that if fewer –ct-next options are provided than

    >      ct actions, that the remaining ct actions are traced as the default. I would

    >      expect the last one specified to be used for any remaining ct actions, if any.

    >      I think the typical usage would be the user providing one –ct-next and wanting

    >      it applying to any/all ct actions.

    
    Thanks for your coments. I feel like what makes more sense maybe to
    query the current ct_state from conntrack module, which will be the
    next thing that I plan to do later on. 

Yi-hung
I am not following how your response is related to my above comments. 
I will ask offline and come back to the alias.


    I choose trk|est because
    ovn-trace use it as default. Actually, if we look at currently
    available conntrack rules in the testcase (git grep '=+trk+est,' *.at
    | wc -l), "+trk+est" hits similar (slightly more) # of rules comparing
    to "+trk".

The number of hits has nothing to do with my; it is quality vs quantity.
My comment is simply that people, like myself, usually want to find out
why a packet is ‘not’ EST. 


    I think ofporto/trace is for advanced users to debug, and people may
    use this tool to debug some tricky coner cases. Instead of speculating
    the default use cases for debugging, as long as we clearly state what
    is the default behavor in the trace and how can the users customize
    the option in the document, it may be good for now. We can always have
    another patch to chagne the defalt behavior when we get more feedback
    from the users later.

Who are the advanced users you are thinking about ?



    >

    >     +.

    >     +.RE

    >     +.

    >     +.IP

    >      Most commonly, one specifies only a flow, using one of the forms

    >      above, but sometimes one might need to specify an actual packet

    >      instead of just a flow:

    >     diff --git a/tests/completion.at b/tests/completion.at

    >     index e28c75239227..00e3a46b8bfa 100644

    >     --- a/tests/completion.at

    >     +++ b/tests/completion.at

    >     @@ -171,7 +171,7 @@ AT_CLEANUP

    >

    >

    >      # complex completion check - ofproto/trace

    >     -# ofproto/trace {[dp_name] odp_flow | bridge br_flow} [-generate|packet]

    >     +# ofproto/trace {[dp_name] odp_flow | bridge br_flow} [OPTIONS] [-generate|packet]

    >      # test expansion on 'dp|dp_name' and 'bridge'

    >      AT_SETUP([appctl-bashcomp - complex completion check 3])

    >      AT_SKIP_IF([test -z ${BASH_VERSION+x}])

    >     @@ -209,7 +209,8 @@ AT_CHECK_UNQUOTED([GET_AVAIL(${INPUT})], [0])

    >      # set odp_flow to some random string, should go to the next level.

    >      INPUT="$(bash ovs-appctl-bashcomp.bash debug ovs-appctl ofproto/trace ovs-dummy "in_port(123),mac(),ip,tcp" TAB 2>&1)"

    >      MATCH="$(GET_COMP_STR([-generate], [-generate])

    >     -GET_COMP_STR([packet], []))"

    >     +GET_COMP_STR([packet], [])

    >     +GET_COMP_STR([OPTIONS...], []))"

    >      AT_CHECK_UNQUOTED([GET_EXPAN(${INPUT})],

    >      [0], [dnl

    >      ${MATCH}

    >     @@ -242,7 +243,8 @@ AT_CHECK_UNQUOTED([GET_AVAIL(${INPUT})], [0])

    >      # set argument to some random string, should go to the odp_flow path.

    >      INPUT="$(bash ovs-appctl-bashcomp.bash debug ovs-appctl ofproto/trace "in_port(123),mac(),ip,tcp" TAB 2>&1)"

    >      MATCH="$(GET_COMP_STR([-generate], [-generate])

    >     -GET_COMP_STR([packet], []))"

    >     +GET_COMP_STR([packet], [])

    >     +GET_COMP_STR([OPTIONS...], []))"

    >      AT_CHECK_UNQUOTED([GET_EXPAN(${INPUT})],

    >      [0], [dnl

    >      ${MATCH}

    >     diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at

    >     index c51c689b9f46..142322afd777 100644

    >     --- a/tests/ofproto-dpif.at

    >     +++ b/tests/ofproto-dpif.at

    >     @@ -5208,14 +5208,6 @@ Trailing garbage in packet data

    >      ovs-appctl: ovs-vswitchd: server returned an error

    >      ])

    >

    >     -# Test incorrect command: ofproto/trace with 4 arguments

    >     -AT_CHECK([ovs-appctl ofproto/trace \

    >     -  arg1, arg2, arg3, arg4], [2], [stdout],[stderr])

    >     -AT_CHECK([tail -2 stderr], [0], [dnl

    >     -"ofproto/trace" command takes at most 3 arguments

    >     -ovs-appctl: ovs-vswitchd: server returned an error

    >     -])

    >     -

    >      # Test incorrect command: ofproto/trace with 0 argument

    >      AT_CHECK([ovs-appctl ofproto/trace ], [2], [stdout],[stderr])

    >      AT_CHECK([tail -2 stderr], [0], [dnl

    >     @@ -9713,13 +9705,14 @@ AT_CLEANUP

    >      AT_SETUP([ofproto-dpif - conntrack - ofproto/trace])

    >      OVS_VSWITCHD_START

    >

    >     -add_of_ports br0 1 2

    >     +add_of_ports br0 1 2 3 4

    >

    >      AT_DATA([flows.txt], [dnl

    >      dnl Table 0

    >      dnl

    >      table=0,priority=100,arp,action=normal

    >      table=0,priority=10,udp,action=ct(table=1,zone=0)

    >     +table=0,priority=10,tcp,action=ct(table=2,zone=1)

    >      table=0,priority=1,action=drop

    >      dnl

    >      dnl Table 1

    >     @@ -9728,6 +9721,18 @@ table=1,priority=10,in_port=1,ct_state=+trk+new,udp,action=ct(commit,zone=0),2

    >      table=1,priority=10,in_port=1,ct_state=+trk+est,udp,action=2

    >      table=1,priority=10,in_port=2,ct_state=+trk+est,udp,action=1

    >      table=1,priority=1,action=drop

    >     +dnl

    >     +dnl Table 2

    >     +dnl

    >     +table=2,priority=10,in_port=1,tcp,ct_state=+trk+new,tcp,action=ct(commit,zone=1),ct(table=3,zone=2)

    >     +table=2,priority=10,in_port=1,tcp,ct_state=+trk+est,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_state=+trk+new,tcp,action=ct(commit,zone=2),4

    >     +table=3,priority=10,in_port=1,tcp,ct_state=+trk+est,tcp,action=3

    >     +table=2,priority=1,action=drop

    >      ])

    >

    >      AT_CHECK([ovs-ofctl add-flows br0 flows.txt])

    >     @@ -9742,6 +9747,16 @@ AT_CHECK([tail -1 stdout], [0],

    >        [Datapath actions: 2

    >      ])

    >

    >     +AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=1,tcp'], [0], [stdout])

    >     +AT_CHECK([tail -1 stdout], [0],

    >     +  [Datapath actions: 3

    >     +])

    >     +

    >     +AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=1,tcp' --ct-next 'trk,new' --ct-next 'trk,new' ], [0], [stdout])

    >     +AT_CHECK([tail -1 stdout], [0],

    >     +  [Datapath actions: ct(commit,zone=2),4

    >     +])

    >     +

    >      OVS_VSWITCHD_STOP

    >      AT_CLEANUP

    >

    >     --

    >     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=JmH2jZDW_1WD9HRXGFuUOs7-4mI6AXUopn7gzbNPXo8&s=Qwjtc88CeYT74ugg8YfxZXQAjxYM_7fELmzeLlVWyVY&e=

    >

    >
Yi-Hung Wei June 27, 2017, 12:30 a.m. UTC | #6 
Darrell and I have some discussion, and we both agree on having the
default option to be 'trk|new'. I will send out a v3 based on that.

-Yi-Hung

On Mon, Jun 26, 2017 at 11:10 AM, Darrell Ball <dball@vmware.com> wrote:
>
>
> On 6/26/17, 10:18 AM, "Yi-Hung Wei" <yihung.wei@gmail.com> wrote:
>
>     On Wed, Jun 21, 2017 at 1:37 PM, 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:
>     >
>     >     Previous patch enables ofproto/trace to automatically trace a flow
>     >     that involves multiple recirculation on conntrack. However, it always
>     >     sets the ct_state to trk|est when it processes recirculated conntrack flows.
>     >     With this patch, users can customize the expected next ct_state in the
>     >     aforementioned use case.
>     >
>     >     Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>
>     >     ---
>     >      NEWS                         |   2 +
>     >      ofproto/ofproto-dpif-trace.c | 111 ++++++++++++++++++++++++++++++++++++-------
>     >      ofproto/ofproto-dpif-trace.h |   6 +++
>     >      ofproto/ofproto-unixctl.man  |  54 +++++++++++++++++++--
>     >      tests/completion.at          |   8 ++--
>     >      tests/ofproto-dpif.at        |  33 +++++++++----
>     >      6 files changed, 182 insertions(+), 32 deletions(-)
>     >
>     >     diff --git a/NEWS b/NEWS
>     >     index 4ea7e628e1fc..1824ec9de744 100644
>     >     --- a/NEWS
>     >     +++ b/NEWS
>     >     @@ -58,6 +58,8 @@ Post-v2.7.0
>     >         - Fedora Packaging:
>     >           * OVN services are no longer restarted automatically after upgrade.
>     >         - Add --cleanup option to command 'ovs-appctl exit' (see ovs-vswitchd(8)).
>     >     +   - Add --ct-next option to command 'ovs-appctl ofprot/trace' (see
>     >     +     ovs-vswitchd(8)).
>     >         - L3 tunneling:
>     >           * Add "layer3" options for tunnel ports that support non-Ethernet (L3)
>     >             payload (GRE, VXLAN-GPE).
>     >     diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c
>     >     index d8cdd92493cf..e75c62d464eb 100644
>     >     --- a/ofproto/ofproto-dpif-trace.c
>     >     +++ b/ofproto/ofproto-dpif-trace.c
>     >     @@ -18,6 +18,7 @@
>     >
>     >      #include "ofproto-dpif-trace.h"
>     >
>     >     +#include "conntrack.h"
>     >      #include "dpif.h"
>     >      #include "ofproto-dpif-xlate.h"
>     >      #include "openvswitch/ofp-parse.h"
>     >     @@ -26,7 +27,7 @@
>     >      static void ofproto_trace(struct ofproto_dpif *, struct flow *,
>     >                                const struct dp_packet *packet,
>     >                                const struct ofpact[], size_t ofpacts_len,
>     >     -                          struct ds *);
>     >     +                          struct ds *, struct ovs_list *next_ct_states);
>     >      static void oftrace_node_destroy(struct oftrace_node *);
>     >
>     >      /* Creates a new oftrace_node, populates it with the given 'type' and a copy of
>     >     @@ -119,6 +120,17 @@ oftrace_recirc_node_destroy(struct oftrace_recirc_node *node)
>     >          free(node);
>     >      }
>     >
>     >     +static uint32_t
>     >     +oftrace_next_ct_state(struct ovs_list *next_ct_states)
>     >     +{
>     >     +    struct oftrace_next_ct_state *next_ct_state;
>     >     +    ASSIGN_CONTAINER(next_ct_state, ovs_list_pop_front(next_ct_states), node);
>     >     +    uint32_t state = next_ct_state->state;
>     >     +    free(next_ct_state);
>     >     +
>     >     +    return state;
>     >     +}
>     >     +
>     >      static void
>     >      oftrace_node_print_details(struct ds *output,
>     >                                 const struct ovs_list *nodes, int level)
>     >     @@ -160,18 +172,47 @@ 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)
>     >     +{
>     >     +    char *error = NULL;
>     >     +    int k;
>     >     +
>     >     +    for (k = 0; k < argc; k++) {
>     >     +        if (!strncmp(argv[k], "--ct-next", 9)) {
>     >     +            if (k + 1 > argc) {
>     >     +                error = xasprintf("Missing argument for option %s", argv[k]);
>     >     +                return error;
>     >     +            }
>     >     +
>     >     +            uint32_t ct_state = parse_ct_state(argv[++k], 0);
>     >     +            struct oftrace_next_ct_state *next_state =
>     >     +                xmalloc(sizeof *next_state);
>     >     +            next_state->state = ct_state;
>     >     +            ovs_list_push_back(next_ct_states, &next_state->node);
>     >     +        } else {
>     >     +            error = xasprintf("Invalid option %s", argv[k]);
>     >     +            return error;
>     >     +        }
>     >     +    }
>     >     +
>     >     +    return error;
>     >     +}
>     >     +
>     >      /* Parses the 'argc' elements of 'argv', ignoring argv[0].  The following
>     >       * forms are supported:
>     >       *
>     >     - *     - [dpname] odp_flow [-generate | packet]
>     >     - *     - bridge br_flow [-generate | packet]
>     >     + *     - [dpname] odp_flow [OPTIONS] [-generate | packet]
>     >     + *     - bridge br_flow [OPTIONS] [-generate | packet]
>     >       *
>     >       * On success, initializes '*ofprotop' and 'flow' and returns NULL.  On failure
>     >       * returns a nonnull malloced error message. */
>     >      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 dp_packet **packetp,
>     >     +                      struct ovs_list *next_ct_states)
>     >      {
>     >          const struct dpif_backer *backer = NULL;
>     >          const char *error = NULL;
>     >     @@ -180,6 +221,7 @@ parse_flow_and_packet(int argc, const char *argv[],
>     >          struct dp_packet *packet;
>     >          struct ofpbuf odp_key;
>     >          struct ofpbuf odp_mask;
>     >     +    int first_option;
>     >
>     >          ofpbuf_init(&odp_key, 0);
>     >          ofpbuf_init(&odp_mask, 0);
>     >     @@ -199,6 +241,25 @@ parse_flow_and_packet(int argc, const char *argv[],
>     >              error = NULL;
>     >          }
>     >
>     >     +    /* Parse options. */
>     >     +    if (argc >= 4) {
>     >     +        if (!strncmp(argv[2], "--", 2)) {
>     >     +            first_option = 2;
>     >     +        } else if (!strncmp(argv[3], "--", 2)) {
>     >     +            first_option = 3;
>     >     +        } else {
>     >     +            error = "Syntax error";
>     >     +            goto exit;
>     >     +        }
>     >     +
>     >     +        m_err = parse_oftrace_options(argc - first_option, argv + first_option,
>     >     +                                      next_ct_states);
>     >     +        if (m_err) {
>     >     +            goto exit;
>     >     +        }
>     >     +        argc = first_option;
>     >     +    }
>     >     +
>     >          /* odp_flow can have its in_port specified as a name instead of port no.
>     >           * We do not yet know whether a given flow is a odp_flow or a br_flow.
>     >           * But, to know whether a flow is odp_flow through odp_flow_from_string(),
>     >     @@ -338,13 +399,16 @@ 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);
>     >
>     >     -    error = parse_flow_and_packet(argc, argv, &ofproto, &flow, &packet);
>     >     +    error = parse_flow_and_packet(argc, argv, &ofproto, &flow, &packet,
>     >     +                                  &next_ct_states);
>     >          if (!error) {
>     >              struct ds result;
>     >
>     >              ds_init(&result);
>     >     -        ofproto_trace(ofproto, &flow, packet, NULL, 0, &result);
>     >     +        ofproto_trace(ofproto, &flow, packet, NULL, 0, &result,
>     >     +                      &next_ct_states);
>     >              unixctl_command_reply(conn, ds_cstr(&result));
>     >              ds_destroy(&result);
>     >              dp_packet_delete(packet);
>     >     @@ -366,6 +430,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);
>     >
>     >          /* Three kinds of error return values! */
>     >          enum ofperr retval;
>     >     @@ -395,7 +460,8 @@ ofproto_unixctl_trace_actions(struct unixctl_conn *conn, int argc,
>     >              enforce_consistency = false;
>     >          }
>     >
>     >     -    error = parse_flow_and_packet(argc, argv, &ofproto, &match.flow, &packet);
>     >     +    error = parse_flow_and_packet(argc, argv, &ofproto, &match.flow, &packet,
>     >     +                                  &next_ct_states);
>     >          if (error) {
>     >              unixctl_command_reply_error(conn, error);
>     >              free(error);
>     >     @@ -443,7 +509,7 @@ ofproto_unixctl_trace_actions(struct unixctl_conn *conn, int argc,
>     >          }
>     >
>     >          ofproto_trace(ofproto, &match.flow, packet,
>     >     -                  ofpacts.data, ofpacts.size, &result);
>     >     +                  ofpacts.data, ofpacts.size, &result, &next_ct_states);
>     >          unixctl_command_reply(conn, ds_cstr(&result));
>     >
>     >      exit:
>     >     @@ -541,7 +607,7 @@ static void
>     >      ofproto_trace(struct ofproto_dpif *ofproto, struct flow *flow,
>     >                    const struct dp_packet *packet,
>     >                    const struct ofpact ofpacts[], size_t ofpacts_len,
>     >     -              struct ds *output)
>     >     +              struct ds *output, struct ovs_list *next_ct_states)
>     >      {
>     >          struct ovs_list recirc_queue = OVS_LIST_INITIALIZER(&recirc_queue);
>     >          oftrace_add_recirc_node(&recirc_queue, OFT_INIT, flow, flow->recirc_id);
>     >     @@ -553,9 +619,21 @@ ofproto_trace(struct ofproto_dpif *ofproto, struct flow *flow,
>     >              ASSIGN_CONTAINER(recirc_node, node, node);
>     >
>     >              if (recirc_node->type == OFT_CONNTRACK) {
>     >     -            recirc_node->flow->ct_state = CS_ESTABLISHED | CS_TRACKED;
>     >     -            ds_put_cstr(output, "\n\nResume conntrack prcessing with "
>     >     -                                "default ct_state=trk|est.\n");
>     >     +            if (ovs_list_is_empty(next_ct_states)) {
>     >     +                recirc_node->flow->ct_state = CS_ESTABLISHED | CS_TRACKED;
>     >     +                ds_put_cstr(output, "\n\nResume conntrack prcessing with "
>     >     +                                    "default ct_state=trk|est. Use --ct-next "
>     >     +                                    "to customize\n");
>     >     +            } else {
>     >     +                recirc_node->flow->ct_state =
>     >     +                    oftrace_next_ct_state(next_ct_states);
>     >     +                struct ds s = DS_EMPTY_INITIALIZER;
>     >     +                format_flags(&s, ct_state_to_string,
>     >     +                             recirc_node->flow->ct_state, '|');
>     >     +                ds_put_format(output, "\n\nResume conntrack prcessing with "
>     >     +                                      "ct_state=%s\n", ds_cstr(&s));
>     >     +                ds_destroy(&s);
>     >     +            }
>     >                  ds_put_char_multiple(output, '-', 79);
>     >                  ds_put_char(output, '\n');
>     >              }
>     >     @@ -576,10 +654,11 @@ ofproto_dpif_trace_init(void)
>     >
>     >          unixctl_command_register(
>     >              "ofproto/trace",
>     >     -        "{[dp_name] odp_flow | bridge br_flow} [-generate|packet]",
>     >     -        1, 3, ofproto_unixctl_trace, NULL);
>     >     +        "{[dp_name] odp_flow | bridge br_flow} [OPTIONS...] "
>     >     +        "[-generate|packet]", 1, INT_MAX, ofproto_unixctl_trace, NULL);
>     >          unixctl_command_register(
>     >              "ofproto/trace-packet-out",
>     >     -        "[-consistent] {[dp_name] odp_flow | bridge br_flow} [-generate|packet] actions",
>     >     -        2, 6, ofproto_unixctl_trace_actions, NULL);
>     >     +        "[-consistent] {[dp_name] odp_flow | bridge br_flow} [OPTIONS...] "
>     >     +        "[-generate|packet] actions",
>     >     +        2, INT_MAX, ofproto_unixctl_trace_actions, NULL);
>     >      }
>     >     diff --git a/ofproto/ofproto-dpif-trace.h b/ofproto/ofproto-dpif-trace.h
>     >     index f7299fd42848..9bb080534a7a 100644
>     >     --- a/ofproto/ofproto-dpif-trace.h
>     >     +++ b/ofproto/ofproto-dpif-trace.h
>     >     @@ -73,6 +73,12 @@ struct oftrace_recirc_node {
>     >          struct flow *flow;
>     >      };
>     >
>     >     +/* A node within a next_ct_state list. */
>     >     +struct oftrace_next_ct_state {
>     >     +    struct ovs_list node;       /* In next_ct_states. */
>     >     +    uint32_t state;
>     >     +};
>     >     +
>     >      void ofproto_dpif_trace_init(void);
>     >
>     >      struct oftrace_node *oftrace_report(struct ovs_list *, enum oftrace_node_type,
>     >     diff --git a/ofproto/ofproto-unixctl.man b/ofproto/ofproto-unixctl.man
>     >     index 423837351910..e6c37894e980 100644
>     >     --- a/ofproto/ofproto-unixctl.man
>     >     +++ b/ofproto/ofproto-unixctl.man
>     >     @@ -6,10 +6,10 @@ These commands manage the core OpenFlow switch implementation (called
>     >      Lists the names of the running ofproto instances.  These are the names
>     >      that may be used on \fBofproto/trace\fR.
>     >      .
>     >     -.IP "\fBofproto/trace\fR [\fIdpname\fR] \fIodp_flow\fR [\fB\-generate \fR| \fIpacket\fR]"
>     >     -.IQ "\fBofproto/trace\fR \fIbridge\fR \fIbr_flow\fR [\fB\-generate \fR| \fIpacket\fR]"
>     >     -.IQ "\fBofproto/trace\-packet\-out\fR [\fB\-consistent\fR] [\fIdpname\fR] \fIodp_flow\fR [\fB\-generate \fR| \fIpacket\fR] \fIactions\fR"
>     >     -.IQ "\fBofproto/trace\-packet\-out\fR [\fB\-consistent\fR] \fIbridge\fR \fIbr_flow\fR [\fB\-generate \fR| \fIpacket\fR] \fIactions\fR"
>     >     +.IP "\fBofproto/trace\fR [\fIdpname\fR] \fIodp_flow\fR [\fIOPTIONS\fR] [\fB\-generate \fR| \fIpacket\fR]"
>     >     +.IQ "\fBofproto/trace\fR \fIbridge\fR \fIbr_flow\fR [\fIOPTIONS\fR] [\fB\-generate \fR| \fIpacket\fR]"
>     >     +.IQ "\fBofproto/trace\-packet\-out\fR [\fB\-consistent\fR] [\fIdpname\fR] \fIodp_flow\fR [\fIOPTIONS\fR] [\fB\-generate \fR| \fIpacket\fR] \fIactions\fR"
>     >     +.IQ "\fBofproto/trace\-packet\-out\fR [\fB\-consistent\fR] \fIbridge\fR \fIbr_flow\fR [\fIOPTIONS\fR] [\fB\-generate \fR| \fIpacket\fR] \fIactions\fR"
>     >      Traces the path of an imaginary packet through \fIswitch\fR and
>     >      reports the path that it took.  The initial treatment of the packet
>     >      varies based on the command:
>     >     @@ -48,6 +48,52 @@ wildcards.)  \fIbridge\fR names of the bridge through which
>     >      .RE
>     >      .
>     >      .IP
>     >     +.RS
>     >     +\fBofproto/trace\fR supports the following options:
>     >     +.
>     >     +.IP "--ct-next \fIflags\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 comma- or
>     >     +space-separated list of the following connection tracking flags:
>     >     +.
>     >     +.RS
>     >     +.IP \(bu
>     >     +\fBtrk\fR: Include to indicate connection tracking has taken place.
>     >     +.
>     >     +.IP \(bu
>     >     +\fBnew\fR: Include to indicate a new flow.
>     >     +.
>     >     +.IP \(bu
>     >     +\fBest\fR: Include to indicate an established flow.
>     >     +.
>     >     +.IP \(bu
>     >     +\fBrel\fR: Include to indicate a related flow.
>     >     +.
>     >     +.IP \(bu
>     >     +\fBrpl\fR: Include to indicate a reply flow.
>     >     +.
>     >     +.IP \(bu
>     >     +\fBinv\fR: Include to indicate a connection entry in a bad state.
>     >     +.
>     >     +.IP \(bu
>     >     +\fBdnat\fR: Include to indicate a packet whose destination IP address has been
>     >     +changed.
>     >     +.
>     >     +.IP \(bu
>     >     +\fBsnat\fR: Include to indicate a packet whose source IP address has been
>     >     +changed.
>     >     +.
>     >     +.RE
>     >     +.
>     >     +.IP
>     >     +When --ct-next is unspecified, or when there are fewer --ct-next options that
>     >     +ct actions, the \fIflags\fR default to trk,est.
>     >
>     > 1/ I think you should drop ‘est’ as part of the default.
>     >     Rarely, is the ‘est’ state being debugged.
>     >     trk by itself will catch more, in case the user does not specify options, is not aware of
>     >     the options or maybe does not even understand them.
>     >
>     > 2/ I think it is very surprising that if fewer –ct-next options are provided than
>     >      ct actions, that the remaining ct actions are traced as the default. I would
>     >      expect the last one specified to be used for any remaining ct actions, if any.
>     >      I think the typical usage would be the user providing one –ct-next and wanting
>     >      it applying to any/all ct actions.
>
>     Thanks for your coments. I feel like what makes more sense maybe to
>     query the current ct_state from conntrack module, which will be the
>     next thing that I plan to do later on.
>
> Yi-hung
> I am not following how your response is related to my above comments.
> I will ask offline and come back to the alias.
>
>
>     I choose trk|est because
>     ovn-trace use it as default. Actually, if we look at currently
>     available conntrack rules in the testcase (git grep '=+trk+est,' *.at
>     | wc -l), "+trk+est" hits similar (slightly more) # of rules comparing
>     to "+trk".
>
> The number of hits has nothing to do with my; it is quality vs quantity.
> My comment is simply that people, like myself, usually want to find out
> why a packet is ‘not’ EST.
>
>
>     I think ofporto/trace is for advanced users to debug, and people may
>     use this tool to debug some tricky coner cases. Instead of speculating
>     the default use cases for debugging, as long as we clearly state what
>     is the default behavor in the trace and how can the users customize
>     the option in the document, it may be good for now. We can always have
>     another patch to chagne the defalt behavior when we get more feedback
>     from the users later.
>
> Who are the advanced users you are thinking about ?
>
>
>
>     >
>     >     +.
>     >     +.RE
>     >     +.
>     >     +.IP
>     >      Most commonly, one specifies only a flow, using one of the forms
>     >      above, but sometimes one might need to specify an actual packet
>     >      instead of just a flow:
>     >     diff --git a/tests/completion.at b/tests/completion.at
>     >     index e28c75239227..00e3a46b8bfa 100644
>     >     --- a/tests/completion.at
>     >     +++ b/tests/completion.at
>     >     @@ -171,7 +171,7 @@ AT_CLEANUP
>     >
>     >
>     >      # complex completion check - ofproto/trace
>     >     -# ofproto/trace {[dp_name] odp_flow | bridge br_flow} [-generate|packet]
>     >     +# ofproto/trace {[dp_name] odp_flow | bridge br_flow} [OPTIONS] [-generate|packet]
>     >      # test expansion on 'dp|dp_name' and 'bridge'
>     >      AT_SETUP([appctl-bashcomp - complex completion check 3])
>     >      AT_SKIP_IF([test -z ${BASH_VERSION+x}])
>     >     @@ -209,7 +209,8 @@ AT_CHECK_UNQUOTED([GET_AVAIL(${INPUT})], [0])
>     >      # set odp_flow to some random string, should go to the next level.
>     >      INPUT="$(bash ovs-appctl-bashcomp.bash debug ovs-appctl ofproto/trace ovs-dummy "in_port(123),mac(),ip,tcp" TAB 2>&1)"
>     >      MATCH="$(GET_COMP_STR([-generate], [-generate])
>     >     -GET_COMP_STR([packet], []))"
>     >     +GET_COMP_STR([packet], [])
>     >     +GET_COMP_STR([OPTIONS...], []))"
>     >      AT_CHECK_UNQUOTED([GET_EXPAN(${INPUT})],
>     >      [0], [dnl
>     >      ${MATCH}
>     >     @@ -242,7 +243,8 @@ AT_CHECK_UNQUOTED([GET_AVAIL(${INPUT})], [0])
>     >      # set argument to some random string, should go to the odp_flow path.
>     >      INPUT="$(bash ovs-appctl-bashcomp.bash debug ovs-appctl ofproto/trace "in_port(123),mac(),ip,tcp" TAB 2>&1)"
>     >      MATCH="$(GET_COMP_STR([-generate], [-generate])
>     >     -GET_COMP_STR([packet], []))"
>     >     +GET_COMP_STR([packet], [])
>     >     +GET_COMP_STR([OPTIONS...], []))"
>     >      AT_CHECK_UNQUOTED([GET_EXPAN(${INPUT})],
>     >      [0], [dnl
>     >      ${MATCH}
>     >     diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
>     >     index c51c689b9f46..142322afd777 100644
>     >     --- a/tests/ofproto-dpif.at
>     >     +++ b/tests/ofproto-dpif.at
>     >     @@ -5208,14 +5208,6 @@ Trailing garbage in packet data
>     >      ovs-appctl: ovs-vswitchd: server returned an error
>     >      ])
>     >
>     >     -# Test incorrect command: ofproto/trace with 4 arguments
>     >     -AT_CHECK([ovs-appctl ofproto/trace \
>     >     -  arg1, arg2, arg3, arg4], [2], [stdout],[stderr])
>     >     -AT_CHECK([tail -2 stderr], [0], [dnl
>     >     -"ofproto/trace" command takes at most 3 arguments
>     >     -ovs-appctl: ovs-vswitchd: server returned an error
>     >     -])
>     >     -
>     >      # Test incorrect command: ofproto/trace with 0 argument
>     >      AT_CHECK([ovs-appctl ofproto/trace ], [2], [stdout],[stderr])
>     >      AT_CHECK([tail -2 stderr], [0], [dnl
>     >     @@ -9713,13 +9705,14 @@ AT_CLEANUP
>     >      AT_SETUP([ofproto-dpif - conntrack - ofproto/trace])
>     >      OVS_VSWITCHD_START
>     >
>     >     -add_of_ports br0 1 2
>     >     +add_of_ports br0 1 2 3 4
>     >
>     >      AT_DATA([flows.txt], [dnl
>     >      dnl Table 0
>     >      dnl
>     >      table=0,priority=100,arp,action=normal
>     >      table=0,priority=10,udp,action=ct(table=1,zone=0)
>     >     +table=0,priority=10,tcp,action=ct(table=2,zone=1)
>     >      table=0,priority=1,action=drop
>     >      dnl
>     >      dnl Table 1
>     >     @@ -9728,6 +9721,18 @@ table=1,priority=10,in_port=1,ct_state=+trk+new,udp,action=ct(commit,zone=0),2
>     >      table=1,priority=10,in_port=1,ct_state=+trk+est,udp,action=2
>     >      table=1,priority=10,in_port=2,ct_state=+trk+est,udp,action=1
>     >      table=1,priority=1,action=drop
>     >     +dnl
>     >     +dnl Table 2
>     >     +dnl
>     >     +table=2,priority=10,in_port=1,tcp,ct_state=+trk+new,tcp,action=ct(commit,zone=1),ct(table=3,zone=2)
>     >     +table=2,priority=10,in_port=1,tcp,ct_state=+trk+est,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_state=+trk+new,tcp,action=ct(commit,zone=2),4
>     >     +table=3,priority=10,in_port=1,tcp,ct_state=+trk+est,tcp,action=3
>     >     +table=2,priority=1,action=drop
>     >      ])
>     >
>     >      AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
>     >     @@ -9742,6 +9747,16 @@ AT_CHECK([tail -1 stdout], [0],
>     >        [Datapath actions: 2
>     >      ])
>     >
>     >     +AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=1,tcp'], [0], [stdout])
>     >     +AT_CHECK([tail -1 stdout], [0],
>     >     +  [Datapath actions: 3
>     >     +])
>     >     +
>     >     +AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=1,tcp' --ct-next 'trk,new' --ct-next 'trk,new' ], [0], [stdout])
>     >     +AT_CHECK([tail -1 stdout], [0],
>     >     +  [Datapath actions: ct(commit,zone=2),4
>     >     +])
>     >     +
>     >      OVS_VSWITCHD_STOP
>     >      AT_CLEANUP
>     >
>     >     --
>     >     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=JmH2jZDW_1WD9HRXGFuUOs7-4mI6AXUopn7gzbNPXo8&s=Qwjtc88CeYT74ugg8YfxZXQAjxYM_7fELmzeLlVWyVY&e=
>     >
>     >
>
>
>
>
Darrell Ball June 27, 2017, 12:49 a.m. UTC | #7 
Thanks Yi-hung
That should partially address point ‘2’  as well.
Maybe we can also describe the iterative process of debugging ct-actions, so
the user has the expectation, basically describing that debugging would probably 
start with one ct-next and then add another one by one on each subsequent run
as more information becomes available.

Darrell

On 6/26/17, 5:30 PM, "Yi-Hung Wei" <yihung.wei@gmail.com> wrote:

    Darrell and I have some discussion, and we both agree on having the
    default option to be 'trk|new'. I will send out a v3 based on that.
    
    -Yi-Hung
    
    On Mon, Jun 26, 2017 at 11:10 AM, Darrell Ball <dball@vmware.com> wrote:
    >

    >

    > On 6/26/17, 10:18 AM, "Yi-Hung Wei" <yihung.wei@gmail.com> wrote:

    >

    >     On Wed, Jun 21, 2017 at 1:37 PM, 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:

    >     >

    >     >     Previous patch enables ofproto/trace to automatically trace a flow

    >     >     that involves multiple recirculation on conntrack. However, it always

    >     >     sets the ct_state to trk|est when it processes recirculated conntrack flows.

    >     >     With this patch, users can customize the expected next ct_state in the

    >     >     aforementioned use case.

    >     >

    >     >     Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>

    >     >     ---

    >     >      NEWS                         |   2 +

    >     >      ofproto/ofproto-dpif-trace.c | 111 ++++++++++++++++++++++++++++++++++++-------

    >     >      ofproto/ofproto-dpif-trace.h |   6 +++

    >     >      ofproto/ofproto-unixctl.man  |  54 +++++++++++++++++++--

    >     >      tests/completion.at          |   8 ++--

    >     >      tests/ofproto-dpif.at        |  33 +++++++++----

    >     >      6 files changed, 182 insertions(+), 32 deletions(-)

    >     >

    >     >     diff --git a/NEWS b/NEWS

    >     >     index 4ea7e628e1fc..1824ec9de744 100644

    >     >     --- a/NEWS

    >     >     +++ b/NEWS

    >     >     @@ -58,6 +58,8 @@ Post-v2.7.0

    >     >         - Fedora Packaging:

    >     >           * OVN services are no longer restarted automatically after upgrade.

    >     >         - Add --cleanup option to command 'ovs-appctl exit' (see ovs-vswitchd(8)).

    >     >     +   - Add --ct-next option to command 'ovs-appctl ofprot/trace' (see

    >     >     +     ovs-vswitchd(8)).

    >     >         - L3 tunneling:

    >     >           * Add "layer3" options for tunnel ports that support non-Ethernet (L3)

    >     >             payload (GRE, VXLAN-GPE).

    >     >     diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c

    >     >     index d8cdd92493cf..e75c62d464eb 100644

    >     >     --- a/ofproto/ofproto-dpif-trace.c

    >     >     +++ b/ofproto/ofproto-dpif-trace.c

    >     >     @@ -18,6 +18,7 @@

    >     >

    >     >      #include "ofproto-dpif-trace.h"

    >     >

    >     >     +#include "conntrack.h"

    >     >      #include "dpif.h"

    >     >      #include "ofproto-dpif-xlate.h"

    >     >      #include "openvswitch/ofp-parse.h"

    >     >     @@ -26,7 +27,7 @@

    >     >      static void ofproto_trace(struct ofproto_dpif *, struct flow *,

    >     >                                const struct dp_packet *packet,

    >     >                                const struct ofpact[], size_t ofpacts_len,

    >     >     -                          struct ds *);

    >     >     +                          struct ds *, struct ovs_list *next_ct_states);

    >     >      static void oftrace_node_destroy(struct oftrace_node *);

    >     >

    >     >      /* Creates a new oftrace_node, populates it with the given 'type' and a copy of

    >     >     @@ -119,6 +120,17 @@ oftrace_recirc_node_destroy(struct oftrace_recirc_node *node)

    >     >          free(node);

    >     >      }

    >     >

    >     >     +static uint32_t

    >     >     +oftrace_next_ct_state(struct ovs_list *next_ct_states)

    >     >     +{

    >     >     +    struct oftrace_next_ct_state *next_ct_state;

    >     >     +    ASSIGN_CONTAINER(next_ct_state, ovs_list_pop_front(next_ct_states), node);

    >     >     +    uint32_t state = next_ct_state->state;

    >     >     +    free(next_ct_state);

    >     >     +

    >     >     +    return state;

    >     >     +}

    >     >     +

    >     >      static void

    >     >      oftrace_node_print_details(struct ds *output,

    >     >                                 const struct ovs_list *nodes, int level)

    >     >     @@ -160,18 +172,47 @@ 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)

    >     >     +{

    >     >     +    char *error = NULL;

    >     >     +    int k;

    >     >     +

    >     >     +    for (k = 0; k < argc; k++) {

    >     >     +        if (!strncmp(argv[k], "--ct-next", 9)) {

    >     >     +            if (k + 1 > argc) {

    >     >     +                error = xasprintf("Missing argument for option %s", argv[k]);

    >     >     +                return error;

    >     >     +            }

    >     >     +

    >     >     +            uint32_t ct_state = parse_ct_state(argv[++k], 0);

    >     >     +            struct oftrace_next_ct_state *next_state =

    >     >     +                xmalloc(sizeof *next_state);

    >     >     +            next_state->state = ct_state;

    >     >     +            ovs_list_push_back(next_ct_states, &next_state->node);

    >     >     +        } else {

    >     >     +            error = xasprintf("Invalid option %s", argv[k]);

    >     >     +            return error;

    >     >     +        }

    >     >     +    }

    >     >     +

    >     >     +    return error;

    >     >     +}

    >     >     +

    >     >      /* Parses the 'argc' elements of 'argv', ignoring argv[0].  The following

    >     >       * forms are supported:

    >     >       *

    >     >     - *     - [dpname] odp_flow [-generate | packet]

    >     >     - *     - bridge br_flow [-generate | packet]

    >     >     + *     - [dpname] odp_flow [OPTIONS] [-generate | packet]

    >     >     + *     - bridge br_flow [OPTIONS] [-generate | packet]

    >     >       *

    >     >       * On success, initializes '*ofprotop' and 'flow' and returns NULL.  On failure

    >     >       * returns a nonnull malloced error message. */

    >     >      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 dp_packet **packetp,

    >     >     +                      struct ovs_list *next_ct_states)

    >     >      {

    >     >          const struct dpif_backer *backer = NULL;

    >     >          const char *error = NULL;

    >     >     @@ -180,6 +221,7 @@ parse_flow_and_packet(int argc, const char *argv[],

    >     >          struct dp_packet *packet;

    >     >          struct ofpbuf odp_key;

    >     >          struct ofpbuf odp_mask;

    >     >     +    int first_option;

    >     >

    >     >          ofpbuf_init(&odp_key, 0);

    >     >          ofpbuf_init(&odp_mask, 0);

    >     >     @@ -199,6 +241,25 @@ parse_flow_and_packet(int argc, const char *argv[],

    >     >              error = NULL;

    >     >          }

    >     >

    >     >     +    /* Parse options. */

    >     >     +    if (argc >= 4) {

    >     >     +        if (!strncmp(argv[2], "--", 2)) {

    >     >     +            first_option = 2;

    >     >     +        } else if (!strncmp(argv[3], "--", 2)) {

    >     >     +            first_option = 3;

    >     >     +        } else {

    >     >     +            error = "Syntax error";

    >     >     +            goto exit;

    >     >     +        }

    >     >     +

    >     >     +        m_err = parse_oftrace_options(argc - first_option, argv + first_option,

    >     >     +                                      next_ct_states);

    >     >     +        if (m_err) {

    >     >     +            goto exit;

    >     >     +        }

    >     >     +        argc = first_option;

    >     >     +    }

    >     >     +

    >     >          /* odp_flow can have its in_port specified as a name instead of port no.

    >     >           * We do not yet know whether a given flow is a odp_flow or a br_flow.

    >     >           * But, to know whether a flow is odp_flow through odp_flow_from_string(),

    >     >     @@ -338,13 +399,16 @@ 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);

    >     >

    >     >     -    error = parse_flow_and_packet(argc, argv, &ofproto, &flow, &packet);

    >     >     +    error = parse_flow_and_packet(argc, argv, &ofproto, &flow, &packet,

    >     >     +                                  &next_ct_states);

    >     >          if (!error) {

    >     >              struct ds result;

    >     >

    >     >              ds_init(&result);

    >     >     -        ofproto_trace(ofproto, &flow, packet, NULL, 0, &result);

    >     >     +        ofproto_trace(ofproto, &flow, packet, NULL, 0, &result,

    >     >     +                      &next_ct_states);

    >     >              unixctl_command_reply(conn, ds_cstr(&result));

    >     >              ds_destroy(&result);

    >     >              dp_packet_delete(packet);

    >     >     @@ -366,6 +430,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);

    >     >

    >     >          /* Three kinds of error return values! */

    >     >          enum ofperr retval;

    >     >     @@ -395,7 +460,8 @@ ofproto_unixctl_trace_actions(struct unixctl_conn *conn, int argc,

    >     >              enforce_consistency = false;

    >     >          }

    >     >

    >     >     -    error = parse_flow_and_packet(argc, argv, &ofproto, &match.flow, &packet);

    >     >     +    error = parse_flow_and_packet(argc, argv, &ofproto, &match.flow, &packet,

    >     >     +                                  &next_ct_states);

    >     >          if (error) {

    >     >              unixctl_command_reply_error(conn, error);

    >     >              free(error);

    >     >     @@ -443,7 +509,7 @@ ofproto_unixctl_trace_actions(struct unixctl_conn *conn, int argc,

    >     >          }

    >     >

    >     >          ofproto_trace(ofproto, &match.flow, packet,

    >     >     -                  ofpacts.data, ofpacts.size, &result);

    >     >     +                  ofpacts.data, ofpacts.size, &result, &next_ct_states);

    >     >          unixctl_command_reply(conn, ds_cstr(&result));

    >     >

    >     >      exit:

    >     >     @@ -541,7 +607,7 @@ static void

    >     >      ofproto_trace(struct ofproto_dpif *ofproto, struct flow *flow,

    >     >                    const struct dp_packet *packet,

    >     >                    const struct ofpact ofpacts[], size_t ofpacts_len,

    >     >     -              struct ds *output)

    >     >     +              struct ds *output, struct ovs_list *next_ct_states)

    >     >      {

    >     >          struct ovs_list recirc_queue = OVS_LIST_INITIALIZER(&recirc_queue);

    >     >          oftrace_add_recirc_node(&recirc_queue, OFT_INIT, flow, flow->recirc_id);

    >     >     @@ -553,9 +619,21 @@ ofproto_trace(struct ofproto_dpif *ofproto, struct flow *flow,

    >     >              ASSIGN_CONTAINER(recirc_node, node, node);

    >     >

    >     >              if (recirc_node->type == OFT_CONNTRACK) {

    >     >     -            recirc_node->flow->ct_state = CS_ESTABLISHED | CS_TRACKED;

    >     >     -            ds_put_cstr(output, "\n\nResume conntrack prcessing with "

    >     >     -                                "default ct_state=trk|est.\n");

    >     >     +            if (ovs_list_is_empty(next_ct_states)) {

    >     >     +                recirc_node->flow->ct_state = CS_ESTABLISHED | CS_TRACKED;

    >     >     +                ds_put_cstr(output, "\n\nResume conntrack prcessing with "

    >     >     +                                    "default ct_state=trk|est. Use --ct-next "

    >     >     +                                    "to customize\n");

    >     >     +            } else {

    >     >     +                recirc_node->flow->ct_state =

    >     >     +                    oftrace_next_ct_state(next_ct_states);

    >     >     +                struct ds s = DS_EMPTY_INITIALIZER;

    >     >     +                format_flags(&s, ct_state_to_string,

    >     >     +                             recirc_node->flow->ct_state, '|');

    >     >     +                ds_put_format(output, "\n\nResume conntrack prcessing with "

    >     >     +                                      "ct_state=%s\n", ds_cstr(&s));

    >     >     +                ds_destroy(&s);

    >     >     +            }

    >     >                  ds_put_char_multiple(output, '-', 79);

    >     >                  ds_put_char(output, '\n');

    >     >              }

    >     >     @@ -576,10 +654,11 @@ ofproto_dpif_trace_init(void)

    >     >

    >     >          unixctl_command_register(

    >     >              "ofproto/trace",

    >     >     -        "{[dp_name] odp_flow | bridge br_flow} [-generate|packet]",

    >     >     -        1, 3, ofproto_unixctl_trace, NULL);

    >     >     +        "{[dp_name] odp_flow | bridge br_flow} [OPTIONS...] "

    >     >     +        "[-generate|packet]", 1, INT_MAX, ofproto_unixctl_trace, NULL);

    >     >          unixctl_command_register(

    >     >              "ofproto/trace-packet-out",

    >     >     -        "[-consistent] {[dp_name] odp_flow | bridge br_flow} [-generate|packet] actions",

    >     >     -        2, 6, ofproto_unixctl_trace_actions, NULL);

    >     >     +        "[-consistent] {[dp_name] odp_flow | bridge br_flow} [OPTIONS...] "

    >     >     +        "[-generate|packet] actions",

    >     >     +        2, INT_MAX, ofproto_unixctl_trace_actions, NULL);

    >     >      }

    >     >     diff --git a/ofproto/ofproto-dpif-trace.h b/ofproto/ofproto-dpif-trace.h

    >     >     index f7299fd42848..9bb080534a7a 100644

    >     >     --- a/ofproto/ofproto-dpif-trace.h

    >     >     +++ b/ofproto/ofproto-dpif-trace.h

    >     >     @@ -73,6 +73,12 @@ struct oftrace_recirc_node {

    >     >          struct flow *flow;

    >     >      };

    >     >

    >     >     +/* A node within a next_ct_state list. */

    >     >     +struct oftrace_next_ct_state {

    >     >     +    struct ovs_list node;       /* In next_ct_states. */

    >     >     +    uint32_t state;

    >     >     +};

    >     >     +

    >     >      void ofproto_dpif_trace_init(void);

    >     >

    >     >      struct oftrace_node *oftrace_report(struct ovs_list *, enum oftrace_node_type,

    >     >     diff --git a/ofproto/ofproto-unixctl.man b/ofproto/ofproto-unixctl.man

    >     >     index 423837351910..e6c37894e980 100644

    >     >     --- a/ofproto/ofproto-unixctl.man

    >     >     +++ b/ofproto/ofproto-unixctl.man

    >     >     @@ -6,10 +6,10 @@ These commands manage the core OpenFlow switch implementation (called

    >     >      Lists the names of the running ofproto instances.  These are the names

    >     >      that may be used on \fBofproto/trace\fR.

    >     >      .

    >     >     -.IP "\fBofproto/trace\fR [\fIdpname\fR] \fIodp_flow\fR [\fB\-generate \fR| \fIpacket\fR]"

    >     >     -.IQ "\fBofproto/trace\fR \fIbridge\fR \fIbr_flow\fR [\fB\-generate \fR| \fIpacket\fR]"

    >     >     -.IQ "\fBofproto/trace\-packet\-out\fR [\fB\-consistent\fR] [\fIdpname\fR] \fIodp_flow\fR [\fB\-generate \fR| \fIpacket\fR] \fIactions\fR"

    >     >     -.IQ "\fBofproto/trace\-packet\-out\fR [\fB\-consistent\fR] \fIbridge\fR \fIbr_flow\fR [\fB\-generate \fR| \fIpacket\fR] \fIactions\fR"

    >     >     +.IP "\fBofproto/trace\fR [\fIdpname\fR] \fIodp_flow\fR [\fIOPTIONS\fR] [\fB\-generate \fR| \fIpacket\fR]"

    >     >     +.IQ "\fBofproto/trace\fR \fIbridge\fR \fIbr_flow\fR [\fIOPTIONS\fR] [\fB\-generate \fR| \fIpacket\fR]"

    >     >     +.IQ "\fBofproto/trace\-packet\-out\fR [\fB\-consistent\fR] [\fIdpname\fR] \fIodp_flow\fR [\fIOPTIONS\fR] [\fB\-generate \fR| \fIpacket\fR] \fIactions\fR"

    >     >     +.IQ "\fBofproto/trace\-packet\-out\fR [\fB\-consistent\fR] \fIbridge\fR \fIbr_flow\fR [\fIOPTIONS\fR] [\fB\-generate \fR| \fIpacket\fR] \fIactions\fR"

    >     >      Traces the path of an imaginary packet through \fIswitch\fR and

    >     >      reports the path that it took.  The initial treatment of the packet

    >     >      varies based on the command:

    >     >     @@ -48,6 +48,52 @@ wildcards.)  \fIbridge\fR names of the bridge through which

    >     >      .RE

    >     >      .

    >     >      .IP

    >     >     +.RS

    >     >     +\fBofproto/trace\fR supports the following options:

    >     >     +.

    >     >     +.IP "--ct-next \fIflags\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 comma- or

    >     >     +space-separated list of the following connection tracking flags:

    >     >     +.

    >     >     +.RS

    >     >     +.IP \(bu

    >     >     +\fBtrk\fR: Include to indicate connection tracking has taken place.

    >     >     +.

    >     >     +.IP \(bu

    >     >     +\fBnew\fR: Include to indicate a new flow.

    >     >     +.

    >     >     +.IP \(bu

    >     >     +\fBest\fR: Include to indicate an established flow.

    >     >     +.

    >     >     +.IP \(bu

    >     >     +\fBrel\fR: Include to indicate a related flow.

    >     >     +.

    >     >     +.IP \(bu

    >     >     +\fBrpl\fR: Include to indicate a reply flow.

    >     >     +.

    >     >     +.IP \(bu

    >     >     +\fBinv\fR: Include to indicate a connection entry in a bad state.

    >     >     +.

    >     >     +.IP \(bu

    >     >     +\fBdnat\fR: Include to indicate a packet whose destination IP address has been

    >     >     +changed.

    >     >     +.

    >     >     +.IP \(bu

    >     >     +\fBsnat\fR: Include to indicate a packet whose source IP address has been

    >     >     +changed.

    >     >     +.

    >     >     +.RE

    >     >     +.

    >     >     +.IP

    >     >     +When --ct-next is unspecified, or when there are fewer --ct-next options that

    >     >     +ct actions, the \fIflags\fR default to trk,est.

    >     >

    >     > 1/ I think you should drop ‘est’ as part of the default.

    >     >     Rarely, is the ‘est’ state being debugged.

    >     >     trk by itself will catch more, in case the user does not specify options, is not aware of

    >     >     the options or maybe does not even understand them.

    >     >

    >     > 2/ I think it is very surprising that if fewer –ct-next options are provided than

    >     >      ct actions, that the remaining ct actions are traced as the default. I would

    >     >      expect the last one specified to be used for any remaining ct actions, if any.

    >     >      I think the typical usage would be the user providing one –ct-next and wanting

    >     >      it applying to any/all ct actions.

    >

    >     Thanks for your coments. I feel like what makes more sense maybe to

    >     query the current ct_state from conntrack module, which will be the

    >     next thing that I plan to do later on.

    >

    > Yi-hung

    > I am not following how your response is related to my above comments.

    > I will ask offline and come back to the alias.

    >

    >

    >     I choose trk|est because

    >     ovn-trace use it as default. Actually, if we look at currently

    >     available conntrack rules in the testcase (git grep '=+trk+est,' *.at

    >     | wc -l), "+trk+est" hits similar (slightly more) # of rules comparing

    >     to "+trk".

    >

    > The number of hits has nothing to do with my; it is quality vs quantity.

    > My comment is simply that people, like myself, usually want to find out

    > why a packet is ‘not’ EST.

    >

    >

    >     I think ofporto/trace is for advanced users to debug, and people may

    >     use this tool to debug some tricky coner cases. Instead of speculating

    >     the default use cases for debugging, as long as we clearly state what

    >     is the default behavor in the trace and how can the users customize

    >     the option in the document, it may be good for now. We can always have

    >     another patch to chagne the defalt behavior when we get more feedback

    >     from the users later.

    >

    > Who are the advanced users you are thinking about ?

    >

    >

    >

    >     >

    >     >     +.

    >     >     +.RE

    >     >     +.

    >     >     +.IP

    >     >      Most commonly, one specifies only a flow, using one of the forms

    >     >      above, but sometimes one might need to specify an actual packet

    >     >      instead of just a flow:

    >     >     diff --git a/tests/completion.at b/tests/completion.at

    >     >     index e28c75239227..00e3a46b8bfa 100644

    >     >     --- a/tests/completion.at

    >     >     +++ b/tests/completion.at

    >     >     @@ -171,7 +171,7 @@ AT_CLEANUP

    >     >

    >     >

    >     >      # complex completion check - ofproto/trace

    >     >     -# ofproto/trace {[dp_name] odp_flow | bridge br_flow} [-generate|packet]

    >     >     +# ofproto/trace {[dp_name] odp_flow | bridge br_flow} [OPTIONS] [-generate|packet]

    >     >      # test expansion on 'dp|dp_name' and 'bridge'

    >     >      AT_SETUP([appctl-bashcomp - complex completion check 3])

    >     >      AT_SKIP_IF([test -z ${BASH_VERSION+x}])

    >     >     @@ -209,7 +209,8 @@ AT_CHECK_UNQUOTED([GET_AVAIL(${INPUT})], [0])

    >     >      # set odp_flow to some random string, should go to the next level.

    >     >      INPUT="$(bash ovs-appctl-bashcomp.bash debug ovs-appctl ofproto/trace ovs-dummy "in_port(123),mac(),ip,tcp" TAB 2>&1)"

    >     >      MATCH="$(GET_COMP_STR([-generate], [-generate])

    >     >     -GET_COMP_STR([packet], []))"

    >     >     +GET_COMP_STR([packet], [])

    >     >     +GET_COMP_STR([OPTIONS...], []))"

    >     >      AT_CHECK_UNQUOTED([GET_EXPAN(${INPUT})],

    >     >      [0], [dnl

    >     >      ${MATCH}

    >     >     @@ -242,7 +243,8 @@ AT_CHECK_UNQUOTED([GET_AVAIL(${INPUT})], [0])

    >     >      # set argument to some random string, should go to the odp_flow path.

    >     >      INPUT="$(bash ovs-appctl-bashcomp.bash debug ovs-appctl ofproto/trace "in_port(123),mac(),ip,tcp" TAB 2>&1)"

    >     >      MATCH="$(GET_COMP_STR([-generate], [-generate])

    >     >     -GET_COMP_STR([packet], []))"

    >     >     +GET_COMP_STR([packet], [])

    >     >     +GET_COMP_STR([OPTIONS...], []))"

    >     >      AT_CHECK_UNQUOTED([GET_EXPAN(${INPUT})],

    >     >      [0], [dnl

    >     >      ${MATCH}

    >     >     diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at

    >     >     index c51c689b9f46..142322afd777 100644

    >     >     --- a/tests/ofproto-dpif.at

    >     >     +++ b/tests/ofproto-dpif.at

    >     >     @@ -5208,14 +5208,6 @@ Trailing garbage in packet data

    >     >      ovs-appctl: ovs-vswitchd: server returned an error

    >     >      ])

    >     >

    >     >     -# Test incorrect command: ofproto/trace with 4 arguments

    >     >     -AT_CHECK([ovs-appctl ofproto/trace \

    >     >     -  arg1, arg2, arg3, arg4], [2], [stdout],[stderr])

    >     >     -AT_CHECK([tail -2 stderr], [0], [dnl

    >     >     -"ofproto/trace" command takes at most 3 arguments

    >     >     -ovs-appctl: ovs-vswitchd: server returned an error

    >     >     -])

    >     >     -

    >     >      # Test incorrect command: ofproto/trace with 0 argument

    >     >      AT_CHECK([ovs-appctl ofproto/trace ], [2], [stdout],[stderr])

    >     >      AT_CHECK([tail -2 stderr], [0], [dnl

    >     >     @@ -9713,13 +9705,14 @@ AT_CLEANUP

    >     >      AT_SETUP([ofproto-dpif - conntrack - ofproto/trace])

    >     >      OVS_VSWITCHD_START

    >     >

    >     >     -add_of_ports br0 1 2

    >     >     +add_of_ports br0 1 2 3 4

    >     >

    >     >      AT_DATA([flows.txt], [dnl

    >     >      dnl Table 0

    >     >      dnl

    >     >      table=0,priority=100,arp,action=normal

    >     >      table=0,priority=10,udp,action=ct(table=1,zone=0)

    >     >     +table=0,priority=10,tcp,action=ct(table=2,zone=1)

    >     >      table=0,priority=1,action=drop

    >     >      dnl

    >     >      dnl Table 1

    >     >     @@ -9728,6 +9721,18 @@ table=1,priority=10,in_port=1,ct_state=+trk+new,udp,action=ct(commit,zone=0),2

    >     >      table=1,priority=10,in_port=1,ct_state=+trk+est,udp,action=2

    >     >      table=1,priority=10,in_port=2,ct_state=+trk+est,udp,action=1

    >     >      table=1,priority=1,action=drop

    >     >     +dnl

    >     >     +dnl Table 2

    >     >     +dnl

    >     >     +table=2,priority=10,in_port=1,tcp,ct_state=+trk+new,tcp,action=ct(commit,zone=1),ct(table=3,zone=2)

    >     >     +table=2,priority=10,in_port=1,tcp,ct_state=+trk+est,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_state=+trk+new,tcp,action=ct(commit,zone=2),4

    >     >     +table=3,priority=10,in_port=1,tcp,ct_state=+trk+est,tcp,action=3

    >     >     +table=2,priority=1,action=drop

    >     >      ])

    >     >

    >     >      AT_CHECK([ovs-ofctl add-flows br0 flows.txt])

    >     >     @@ -9742,6 +9747,16 @@ AT_CHECK([tail -1 stdout], [0],

    >     >        [Datapath actions: 2

    >     >      ])

    >     >

    >     >     +AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=1,tcp'], [0], [stdout])

    >     >     +AT_CHECK([tail -1 stdout], [0],

    >     >     +  [Datapath actions: 3

    >     >     +])

    >     >     +

    >     >     +AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=1,tcp' --ct-next 'trk,new' --ct-next 'trk,new' ], [0], [stdout])

    >     >     +AT_CHECK([tail -1 stdout], [0],

    >     >     +  [Datapath actions: ct(commit,zone=2),4

    >     >     +])

    >     >     +

    >     >      OVS_VSWITCHD_STOP

    >     >      AT_CLEANUP

    >     >

    >     >     --

    >     >     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=JmH2jZDW_1WD9HRXGFuUOs7-4mI6AXUopn7gzbNPXo8&s=Qwjtc88CeYT74ugg8YfxZXQAjxYM_7fELmzeLlVWyVY&e=

    >     >

    >     >

    >

    >

    >

    >
diff mbox

Patch

diff --git a/NEWS b/NEWS
index 4ea7e628e1fc..1824ec9de744 100644
--- a/NEWS
+++ b/NEWS
@@ -58,6 +58,8 @@  Post-v2.7.0
    - Fedora Packaging:
      * OVN services are no longer restarted automatically after upgrade.
    - Add --cleanup option to command 'ovs-appctl exit' (see ovs-vswitchd(8)).
+   - Add --ct-next option to command 'ovs-appctl ofprot/trace' (see
+     ovs-vswitchd(8)).
    - L3 tunneling:
      * Add "layer3" options for tunnel ports that support non-Ethernet (L3)
        payload (GRE, VXLAN-GPE).
diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c
index d8cdd92493cf..e75c62d464eb 100644
--- a/ofproto/ofproto-dpif-trace.c
+++ b/ofproto/ofproto-dpif-trace.c
@@ -18,6 +18,7 @@ 
 
 #include "ofproto-dpif-trace.h"
 
+#include "conntrack.h"
 #include "dpif.h"
 #include "ofproto-dpif-xlate.h"
 #include "openvswitch/ofp-parse.h"
@@ -26,7 +27,7 @@ 
 static void ofproto_trace(struct ofproto_dpif *, struct flow *,
                           const struct dp_packet *packet,
                           const struct ofpact[], size_t ofpacts_len,
-                          struct ds *);
+                          struct ds *, struct ovs_list *next_ct_states);
 static void oftrace_node_destroy(struct oftrace_node *);
 
 /* Creates a new oftrace_node, populates it with the given 'type' and a copy of
@@ -119,6 +120,17 @@  oftrace_recirc_node_destroy(struct oftrace_recirc_node *node)
     free(node);
 }
 
+static uint32_t
+oftrace_next_ct_state(struct ovs_list *next_ct_states)
+{
+    struct oftrace_next_ct_state *next_ct_state;
+    ASSIGN_CONTAINER(next_ct_state, ovs_list_pop_front(next_ct_states), node);
+    uint32_t state = next_ct_state->state;
+    free(next_ct_state);
+
+    return state;
+}
+
 static void
 oftrace_node_print_details(struct ds *output,
                            const struct ovs_list *nodes, int level)
@@ -160,18 +172,47 @@  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)
+{
+    char *error = NULL;
+    int k;
+
+    for (k = 0; k < argc; k++) {
+        if (!strncmp(argv[k], "--ct-next", 9)) {
+            if (k + 1 > argc) {
+                error = xasprintf("Missing argument for option %s", argv[k]);
+                return error;
+            }
+
+            uint32_t ct_state = parse_ct_state(argv[++k], 0);
+            struct oftrace_next_ct_state *next_state =
+                xmalloc(sizeof *next_state);
+            next_state->state = ct_state;
+            ovs_list_push_back(next_ct_states, &next_state->node);
+        } else {
+            error = xasprintf("Invalid option %s", argv[k]);
+            return error;
+        }
+    }
+
+    return error;
+}
+
 /* Parses the 'argc' elements of 'argv', ignoring argv[0].  The following
  * forms are supported:
  *
- *     - [dpname] odp_flow [-generate | packet]
- *     - bridge br_flow [-generate | packet]
+ *     - [dpname] odp_flow [OPTIONS] [-generate | packet]
+ *     - bridge br_flow [OPTIONS] [-generate | packet]
  *
  * On success, initializes '*ofprotop' and 'flow' and returns NULL.  On failure
  * returns a nonnull malloced error message. */
 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 dp_packet **packetp,
+                      struct ovs_list *next_ct_states)
 {
     const struct dpif_backer *backer = NULL;
     const char *error = NULL;
@@ -180,6 +221,7 @@  parse_flow_and_packet(int argc, const char *argv[],
     struct dp_packet *packet;
     struct ofpbuf odp_key;
     struct ofpbuf odp_mask;
+    int first_option;
 
     ofpbuf_init(&odp_key, 0);
     ofpbuf_init(&odp_mask, 0);
@@ -199,6 +241,25 @@  parse_flow_and_packet(int argc, const char *argv[],
         error = NULL;
     }
 
+    /* Parse options. */
+    if (argc >= 4) {
+        if (!strncmp(argv[2], "--", 2)) {
+            first_option = 2;
+        } else if (!strncmp(argv[3], "--", 2)) {
+            first_option = 3;
+        } else {
+            error = "Syntax error";
+            goto exit;
+        }
+
+        m_err = parse_oftrace_options(argc - first_option, argv + first_option,
+                                      next_ct_states);
+        if (m_err) {
+            goto exit;
+        }
+        argc = first_option;
+    }
+
     /* odp_flow can have its in_port specified as a name instead of port no.
      * We do not yet know whether a given flow is a odp_flow or a br_flow.
      * But, to know whether a flow is odp_flow through odp_flow_from_string(),
@@ -338,13 +399,16 @@  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);
 
-    error = parse_flow_and_packet(argc, argv, &ofproto, &flow, &packet);
+    error = parse_flow_and_packet(argc, argv, &ofproto, &flow, &packet,
+                                  &next_ct_states);
     if (!error) {
         struct ds result;
 
         ds_init(&result);
-        ofproto_trace(ofproto, &flow, packet, NULL, 0, &result);
+        ofproto_trace(ofproto, &flow, packet, NULL, 0, &result,
+                      &next_ct_states);
         unixctl_command_reply(conn, ds_cstr(&result));
         ds_destroy(&result);
         dp_packet_delete(packet);
@@ -366,6 +430,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);
 
     /* Three kinds of error return values! */
     enum ofperr retval;
@@ -395,7 +460,8 @@  ofproto_unixctl_trace_actions(struct unixctl_conn *conn, int argc,
         enforce_consistency = false;
     }
 
-    error = parse_flow_and_packet(argc, argv, &ofproto, &match.flow, &packet);
+    error = parse_flow_and_packet(argc, argv, &ofproto, &match.flow, &packet,
+                                  &next_ct_states);
     if (error) {
         unixctl_command_reply_error(conn, error);
         free(error);
@@ -443,7 +509,7 @@  ofproto_unixctl_trace_actions(struct unixctl_conn *conn, int argc,
     }
 
     ofproto_trace(ofproto, &match.flow, packet,
-                  ofpacts.data, ofpacts.size, &result);
+                  ofpacts.data, ofpacts.size, &result, &next_ct_states);
     unixctl_command_reply(conn, ds_cstr(&result));
 
 exit:
@@ -541,7 +607,7 @@  static void
 ofproto_trace(struct ofproto_dpif *ofproto, struct flow *flow,
               const struct dp_packet *packet,
               const struct ofpact ofpacts[], size_t ofpacts_len,
-              struct ds *output)
+              struct ds *output, struct ovs_list *next_ct_states)
 {
     struct ovs_list recirc_queue = OVS_LIST_INITIALIZER(&recirc_queue);
     oftrace_add_recirc_node(&recirc_queue, OFT_INIT, flow, flow->recirc_id);
@@ -553,9 +619,21 @@  ofproto_trace(struct ofproto_dpif *ofproto, struct flow *flow,
         ASSIGN_CONTAINER(recirc_node, node, node);
 
         if (recirc_node->type == OFT_CONNTRACK) {
-            recirc_node->flow->ct_state = CS_ESTABLISHED | CS_TRACKED;
-            ds_put_cstr(output, "\n\nResume conntrack prcessing with "
-                                "default ct_state=trk|est.\n");
+            if (ovs_list_is_empty(next_ct_states)) {
+                recirc_node->flow->ct_state = CS_ESTABLISHED | CS_TRACKED;
+                ds_put_cstr(output, "\n\nResume conntrack prcessing with "
+                                    "default ct_state=trk|est. Use --ct-next "
+                                    "to customize\n");
+            } else {
+                recirc_node->flow->ct_state =
+                    oftrace_next_ct_state(next_ct_states);
+                struct ds s = DS_EMPTY_INITIALIZER;
+                format_flags(&s, ct_state_to_string,
+                             recirc_node->flow->ct_state, '|');
+                ds_put_format(output, "\n\nResume conntrack prcessing with "
+                                      "ct_state=%s\n", ds_cstr(&s));
+                ds_destroy(&s);
+            }
             ds_put_char_multiple(output, '-', 79);
             ds_put_char(output, '\n');
         }
@@ -576,10 +654,11 @@  ofproto_dpif_trace_init(void)
 
     unixctl_command_register(
         "ofproto/trace",
-        "{[dp_name] odp_flow | bridge br_flow} [-generate|packet]",
-        1, 3, ofproto_unixctl_trace, NULL);
+        "{[dp_name] odp_flow | bridge br_flow} [OPTIONS...] "
+        "[-generate|packet]", 1, INT_MAX, ofproto_unixctl_trace, NULL);
     unixctl_command_register(
         "ofproto/trace-packet-out",
-        "[-consistent] {[dp_name] odp_flow | bridge br_flow} [-generate|packet] actions",
-        2, 6, ofproto_unixctl_trace_actions, NULL);
+        "[-consistent] {[dp_name] odp_flow | bridge br_flow} [OPTIONS...] "
+        "[-generate|packet] actions",
+        2, INT_MAX, ofproto_unixctl_trace_actions, NULL);
 }
diff --git a/ofproto/ofproto-dpif-trace.h b/ofproto/ofproto-dpif-trace.h
index f7299fd42848..9bb080534a7a 100644
--- a/ofproto/ofproto-dpif-trace.h
+++ b/ofproto/ofproto-dpif-trace.h
@@ -73,6 +73,12 @@  struct oftrace_recirc_node {
     struct flow *flow;
 };
 
+/* A node within a next_ct_state list. */
+struct oftrace_next_ct_state {
+    struct ovs_list node;       /* In next_ct_states. */
+    uint32_t state;
+};
+
 void ofproto_dpif_trace_init(void);
 
 struct oftrace_node *oftrace_report(struct ovs_list *, enum oftrace_node_type,
diff --git a/ofproto/ofproto-unixctl.man b/ofproto/ofproto-unixctl.man
index 423837351910..e6c37894e980 100644
--- a/ofproto/ofproto-unixctl.man
+++ b/ofproto/ofproto-unixctl.man
@@ -6,10 +6,10 @@  These commands manage the core OpenFlow switch implementation (called
 Lists the names of the running ofproto instances.  These are the names
 that may be used on \fBofproto/trace\fR.
 .
-.IP "\fBofproto/trace\fR [\fIdpname\fR] \fIodp_flow\fR [\fB\-generate \fR| \fIpacket\fR]"
-.IQ "\fBofproto/trace\fR \fIbridge\fR \fIbr_flow\fR [\fB\-generate \fR| \fIpacket\fR]"
-.IQ "\fBofproto/trace\-packet\-out\fR [\fB\-consistent\fR] [\fIdpname\fR] \fIodp_flow\fR [\fB\-generate \fR| \fIpacket\fR] \fIactions\fR"
-.IQ "\fBofproto/trace\-packet\-out\fR [\fB\-consistent\fR] \fIbridge\fR \fIbr_flow\fR [\fB\-generate \fR| \fIpacket\fR] \fIactions\fR"
+.IP "\fBofproto/trace\fR [\fIdpname\fR] \fIodp_flow\fR [\fIOPTIONS\fR] [\fB\-generate \fR| \fIpacket\fR]"
+.IQ "\fBofproto/trace\fR \fIbridge\fR \fIbr_flow\fR [\fIOPTIONS\fR] [\fB\-generate \fR| \fIpacket\fR]"
+.IQ "\fBofproto/trace\-packet\-out\fR [\fB\-consistent\fR] [\fIdpname\fR] \fIodp_flow\fR [\fIOPTIONS\fR] [\fB\-generate \fR| \fIpacket\fR] \fIactions\fR"
+.IQ "\fBofproto/trace\-packet\-out\fR [\fB\-consistent\fR] \fIbridge\fR \fIbr_flow\fR [\fIOPTIONS\fR] [\fB\-generate \fR| \fIpacket\fR] \fIactions\fR"
 Traces the path of an imaginary packet through \fIswitch\fR and
 reports the path that it took.  The initial treatment of the packet
 varies based on the command:
@@ -48,6 +48,52 @@  wildcards.)  \fIbridge\fR names of the bridge through which
 .RE
 .
 .IP
+.RS
+\fBofproto/trace\fR supports the following options:
+.
+.IP "--ct-next \fIflags\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 comma- or
+space-separated list of the following connection tracking flags:
+.
+.RS
+.IP \(bu
+\fBtrk\fR: Include to indicate connection tracking has taken place.
+.
+.IP \(bu
+\fBnew\fR: Include to indicate a new flow.
+.
+.IP \(bu
+\fBest\fR: Include to indicate an established flow.
+.
+.IP \(bu
+\fBrel\fR: Include to indicate a related flow.
+.
+.IP \(bu
+\fBrpl\fR: Include to indicate a reply flow.
+.
+.IP \(bu
+\fBinv\fR: Include to indicate a connection entry in a bad state.
+.
+.IP \(bu
+\fBdnat\fR: Include to indicate a packet whose destination IP address has been
+changed.
+.
+.IP \(bu
+\fBsnat\fR: Include to indicate a packet whose source IP address has been
+changed.
+.
+.RE
+.
+.IP
+When --ct-next is unspecified, or when there are fewer --ct-next options that
+ct actions, the \fIflags\fR default to trk,est.
+.
+.RE
+.
+.IP
 Most commonly, one specifies only a flow, using one of the forms
 above, but sometimes one might need to specify an actual packet
 instead of just a flow:
diff --git a/tests/completion.at b/tests/completion.at
index e28c75239227..00e3a46b8bfa 100644
--- a/tests/completion.at
+++ b/tests/completion.at
@@ -171,7 +171,7 @@  AT_CLEANUP
 
 
 # complex completion check - ofproto/trace
-# ofproto/trace {[dp_name] odp_flow | bridge br_flow} [-generate|packet]
+# ofproto/trace {[dp_name] odp_flow | bridge br_flow} [OPTIONS] [-generate|packet]
 # test expansion on 'dp|dp_name' and 'bridge'
 AT_SETUP([appctl-bashcomp - complex completion check 3])
 AT_SKIP_IF([test -z ${BASH_VERSION+x}])
@@ -209,7 +209,8 @@  AT_CHECK_UNQUOTED([GET_AVAIL(${INPUT})], [0])
 # set odp_flow to some random string, should go to the next level.
 INPUT="$(bash ovs-appctl-bashcomp.bash debug ovs-appctl ofproto/trace ovs-dummy "in_port(123),mac(),ip,tcp" TAB 2>&1)"
 MATCH="$(GET_COMP_STR([-generate], [-generate])
-GET_COMP_STR([packet], []))"
+GET_COMP_STR([packet], [])
+GET_COMP_STR([OPTIONS...], []))"
 AT_CHECK_UNQUOTED([GET_EXPAN(${INPUT})],
 [0], [dnl
 ${MATCH}
@@ -242,7 +243,8 @@  AT_CHECK_UNQUOTED([GET_AVAIL(${INPUT})], [0])
 # set argument to some random string, should go to the odp_flow path.
 INPUT="$(bash ovs-appctl-bashcomp.bash debug ovs-appctl ofproto/trace "in_port(123),mac(),ip,tcp" TAB 2>&1)"
 MATCH="$(GET_COMP_STR([-generate], [-generate])
-GET_COMP_STR([packet], []))"
+GET_COMP_STR([packet], [])
+GET_COMP_STR([OPTIONS...], []))"
 AT_CHECK_UNQUOTED([GET_EXPAN(${INPUT})],
 [0], [dnl
 ${MATCH}
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index c51c689b9f46..142322afd777 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -5208,14 +5208,6 @@  Trailing garbage in packet data
 ovs-appctl: ovs-vswitchd: server returned an error
 ])
 
-# Test incorrect command: ofproto/trace with 4 arguments
-AT_CHECK([ovs-appctl ofproto/trace \
-  arg1, arg2, arg3, arg4], [2], [stdout],[stderr])
-AT_CHECK([tail -2 stderr], [0], [dnl
-"ofproto/trace" command takes at most 3 arguments
-ovs-appctl: ovs-vswitchd: server returned an error
-])
-
 # Test incorrect command: ofproto/trace with 0 argument
 AT_CHECK([ovs-appctl ofproto/trace ], [2], [stdout],[stderr])
 AT_CHECK([tail -2 stderr], [0], [dnl
@@ -9713,13 +9705,14 @@  AT_CLEANUP
 AT_SETUP([ofproto-dpif - conntrack - ofproto/trace])
 OVS_VSWITCHD_START
 
-add_of_ports br0 1 2
+add_of_ports br0 1 2 3 4
 
 AT_DATA([flows.txt], [dnl
 dnl Table 0
 dnl
 table=0,priority=100,arp,action=normal
 table=0,priority=10,udp,action=ct(table=1,zone=0)
+table=0,priority=10,tcp,action=ct(table=2,zone=1)
 table=0,priority=1,action=drop
 dnl
 dnl Table 1
@@ -9728,6 +9721,18 @@  table=1,priority=10,in_port=1,ct_state=+trk+new,udp,action=ct(commit,zone=0),2
 table=1,priority=10,in_port=1,ct_state=+trk+est,udp,action=2
 table=1,priority=10,in_port=2,ct_state=+trk+est,udp,action=1
 table=1,priority=1,action=drop
+dnl
+dnl Table 2
+dnl
+table=2,priority=10,in_port=1,tcp,ct_state=+trk+new,tcp,action=ct(commit,zone=1),ct(table=3,zone=2)
+table=2,priority=10,in_port=1,tcp,ct_state=+trk+est,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_state=+trk+new,tcp,action=ct(commit,zone=2),4
+table=3,priority=10,in_port=1,tcp,ct_state=+trk+est,tcp,action=3
+table=2,priority=1,action=drop
 ])
 
 AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
@@ -9742,6 +9747,16 @@  AT_CHECK([tail -1 stdout], [0],
   [Datapath actions: 2
 ])
 
+AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=1,tcp'], [0], [stdout])
+AT_CHECK([tail -1 stdout], [0],
+  [Datapath actions: 3
+])
+
+AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=1,tcp' --ct-next 'trk,new' --ct-next 'trk,new' ], [0], [stdout])
+AT_CHECK([tail -1 stdout], [0],
+  [Datapath actions: ct(commit,zone=2),4
+])
+
 OVS_VSWITCHD_STOP
 AT_CLEANUP