On Wed, Oct 30, 2013 at 2:13 PM, Ben Pfaff <b...@nicira.com> wrote:
> Feature #20543.
> Requested-by: Ronghua Zhang <rzh...@vmware.com>
> Signed-off-by: Ben Pfaff <b...@nicira.com>
> ---
> ofproto/ofproto-dpif.c | 123
> ++++++++++++++++++++++++++++++++++++++-----
> ofproto/ofproto-unixctl.man | 57 +++++++++++++++-----
> tests/ofproto-dpif.at | 19 +++++++
> 3 files changed, 172 insertions(+), 27 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index d735507..c79570e 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -515,7 +515,9 @@ ofproto_dpif_cast(const struct ofproto *ofproto)
> static struct ofport_dpif *get_ofp_port(const struct ofproto_dpif *ofproto,
> ofp_port_t ofp_port);
> static void ofproto_trace(struct ofproto_dpif *, const struct flow *,
> - const struct ofpbuf *packet, struct ds *);
> + const struct ofpbuf *packet,
> + const struct ofpact[], size_t ofpacts_len,
> + struct ds *);
I think the above change in ofproto_trace definition should be in patch 1.
>
> /* Upcalls. */
> static void handle_upcalls(struct dpif_backer *);
> @@ -5278,31 +5280,118 @@ ofproto_unixctl_trace(struct unixctl_conn *conn, int
> argc, const char *argv[],
> }
>
> static void
> +ofproto_unixctl_trace_actions(struct unixctl_conn *conn, int argc,
> + const char *argv[], void *aux OVS_UNUSED)
> +{
> + enum ofputil_protocol usable_protocols;
> + struct ofproto_dpif *ofproto;
> + bool enforce_consistency;
> + struct ofpbuf ofpacts;
> + struct ofpbuf *packet;
> + struct ds result;
> + struct flow flow;
> + uint16_t in_port;
> +
> + /* Three kinds of error return values! */
> + enum ofperr retval;
> + const char *error;
> + char *rw_error;
> +
> + packet = NULL;
> + ds_init(&result);
> + ofpbuf_init(&ofpacts, 0);
> +
> + /* Parse actions. */
> + rw_error = parse_ofpacts(argv[--argc], &ofpacts, &usable_protocols);
> + if (rw_error) {
> + unixctl_command_reply_error(conn, rw_error);
> + free(rw_error);
> + goto exit;
> + }
> +
> + /* OpenFlow 1.1 and later suggest that the switch enforces certain forms
> of
> + * consistency between the flow and the actions, so enforce these by
> + * default if the actions can only work in OF1.1 or later. */
> + enforce_consistency = !(usable_protocols & OFPUTIL_P_OF10_ANY);
> + if (!strcmp(argv[1], "-consistent")) {
> + enforce_consistency = true;
> + argv++;
> + argc--;
> + }
> +
> + error = parse_flow_and_packet(argc, argv, &ofproto, &flow, &packet);
> + if (error) {
> + unixctl_command_reply_error(conn, error);
> + goto exit;
> + }
> +
> + /* Do the same checks as handle_packet_out() in ofproto.c.
> + *
> + * We pass a 'table_id' of 0 to ofproto_check_ofpacts(), which isn't
> + * strictly correct because these actions aren't in any table, but it's
> OK
> + * because it 'table_id' is used only to check goto_table instructions,
> but
> + * packet-outs take a list of actions and therefore it can't include
> + * instructions.
> + *
> + * We skip the "meter" check here because meter is an instruction, not an
> + * action, and thus cannot appear in ofpacts. */
> + in_port = ofp_to_u16(flow.in_port.ofp_port);
> + if (in_port >= ofproto->up.max_ports && in_port < ofp_to_u16(OFPP_MAX)) {
> + unixctl_command_reply_error(conn, "invalid in_port");
> + goto exit;
> + }
> + retval = ofpacts_check(ofpacts.data, ofpacts.size, &flow,
> + u16_to_ofp(ofproto->up.max_ports), 0,
> + enforce_consistency);
This patch needs a rebase as ofpacts_check interface has changed by a
later commit.
Otherrwise, This looks good to me.
> + if (retval) {
> + ds_clear(&result);
> + ds_put_format(&result, "Bad actions: %s", ofperr_to_string(retval));
> + unixctl_command_reply_error(conn, ds_cstr(&result));
> + goto exit;
> + }
> +
> + ofproto_trace(ofproto, &flow, packet, ofpacts.data, ofpacts.size,
> &result);
> + unixctl_command_reply(conn, ds_cstr(&result));
> +
> +exit:
> + ds_destroy(&result);
> + ofpbuf_delete(packet);
> + ofpbuf_uninit(&ofpacts);
> +}
> +
> +static void
> ofproto_trace(struct ofproto_dpif *ofproto, const struct flow *flow,
> - const struct ofpbuf *packet, struct ds *ds)
> + const struct ofpbuf *packet,
> + const struct ofpact ofpacts[], size_t ofpacts_len,
> + struct ds *ds)
It will be nice if this function has a comment at the top.
> {
> struct rule_dpif *rule;
> struct flow_wildcards wc;
>
> + ds_put_format(ds, "Bridge: %s\n", ofproto->up.name);
> ds_put_cstr(ds, "Flow: ");
> flow_format(ds, flow);
> ds_put_char(ds, '\n');
>
> flow_wildcards_init_catchall(&wc);
> - rule_dpif_lookup(ofproto, flow, &wc, &rule);
> + if (ofpacts) {
> + rule = NULL;
> + } else {
> + rule_dpif_lookup(ofproto, flow, &wc, &rule);
>
> - trace_format_rule(ds, 0, rule);
> - if (rule == ofproto->miss_rule) {
> - ds_put_cstr(ds, "\nNo match, flow generates \"packet in\"s.\n");
> - } else if (rule == ofproto->no_packet_in_rule) {
> - ds_put_cstr(ds, "\nNo match, packets dropped because "
> - "OFPPC_NO_PACKET_IN is set on in_port.\n");
> - } else if (rule == ofproto->drop_frags_rule) {
> - ds_put_cstr(ds, "\nPackets dropped because they are IP fragments "
> - "and the fragment handling mode is \"drop\".\n");
> + trace_format_rule(ds, 0, rule);
> + if (rule == ofproto->miss_rule) {
> + ds_put_cstr(ds, "\nNo match, flow generates \"packet in\"s.\n");
> + } else if (rule == ofproto->no_packet_in_rule) {
> + ds_put_cstr(ds, "\nNo match, packets dropped because "
> + "OFPPC_NO_PACKET_IN is set on in_port.\n");
> + } else if (rule == ofproto->drop_frags_rule) {
> + ds_put_cstr(ds, "\nPackets dropped because they are IP fragments
> "
> + "and the fragment handling mode is \"drop\".\n");
> + }
> }
>
> - if (rule) {
> + if (rule || ofpacts) {
> uint64_t odp_actions_stub[1024 / 8];
> struct ofpbuf odp_actions;
> struct trace_ctx trace;
> @@ -5315,6 +5404,10 @@ ofproto_trace(struct ofproto_dpif *ofproto, const
> struct flow *flow,
> ofpbuf_use_stub(&odp_actions,
> odp_actions_stub, sizeof odp_actions_stub);
> xlate_in_init(&trace.xin, ofproto, flow, rule, tcp_flags, packet);
> + if (ofpacts) {
> + trace.xin.ofpacts = ofpacts;
> + trace.xin.ofpacts_len = ofpacts_len;
> + }
> trace.xin.resubmit_hook = trace_resubmit;
> trace.xin.report_hook = trace_report;
>
> @@ -5750,6 +5843,10 @@ ofproto_dpif_unixctl_init(void)
> "ofproto/trace",
> "{[dp_name] odp_flow | bridge br_flow} [-generate|packet]",
> 1, 3, 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);
> unixctl_command_register("fdb/flush", "[bridge]", 0, 1,
> ofproto_unixctl_fdb_flush, NULL);
> unixctl_command_register("fdb/show", "bridge", 1, 1,
> diff --git a/ofproto/ofproto-unixctl.man b/ofproto/ofproto-unixctl.man
> index dd8e8d8..89013d9 100644
> --- a/ofproto/ofproto-unixctl.man
> +++ b/ofproto/ofproto-unixctl.man
> @@ -6,15 +6,30 @@ 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]"
> +.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"
> Traces the path of an imaginary packet through \fIswitch\fR and
> -reports the path that it took. The packet's headers (e.g. source and
> -destination) and metadata (e.g. input port), together called its
> -``flow,'' are usually all that matter for this purpose. You can
> -specify the flow in the following ways:
> +reports the path that it took. The initial treatment of the packet
> +varies based on the command:
> +.
> +.RS
> +.IP \(bu
> +\fBofproto/trace\fR looks the packet up in the OpenFlow flow table, as
> +if the packet had arrived on an OpenFlow port.
> +.
> +.IP \(bu
> +\fBofproto/trace\-packet\-out\fR applies the specified OpenFlow
> +\fIactions\fR, as if the packet, flow, and actions had been specified
> +in an OpenFlow ``packet-out'' request.
> +.RE
> +.
> +.IP
> +The packet's headers (e.g. source and destination) and metadata
> +(e.g. input port), together called its ``flow,'' are usually all that
> +matter for the purpose of tracing a packet. You can specify the flow
> +in the following ways:
> .
> .RS
> .IP "\fIdpname\fR \fIodp_flow\fR"
> @@ -41,13 +56,13 @@ instead of just a flow:
> .IP "Side effects."
> Some actions have side effects. For example, the \fBnormal\fR action
> can update the MAC learning table, and the \fBlearn\fR action can
> -change OpenFlow tables. \fBofproto/trace\fR only performs side
> +change OpenFlow tables. The trace commands only perform side
> effects when a packet is specified. If you want side effects to take
> place, then you must supply a packet.
> .
> .IP
> (Output actions are obviously side effects too, but
> -\fBofproto/trace\fR never executes them, even when one specifies a
> +the trace commands never execute them, even when one specifies a
> packet.)
> .
> .IP "Incomplete information."
> @@ -55,12 +70,12 @@ Most of the time, Open vSwitch can figure out everything
> about the
> path of a packet using just the flow, but in some special
> circumstances it needs to look at parts of the packet that are not
> included in the flow. When this is the case, and you do not supply a
> -packet, then \fBofproto/trace\fR will tell you it needs a packet.
> +packet, then a trace command will tell you it needs a packet.
> .RE
> .
> .IP
> -If you wish to include a packet as part of the \fBofproto/trace\fR
> -operation, there are two ways to do it:
> +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
> @@ -93,11 +108,25 @@ The tunnel ID on which the packet arrived.
> .IP \fIin_port\fR
> The port on which the packet arrived.
> .RE
> +.RE
> .
> +.IP
> 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.
> -.RE
> +.
> +.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 "\fBofproto/self\-check\fR [\fIswitch\fR]"
> Runs an internal consistency check on \fIswitch\fR, if specified,
> otherwise on all ofproto instances, and responds with a brief summary
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index 4ae1054..a81b757 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -1504,6 +1504,25 @@ ovs-appctl: ovs-vswitchd: server returned an error
> OVS_VSWITCHD_STOP
> AT_CLEANUP
>
> +AT_SETUP([ofproto-dpif - ofproto/trace-packet-out])
> +OVS_VSWITCHD_START
> +ADD_OF_PORTS([br0], 1, 2, 3)
> +
> +AT_DATA([flows.txt], [dnl
> +in_port=1 actions=output:2
> +in_port=2 actions=output:1
> +])
> +AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
> +
> +AT_CHECK([ovs-appctl ofproto/trace-packet-out br0 in_port=1
> 'mod_vlan_vid:123,resubmit(,0)'], [0], [stdout])
> +AT_CHECK([tail -1 stdout], [0], [dnl
> +Datapath actions: push_vlan(vid=123,pcp=0),2
> +])
> +
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> +
> +
> m4_define([OFPROTO_TRACE],
> [flow="$2"
> AT_CHECK([ovs-appctl ofproto/trace $1 "$flow" $3], [0], [stdout])
> --
> 1.7.10.4
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
