[ovs-dev] [PATCH 3/3] ofproto/trace: Add --ct-next option to ofproto/trace
Joe Stringer
joe at ovn.org
Wed Jun 21 20:02:31 UTC 2017
On 21 June 2017 at 11:06, Yi-Hung Wei <yihung.wei at 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 at 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!
More information about the dev
mailing list