Your return email address has a space in it. It appears as "Alex Wang <[email protected] >" (notice the space between ".com" and ">". I guess there may be a typo in your .gitconfig?
On Mon, May 06, 2013 at 05:30:51PM -0700, Alex Wang <[email protected] > wrote: > Subject: [ovs-dev] [PATCH 1/2] ofproto: Re-implement the ofproto/trace > command, fix broken tests and man page You don't need to mention that you fixed broken tests or the manpage in the subject, because it goes without saying. > From: Alex Wang <[email protected]> > > Since the use of single datapath, all bridges belonging to the same type of > datapath will use the same (single) datapath. This causes confusion in the > current 'ofproto/trace' command. Especially, when given the unrelated > 'bridge' and 'in_port' combination, the current implementation will still > be able to process and give misleading output. Thusly, this patch changes > the 'ofproto/trace' command syntax to formats shown as follow. > > ofproto/trace [datapath] priority tun_id in_port mark packet > ofproto/trace [datapath] dp_flow [-generate] > ofproto/trace bridge br_flow [-generate] > > Therein, the bridge name is replaced by datapath name in the first format, > since the mapping between in_port and the corresponding datapath is unique. > The second format requires datapath name since it uses 'ovs-dpctl dump-flows' > output. And the thrid format requires bridge name since it uses 'ovs-ofctl "thrid" =>" "third" > add-flow' input like flow. > > The following three tests are affected by the re-implemented ofproto/trace > command. And they are fixed in this patch. > > tests/learn.at > tests/tunnal.at > tests/ofproto-dpif.at > > Finally, this patch also fixes the man page so that it is consistent with > the new implementation. The above paragraphs are more detail than one really needs for the tests and manpage. If you want to say something anyway (sometimes I do too), something like, "Also, update tests and manpages to match." is enough. > Signed-off-by: Alex Wang <[email protected]> "git am" reports some whitespace errors: /home/blp/ovs/.git/rebase-apply/patch:274: trailing whitespace. Traces the path of an imaginary packet through \fiswitch\fR. The first /home/blp/ovs/.git/rebase-apply/patch:275: trailing whitespace. two forms require an optional \fIdatapath\fR argument, which is the datapath /home/blp/ovs/.git/rebase-apply/patch:276: trailing whitespace. the \fiswitch\fR is on (can be found by \fBdpif/show\fR command). If more /home/blp/ovs/.git/rebase-apply/patch:277: trailing whitespace. than one datapath exists, the \fIdatapath\fR argument is compulsory. /home/blp/ovs/.git/rebase-apply/patch:278: trailing whitespace. The third form requires a \fIswitch\fR argument (one of those listed by warning: squelched 10 whitespace errors warning: 15 lines add whitespace errors. > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index 6ec1c23..6550af3 100644 > --- a/ofproto/ofproto-dpif.c > +++ b/ofproto/ofproto-dpif.c > @@ -6073,7 +6073,7 @@ fix_sflow_action(struct action_xlate_ctx *ctx) > } > > cookie = ofpbuf_at(ctx->odp_actions, ctx->user_cookie_offset, > - sizeof cookie->sflow); > + sizeof(*cookie)); The above appears to revert the bug fix from commit a93f1927f21f2 (ofproto-dpif: Fix 'size' argument to fix_sflow_action().). Why does it do that? The rest of the patch looks like it implements what we asked you to do. Thank you. But, in retrospect, I am not sure that we asked you to do something that makes much sense. The high-level idea was that ofproto/trace should be easier to understand and to use, presumably by having a more uniform interface, but I think that the changes we asked you to make actually resulted in code and documentation that is harder to understand. I think that it would be better to step back a little and reconsider the syntax, so I did that. Here is a patch against the unmodified "master" version of ofproto-unixctl.man that gives a revised specification for ofproto/trace that appears, to me, to be easier to understand and more uniform. If you agree, then would you please use this patch instead for the documentation, and then update the code to match this documentation? And I am of course happy to iterate on the documentation in advance of updating the code, so that we end up with something better. Thanks, Ben. diff --git a/ofproto/ofproto-unixctl.man b/ofproto/ofproto-unixctl.man index 8890343..de19281 100644 *** a/ofproto/ofproto-unixctl.man --- b/ofproto/ofproto-unixctl.man *************** *** 6,75 **** Lists the names of the running ofproto instances. These are the names that may be used on \fBofproto/trace\fR. . ! .IP "\fBofproto/trace \fIswitch priority tun_id in_port mark packet\fR" ! .IQ "\fBofproto/trace \fIswitch flow \fB\-generate\fR" ! Traces the path of an imaginary packet through \fIswitch\fR. Both ! forms require \fIswitch\fR, the switch on which the packet arrived ! (one of those listed by \fBofproto/list\fR). The first form specifies ! a packet's contents explicitly: .RS ! .IP "\fIpriority\fR" ! Packet QoS priority. Use \fB0\fR if QoS is not setup. ! .IP "\fItun_id\fR" ! The tunnel ID on which the packet arrived. Use ! \fB0\fR if the packet did not arrive through a tunnel. ! .IP "\fIin_port\fR" ! The OpenFlow port on which the packet arrived. Use \fB65534\fR if the ! packet arrived on \fBOFPP_LOCAL\fR, the local port. ! .IP "\fImark\fR" ! SKB mark of the packet. Use \fB0\fR if Netfilter marks are not used. ! .IP "\fIpacket\fR" ! A sequence of hex digits specifying the packet's contents. An ! Ethernet frame is at least 14 bytes long, so there must be at least 28 ! hex digits. Obviously, it is inconvenient to type in the hex digits ! by hand, so the \fBovs\-pcap\fR(1) and \fBovs\-tcpundump\fR(1) ! utilities provide easier ways. .RE .IP ! The second form specifies the packet's contents implicitly: .RS ! .IP "\fIflow\fR" ! A flow in one of two forms: either the form printed by ! \fBovs\-dpctl\fR(8)'s \fBdump\-flows\fR command, or in a format ! similar to that accepted by \fBovs\-ofctl\fR(8)'s \fBadd\-flow\fR ! command. This is not an OpenFlow flow: besides other differences, it ! never contains wildcards. \fB\*(PN\fR generates an arbitrary packet ! that has the specified \fIflow\fR. ! .RE .IP ! \fB\*(PN\fR will respond with extensive information on how the packet ! would be handled if it were to be received. The packet will not ! actually be sent, but side effects such as MAC learning will occur. . ! .IP "\fBofproto/trace \fIswitch flow\fR" ! Traces the path of a packet in an imaginary flow through ! \fIswitch\fR. The arguments are: ! .RS ! .IP "\fIswitch\fR" ! The switch on which the packet arrived (one of those listed by ! \fBofproto/list\fR). ! .IP "\fIflow\fR" ! A flow in one of two forms: either the form printed by ! \fBovs\-dpctl\fR(8)'s \fBdump\-flows\fR command, or in a format ! similar to that accepted by \fBovs\-ofctl\fR(8)'s \fBadd\-flow\fR ! command. This is not an OpenFlow flow: besides other differences, it ! never contains wildcards. .RE .IP ! \fB\*(PN\fR will respond with extensive information on how a packet ! in \fIflow\fR would be handled if it were received by ! \fIswitch\fR. No packet will actually be sent. Some side effects may ! occur, but MAC learning in particular will not. .IP ! This form of \fBofproto/trace\fR cannot determine the complete set of ! datapath actions in some corner cases. If the results say that this ! is the case, rerun \fBofproto/trace\fR supplying a packet in the flow ! to get complete results. .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 --- 6,88 ---- Lists the names of the running ofproto instances. These are the names that may be used on \fBofproto/trace\fR. . ! .IP "\fBofproto/trace\fR [\fIdatapath\fR] \fIodp_flow\fR [\fB\-generate \fR| \fIpacket\fR] ! .IQ "\fBofproto/trace\fR \fIbridge\fR \fIbr_flow\fR [\fB\-generate \fR| \fIpacket\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: ! . .RS ! .IP "[\fIdatapath\fR] \fIodp_flow\fR" ! \fIodp_flow\fR is a flow in the form printed by \fBovs\-dpctl\fR(8)'s ! \fBdump\-flows\fR command. If all of your bridges have the same type, ! which is the common case, then you can omit \fIdatapath\fR, but if you ! have bridges of different types (say, both \fBnetdev\fR and ! \fBsystem\fR datapaths), then you need to specify a datapath name to ! disambiguate. ! . ! .IP "\fIbridge\fR \fIbr_flow\fR" ! \fIbr_flow\fR is a flow in the form similar to that accepted by ! \fBovs\-ofctl\fR(8)'s \fBadd\-flow\fR command. (This is not an ! OpenFlow flow: besides other differences, it never contains ! wildcards.) \fIbridge\fR names of the bridge through which ! \fIbr_flow\fR should be traced. .RE + . .IP ! Most commonly, one specifies only a flow, using one of the forms ! above, but sometimes one might need to specify an actual packet ! instead of just a flow: ! . .RS ! .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 ! 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 ! packet.) . ! .IP "Incomplete information." ! 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. .RE + . .IP ! If you wish to include a packet as part of the \fBofproto/trace\fR ! operation, there are two ways to do it: ! . ! .RS ! .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 ! 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. ! . ! .IP \fIpacket\fR ! This form supplies an explicit \fIpacket\fR as a sequence of hex ! digits. An Ethernet frame is at least 14 bytes long, so there must be ! at least 28 hex digits. Obviously, it is inconvenient to type in the ! hex digits by hand, so the \fBovs\-pcap\fR(1) and ! \fBovs\-tcpundump\fR(1) utilities provide easier ways. .IP ! With this form, packet headers are extracted directly from ! \fIpacket\fR, so the \fIodp_flow\fR or \fIbr_flow\fR should specify ! only metadata. ! .RE .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 _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
