On Wed, Jun 21, 2017 at 1:02 PM, Joe Stringer <[email protected]> wrote:
> On 21 June 2017 at 11:06, Yi-Hung Wei <[email protected]> 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 <[email protected]>
>> ---
>> 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!
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
