Thanks for the patch, looks good to me.
Tested-by: Yifeng Sun <pkusunyif...@gmail.com>
Reviewed-by: Yifeng Sun <pkusunyif...@gmail.com>
On Thu, Jan 18, 2018 at 2:45 PM, Ben Pfaff <b...@ovn.org> wrote:
> ofproto/trace takes a bunch of options that have weird placement and
> syntax. This commit changes the syntax so that the options can be placed
> anywhere and consistently use a double-dash option prefix. For
> compatibility, the previous syntax is also supported.
>
> An upcoming commit will add new options and this change allows that
> upcoming commit to be less confusing.
>
> Signed-off-by: Ben Pfaff <b...@ovn.org>
> ---
> ofproto/ofproto-dpif-trace.c | 194 +++++++++++++++++++++---------
> -------------
> ofproto/ofproto-unixctl.man | 42 +++++-----
> tests/ofproto-dpif.at | 2 +-
> 3 files changed, 120 insertions(+), 118 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c
> index 4999d1d6f326..75730155080c 100644
> --- a/ofproto/ofproto-dpif-trace.c
> +++ b/ofproto/ofproto-dpif-trace.c
> @@ -183,41 +183,11 @@ 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)
> -{
> - int k;
> - struct ds ds = DS_EMPTY_INITIALIZER;
> -
> - for (k = 0; k < argc; k++) {
> - if (!strncmp(argv[k], "--ct-next", 9)) {
> - if (k + 1 > argc) {
> - return xasprintf("Missing argument for option %s",
> argv[k]);
> - }
> -
> - uint32_t ct_state;
> - if (!parse_ct_state(argv[++k], 0, &ct_state, &ds)) {
> - return ds_steal_cstr(&ds);
> - }
> - if (!validate_ct_state(ct_state, &ds)) {
> - return ds_steal_cstr(&ds);
> - }
> - oftrace_push_ct_state(next_ct_states, ct_state);
> - } else {
> - return xasprintf("Invalid option %s", argv[k]);
> - }
> - }
> -
> - ds_destroy(&ds);
> - return NULL;
> -}
> -
> /* Parses the 'argc' elements of 'argv', ignoring argv[0]. The following
> * forms are supported:
> *
> - * - [dpname] odp_flow [OPTIONS] [-generate | packet]
> - * - bridge br_flow [OPTIONS] [-generate | packet]
> + * - [options] [dpname] odp_flow [packet]
> + * - [options] bridge br_flow [packet]
> *
> * On success, initializes '*ofprotop' and 'flow' and returns NULL. On
> failure
> * returns a nonnull malloced error message. */
> @@ -225,67 +195,109 @@ static char * OVS_WARN_UNUSED_RESULT
> parse_flow_and_packet(int argc, const char *argv[],
> struct ofproto_dpif **ofprotop, struct flow *flow,
> struct dp_packet **packetp,
> - struct ovs_list *next_ct_states)
> + struct ovs_list *next_ct_states,
> + bool *consistent)
> {
> const struct dpif_backer *backer = NULL;
> const char *error = NULL;
> char *m_err = NULL;
> struct simap port_names = SIMAP_INITIALIZER(&port_names);
> - struct dp_packet *packet;
> + struct dp_packet *packet = NULL;
> struct ofpbuf odp_key;
> struct ofpbuf odp_mask;
> - int first_option;
>
> ofpbuf_init(&odp_key, 0);
> ofpbuf_init(&odp_mask, 0);
>
> - /* Handle "-generate" or a hex string as the last argument. */
> - if (!strcmp(argv[argc - 1], "-generate")) {
> - packet = dp_packet_new(0);
> - argc--;
> - } else {
> - error = eth_from_hex(argv[argc - 1], &packet);
> - if (!error) {
> - argc--;
> - } else if (argc == 4) {
> - /* The 3-argument form must end in "-generate' or a hex
> string. */
> - goto exit;
> - }
> - error = NULL;
> + const char *args[3];
> + int n_args = 0;
> + bool generate_packet = false;
> + if (consistent) {
> + *consistent = false;
> }
> + for (int i = 1; i < argc; i++) {
> + const char *arg = argv[i];
> + if (!strcmp(arg, "-generate") || !strcmp(arg, "--generate")) {
> + generate_packet = true;
> + } else if (consistent
> + && (!strcmp(arg, "-consistent") ||
> + !strcmp(arg, "--consistent"))) {
> + *consistent = true;
> + } else if (!strcmp(arg, "--ct-next")) {
> + if (i + 1 >= argc) {
> + m_err = xasprintf("Missing argument for option %s", arg);
> + goto exit;
> + }
>
> - /* 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: invalid option";
> + uint32_t ct_state;
> + struct ds ds = DS_EMPTY_INITIALIZER;
> + if (!parse_ct_state(argv[++i], 0, &ct_state, &ds)
> + || !validate_ct_state(ct_state, &ds)) {
> + m_err = ds_steal_cstr(&ds);
> + goto exit;
> + }
> + oftrace_push_ct_state(next_ct_states, ct_state);
> + } else if (arg[0] == '-') {
> + m_err = xasprintf("%s: unknown option", arg);
> + goto exit;
> + } else if (n_args >= ARRAY_SIZE(args)) {
> + m_err = xstrdup("too many arguments");
> goto exit;
> + } else {
> + args[n_args++] = arg;
> }
> + }
>
> - m_err = parse_oftrace_options(argc - first_option, argv +
> first_option,
> - next_ct_states);
> - if (m_err) {
> + /* 'args' must now have one of the following forms:
> + *
> + * odp_flow
> + * dpname odp_flow
> + * bridge br_flow
> + * odp_flow packet
> + * dpname odp_flow packet
> + * bridge br_flow packet
> + *
> + * Parse the packet if it's there. Note that:
> + *
> + * - If there is one argument, there cannot be a packet.
> + *
> + * - If there are three arguments, there must be a packet.
> + *
> + * If there is a packet, we strip it off.
> + */
> + if (!generate_packet && n_args > 1) {
> + error = eth_from_hex(args[n_args - 1], &packet);
> + if (!error) {
> + n_args--;
> + } else if (n_args > 2) {
> + /* The 3-argument form must end in a hex string. */
> goto exit;
> }
> - argc = first_option;
> + error = NULL;
> }
>
> - /* 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(),
> - * we need to create a simap of name to port no. */
> - if (argc == 3) {
> + /* We stripped off the packet if there was one, so 'args' now has one
> of
> + * the following forms:
> + *
> + * odp_flow
> + * dpname odp_flow
> + * bridge br_flow
> + *
> + * Before we parse the flow, try to identify the backer, then use that
> + * backer to assemble a collection of port names. The port names are
> + * useful so that the user can specify ports by name instead of
> number in
> + * the flow. */
> + if (n_args == 2) {
> + /* args[0] might be dpname. */
> const char *dp_type;
> - if (!strncmp(argv[1], "ovs-", 4)) {
> - dp_type = argv[1] + 4;
> + if (!strncmp(args[0], "ovs-", 4)) {
> + dp_type = args[0] + 4;
> } else {
> - dp_type = argv[1];
> + dp_type = args[0];
> }
> backer = shash_find_data(&all_dpif_backers, dp_type);
> - } else if (argc == 2) {
> + } else if (n_args == 1) {
> + /* Pick default backer. */
> struct shash_node *node;
> if (shash_count(&all_dpif_backers) == 1) {
> node = shash_first(&all_dpif_backers);
> @@ -308,7 +320,7 @@ parse_flow_and_packet(int argc, const char *argv[],
> * bridge is specified. If function odp_flow_key_from_string()
> * returns 0, the flow is a odp_flow. If function
> * parse_ofp_exact_flow() returns NULL, the flow is a br_flow. */
> - if (!odp_flow_from_string(argv[argc - 1], &port_names,
> + if (!odp_flow_from_string(args[n_args - 1], &port_names,
> &odp_key, &odp_mask)) {
> if (!backer) {
> error = "Cannot find the datapath";
> @@ -349,14 +361,14 @@ parse_flow_and_packet(int argc, const char *argv[],
> } else {
> char *err;
>
> - if (argc != 3) {
> + if (n_args != 2) {
> error = "Must specify bridge name";
> goto exit;
> }
>
> - *ofprotop = ofproto_dpif_lookup_by_name(argv[1]);
> + *ofprotop = ofproto_dpif_lookup_by_name(args[0]);
> if (!*ofprotop) {
> - error = "Unknown bridge name";
> + m_err = xasprintf("%s: unknown bridge", args[0]);
> goto exit;
> }
>
> @@ -368,7 +380,7 @@ parse_flow_and_packet(int argc, const char *argv[],
> }
> err = parse_ofp_exact_flow(flow, NULL,
> ofproto_get_tun_tab(&(*ofprotop)->up),
> - argv[argc - 1], &map);
> + args[n_args - 1], &map);
> ofputil_port_map_destroy(&map);
> if (err) {
> m_err = xasprintf("Bad openflow flow syntax: %s", err);
> @@ -377,16 +389,15 @@ parse_flow_and_packet(int argc, const char *argv[],
> }
> }
>
> - /* Generate a packet, if requested. */
> - if (packet) {
> - if (!dp_packet_size(packet)) {
> - flow_compose(packet, flow, 0);
> - } else {
> - /* Use the metadata from the flow and the packet argument
> - * to reconstruct the flow. */
> - pkt_metadata_from_flow(&packet->md, flow);
> - flow_extract(packet, flow);
> - }
> + if (generate_packet) {
> + /* Generate a packet, as requested. */
> + packet = dp_packet_new(0);
> + flow_compose(packet, flow, 0);
> + } else if (packet) {
> + /* Use the metadata from the flow and the packet argument to
> + * reconstruct the flow. */
> + pkt_metadata_from_flow(&packet->md, flow);
> + flow_extract(packet, flow);
> }
>
> exit:
> @@ -423,7 +434,7 @@ ofproto_unixctl_trace(struct unixctl_conn *conn, int
> argc, const char *argv[],
> struct ovs_list next_ct_states = OVS_LIST_INITIALIZER(&next_ct_
> states);
>
> error = parse_flow_and_packet(argc, argv, &ofproto, &flow, &packet,
> - &next_ct_states);
> + &next_ct_states, NULL);
> if (!error) {
> struct ds result;
>
> @@ -471,19 +482,8 @@ ofproto_unixctl_trace_actions(struct unixctl_conn
> *conn, int argc,
> goto exit;
> }
>
> - /* OpenFlow 1.1 and later suggest that the switch enforces certain
> forms of
> - * consistency between the flow and the actions. With -consistent, we
> - * enforce consistency even for a flow supported in OpenFlow 1.0. */
> - if (!strcmp(argv[1], "-consistent")) {
> - enforce_consistency = true;
> - argv++;
> - argc--;
> - } else {
> - enforce_consistency = false;
> - }
> -
> error = parse_flow_and_packet(argc, argv, &ofproto, &match.flow,
> &packet,
> - &next_ct_states);
> + &next_ct_states, &enforce_consistency);
> if (error) {
> unixctl_command_reply_error(conn, error);
> free(error);
> diff --git a/ofproto/ofproto-unixctl.man b/ofproto/ofproto-unixctl.man
> index ee1f81fceaec..8f1d12c654ab 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 [\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"
> +.IP "\fBofproto/trace\fR [\fIoptions\fR] [\fIdpname\fR] \fIodp_flow\fR
> [\fIpacket\fR]
> +.IQ "\fBofproto/trace\fR [\fIoptions\fR] \fIbridge\fR \fIbr_flow\fR
> [\fIpacket\fR]]
> +.IQ "\fBofproto/trace\-packet\-out\fR [\fIoptions\fR] [\fIdpname\fR]
> \fIodp_flow\fR [\fIpacket\fR] \fIactions\fR"
> +.IQ "\fBofproto/trace\-packet\-out\fR [\fIoptions\fR \fIbridge\fR
> \fIbr_flow\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:
> @@ -49,7 +49,21 @@ wildcards.) \fIbridge\fR names of the bridge through
> which
> .
> .IP
> .RS
> -\fBofproto/trace\fR supports the following options:
> +These commands support the following options:
> +.IP \fB\-\-generate\fR
> +Generate a packet from the flow (see below for more information).
> +.
> +.IP \fB\-\-consistent\fR
> +Accepted by \fBofproto\-trace\-packet\-out\fR only. With this option,
> +the command rejects \fIactions\fR that are inconsistent with the
> +specified packet. (An example of an inconsistency is attempting to
> +strip the VLAN tag from a packet that does not have a VLAN tag.) Open
> +vSwitch ignores most forms of inconsistency in OpenFlow 1.0 and
> +rejects inconsistencies in later versions of OpenFlow. The option is
> +necessary because the command does not ordinarily imply a particular
> +OpenFlow version. One exception is that, when \fIactions\fR includes
> +an action that only OpenFlow 1.1 and later supports (such as
> +\fBpush_vlan\fR), \fB\-\-consistent\fR is automatically enabled.
> .
> .IP "--ct-next \fIflags\fR"
> When the traced flow triggers conntrack actions, \fBofproto/trace\fR
> @@ -124,12 +138,12 @@ If you wish to include a packet as part of a trace
> operation, there
> are two ways to do it:
> .
> .RS
> -.IP \fB\-generate\fR
> +.IP \fB\-\-generate\fR
> This option, added to one of the ways to specify a flow already
> described, causes Open vSwitch to internally generate a packet with
> the flow described and then to use that packet. If your goal is to
> -execute side effects, then \fB\-generate\fR is the easiest way to do
> -it, but \fB\-generate\fR is not a good way to fill in incomplete
> +execute side effects, then \fB\-\-generate\fR is the easiest way to do
> +it, but \fB\-\-generate\fR is not a good way to fill in incomplete
> information, because it generates packets based on only the flow
> information, which means that the packets really do not have any more
> information than the flow.
> @@ -169,18 +183,6 @@ The in_port value is kernel datapath port number for
> the first format
> and OpenFlow port number for the second format. The numbering of these
> two types of port usually differs and there is no relationship.
> .
> -.IP
> -\fBofproto\-trace\-packet\-out\fR accepts an additional
> -\fB\-consistent\fR option. With this option specified, the command
> -rejects \fIactions\fR that are inconsistent with the specified packet.
> -(An example of an inconsistency is attempting to strip the VLAN tag
> -from a packet that does not have a VLAN tag.) Open vSwitch ignores
> -most forms of inconsistency in OpenFlow 1.0 and rejects
> -inconsistencies in later versions of OpenFlow. The option is
> -necessary because the command does not ordinarily imply a particular
> -OpenFlow version. One exception is that, when \fIactions\fR includes
> -an action that only OpenFlow 1.1 and later supports (such as
> -\fBpush_vlan\fR), \fB\-consistent\fR is automatically enabled.
> .
> .IP "Usage examples:"
> .RS 4
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index a582aaf391b1..cbe0d91352fa 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -5302,7 +5302,7 @@ m4_foreach(
> [AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$br_flow" option],
> [2], [], [stderr])
> AT_CHECK([tail -2 stderr], [0], [dnl
> -Unknown bridge name
> +ovs-dummy: unknown bridge
> ovs-appctl: ovs-vswitchd: server returned an error
> ])])
>
> --
> 2.10.2
>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
