From patchwork Thu Jan 18 22:45:12 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ben Pfaff X-Patchwork-Id: 863199 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=openvswitch.org (client-ip=140.211.169.12; helo=mail.linuxfoundation.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Received: from mail.linuxfoundation.org (mail.linuxfoundation.org [140.211.169.12]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3zMzYx3ghMz9sBd for ; Fri, 19 Jan 2018 09:45:25 +1100 (AEDT) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id 0B884E80; Thu, 18 Jan 2018 22:45:24 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@mail.linuxfoundation.org Received: from smtp1.linuxfoundation.org (smtp1.linux-foundation.org [172.17.192.35]) by mail.linuxfoundation.org (Postfix) with ESMTPS id 5597BE42 for ; Thu, 18 Jan 2018 22:45:23 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net [217.70.183.198]) by smtp1.linuxfoundation.org (Postfix) with ESMTPS id E8CAB51E for ; Thu, 18 Jan 2018 22:45:21 +0000 (UTC) X-Originating-IP: 208.91.3.26 Received: from sigabrt.benpfaff.org (unknown [208.91.3.26]) (Authenticated sender: blp@ovn.org) by relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 52A2AFB87D; Thu, 18 Jan 2018 23:45:18 +0100 (CET) From: Ben Pfaff To: dev@openvswitch.org Date: Thu, 18 Jan 2018 14:45:12 -0800 Message-Id: <20180118224515.4208-1-blp@ovn.org> X-Mailer: git-send-email 2.10.2 X-Spam-Status: No, score=-2.6 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on smtp1.linux-foundation.org Cc: Ben Pfaff Subject: [ovs-dev] [PATCH 1/4] ofproto-dpif-trace: Generalize syntax for ofproto/trace. X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.12 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Sender: ovs-dev-bounces@openvswitch.org Errors-To: ovs-dev-bounces@openvswitch.org 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 Tested-by: Yifeng Sun Reviewed-by: Yifeng Sun --- 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 ])])