Thanks for the review and for the fixes! Daniele
On 8/4/14, 2:31 PM, "Ben Pfaff" <b...@nicira.com> wrote: >On Thu, Jul 17, 2014 at 05:26:00PM -0700, Daniele Di Proietto wrote: >> This commit introduces multiple appctl commands (dpctl/*) >> >> They are needed to interact with userspace datapaths (dpif-netdev), >>because the >> ovs-dpctl command runs in a separate process and cannot see the >>userspace >> datapaths inside vswitchd. >> >> This change moves most of the code of utilities/ovs-dpctl.c in >>lib/dpctl.c. >> >> Both the ovs-dpctl command and the ovs-appctl dpctl/* commands make >>calls to >> lib/dpctl.c functions, to interact with datapaths. >> >> The code from utilities/ovs-dpctl.c has been moved to lib/dpctl.c and >>has been >> changed for different reasons: >> - An exit() call in the old code made perfectly sense. Now (since >>the code >> can be run inside vswitchd) it would terminate the daemon. Same >>reasoning >> can be applied to ovs_fatal_*() calls. >> - The lib/dpctl.c code _should_ not leak memory. >> - All the print* have been replaced with a function pointer provided >>by the >> caller, since this code can be run in the ovs-dpctl process (in >>which >> case we need to print to stdout) or in response to a unixctl >>request (and >> in this case we need to send everything through a socket, using >>JSON >> encapsulation). >> >> The syntax is >> ovs-appctl dpctl/(COMMAND) [OPTIONS] [PARAMETERS] >> while the ovs-dpctl syntax (which _should_ remain the same after this >>change) >> is >> ovs-dpctl [OPTIONS] (COMMAND) [PARAMETERS] >> >> Signed-off-by: Daniele Di Proietto <ddiproie...@vmware.com> > >I'm happy with this. I folded in the following minor changes and I'll >apply this to master in a minute. > >diff --git a/NEWS b/NEWS >index 742ae22..bf7eb2f 100644 >--- a/NEWS >+++ b/NEWS >@@ -8,6 +8,10 @@ Post-v2.3.0 > release. The protocol is documented at > >https://urldefense.proofpoint.com/v1/url?u=http://tools.ietf.org/html/draf >t-gross-geneve-00&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=MV9BdLjtFIdhBDBaw5 >z%2BU6SSA2gAfY4L%2F1HCy3VjlKU%3D%0A&m=UnG1y54Wx9Bs4iOLlUJN8aYOQyakvbhFglUN >PGlhQ1A%3D%0A&s=9d7ac58ffcf650ccd68af12f749d89fc7f187f3d8d83edd7874ae8d736 >d457a8 > - The OVS database now reports controller rate limiting statistics. >+ - ovs-dpctl functionality is now available for datapaths integrated >+ into ovs-vswitchd, via ovs-appctl. Some existing ovs-appctl >+ commands are now redundant and will be removed in a future >+ release. See ovs-vswitchd(8) for details. > - OpenFlow: > * OpenFlow 1.5 (draft) extended registers are now supported. > >diff --git a/lib/dpctl.c b/lib/dpctl.c >index 17d8b56..623f5b1 100644 >--- a/lib/dpctl.c >+++ b/lib/dpctl.c >@@ -92,17 +92,17 @@ dpctl_error(struct dpctl_params* dpctl_p, int err_no, >const char *fmt, ...) > } > ds_put_cstr(&ds, "\n"); > >- dpctl_puts(dpctl_p, true, ds_cstr(&ds)); >+ dpctl_puts(dpctl_p, true, ds_cstr(&ds)); > > ds_destroy(&ds); > > errno = save_errno; > } > ? >-static int dpctl_add_if(int argc, const char *argv[], >- struct dpctl_params *dpctl_p); >+static int dpctl_add_if(int argc, const char *argv[], struct >dpctl_params *); > >-static int if_up(struct netdev *netdev) >+static int >+if_up(struct netdev *netdev) > { > return netdev_turn_flags_on(netdev, NETDEV_UP, NULL); > } >@@ -1154,7 +1154,7 @@ dpctl_normalize_actions(int argc, const char >*argv[], > ds_clear(&s); > odp_flow_format(ofpbuf_data(&keybuf), ofpbuf_size(&keybuf), NULL, 0, >NULL, > &s, dpctl_p->verbosity); >- printf("input flow: %s\n", ds_cstr(&s)); >+ dpctl_print(dpctl_p, "input flow: %s\n", ds_cstr(&s)); > > error = odp_flow_key_to_flow(ofpbuf_data(&keybuf), >ofpbuf_size(&keybuf), > &flow); >@@ -1221,7 +1221,7 @@ dpctl_normalize_actions(int argc, const char >*argv[], > vlan_tci_to_vid(af->flow.vlan_tci), > vlan_tci_to_pcp(af->flow.vlan_tci)); > } else { >- printf("no vlan: "); >+ dpctl_print(dpctl_p, "no vlan: "); > } > > if (eth_type_mpls(af->flow.dl_type)) { >@@ -1289,8 +1289,8 @@ static const struct dpctl_command all_commands[] = { > /* Runs the command designated by argv[0] within the command table >specified by > * 'commands', which must be terminated by a command whose 'name' member >is a > * null pointer. */ >-int dpctl_run_command(int argc, const char *argv[], >- struct dpctl_params *dpctl_p) >+int >+dpctl_run_command(int argc, const char *argv[], struct dpctl_params >*dpctl_p) > { > const struct dpctl_command *p; > >diff --git a/lib/dpctl.h b/lib/dpctl.h >index f97b7b7..41186f6 100644 >--- a/lib/dpctl.h >+++ b/lib/dpctl.h >@@ -21,13 +21,15 @@ > #include "compiler.h" > > struct dpctl_params { >- /* Options */ > /* -s, --statistics: Print port/flow statistics? */ > bool print_statistics; >+ > /* --clear: Reset existing statistics to zero when modifying a flow? >*/ > bool zero_statistics; >+ > /* --may-create: Allow mod-flows command to create a new flow? */ > bool may_create; >+ > /* -m, --more: Increase output verbosity. */ > int verbosity; > >diff --git a/lib/dpctl.man b/lib/dpctl.man >index 98cd8af..c678576 100644 >--- a/lib/dpctl.man >+++ b/lib/dpctl.man >@@ -3,7 +3,7 @@ > Creates datapath \fIdp\fR, with a local port also named \fIdp\fR. > This will fail if a network device \fIdp\fR already exists. > .IP >-If \fInetdev\fRs are specified, \fBovs\-dpctl\fR adds them to the >+If \fInetdev\fRs are specified, \fB\*(PN\fR adds them to the > new datapath, just as if \fBadd\-if\fR was specified. > . > .TP >@@ -82,9 +82,9 @@ visited per packet; the ratio between "hit" and total >number of > packets processed by the datapath". > .IP > If one or more datapaths are specified, information on only those >-datapaths are displayed. Otherwise, \fBovs\-dpctl\fR displays >information >+datapaths are displayed. Otherwise, \fB\*(PN\fR displays information > about all configured datapaths. >-.SS "DPCTL DEBUGGING COMMANDS" >+.SS "DATAPATH FLOW TABLE DEBUGGING COMMANDS" > The following commands are primarily useful for debugging Open > vSwitch. The flow table entries (both matches and actions) that they > work with are not OpenFlow flow entries. Instead, they are different >diff --git a/ofproto/ofproto-dpif-unixctl.man >b/ofproto/ofproto-dpif-unixctl.man >index 14e9ff2..bbf7fbb 100644 >--- a/ofproto/ofproto-dpif-unixctl.man >+++ b/ofproto/ofproto-dpif-unixctl.man >@@ -1,6 +1,9 @@ >-.SS "DATAPATH COMMANDS" >-These commands manage logical datapaths. They are are similar to the >-equivalent \fBovs\-dpctl\fR commands. >+.SS "DATAPATH DEBUGGING COMMANDS" >+These commands query and modify datapaths. They are are similar to >+\fBovs\-dpctl\fR(8) commands. \fBdpif/show\fR has the additional >+functionality, beyond \fBdpctl/show\fR of printing OpenFlow port >+numbers. The other commands are redundant and will be removed in a >+future release. > . > .IP "\fBdpif/dump\-dps\fR" > Prints the name of each configured datapath on a separate line. >diff --git a/vswitchd/ovs-vswitchd.8.in b/vswitchd/ovs-vswitchd.8.in >index d58b0d5..0fd13e3 100644 >--- a/vswitchd/ovs-vswitchd.8.in >+++ b/vswitchd/ovs-vswitchd.8.in >@@ -216,12 +216,17 @@ whether it is attached or detached, port id and >priority, actor > information, and partner information. If \fIport\fR is not specified, > then displays detailed information about all interfaces with CFM > enabled. >-.SS "DPCTL COMMANDS" >-These commands can be used to manage datapaths. They implement the same >-features (and syntax) as \fBovs\-dpctl\fR commands, but they work also >with >-non-kernel datapaths (dpif-netdev), which \fBovs\-dpctl\fR cannot >control. >+.SS "DPCTL DATAPATH DEBUGGING COMMANDS" >+The primary way to configure \fBovs\-vswitchd\fR is through the Open >+vSwitch database, e.g. using \fBovs\-vsctl\fR(8). These commands >+provide a debugging interface for managing datapaths. They implement >+the same features (and syntax) as \fBovs\-dpctl\fR(8). Unlike >+\fBovs\-dpctl\fR(8), these commands work with datapaths that are >+integrated into \fBovs\-vswitchd\fR (e.g. the \fBnetdev\fR datapath >+type). >+.PP > . >-.ds DX "\fBdpctl/\fR >+.ds DX \fBdpctl/\fR > .de DO > \\$2 \\$1 \\$3 > .. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
