[ovs-dev] dpctl: add examples to the manpage

On 21 March 2017 at 13:32, Aaron Conole <aconole@redhat.com> wrote:
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> ---

Hi Aaron, thanks for the patch. Feedback below, I can apply if you're
happy with it.

>  lib/dpctl.man | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
>
> diff --git a/lib/dpctl.man b/lib/dpctl.man
> index 2fcbc94..c5fdaa7 100644
> --- a/lib/dpctl.man
> +++ b/lib/dpctl.man
> @@ -136,7 +136,42 @@ With \fB\-\-clear\fR, \fBmod\-flow\fR zeros out the flow's
>  statistics.  The statistics printed if \fB\-s\fR or
>  \fB\-\-statistics\fR is also specified are those from just before
>  clearing the statistics.
> +.IP "NOTE:"
> +\fIflow\fR and \fIactions\fR do not match the syntax used with the
> +\fBovs\-ofctl\fR \fBadd\-flow\fR command.
>  .
> +.IP "Usage Examples:"
> +.RS 4
> +.PP
> +\fBForward ARP between ports 3 and 4 on datapath myDP\fR
> +.RS 4
> +.nf
> +ovs-dpctl add-flow myDP \\
> +.
> +  "in_port(3),eth(),eth_type(0x0806),arp()" 4
> +.
> +ovs-dpctl add-flow myDP \\
> +.
> +  "in_port(4),eth(),eth_type(0x0806),arp()" 3

It seems that ports 3 and 4 are used here, but 1 and 2 are used in the
later example.

There's also a bunch of unusual formatting here, things like
specifically indenting a certain distance using ".RS 4", and a few man
directives I have no familiarity with.

I tried to make this more consistent with the rest of the manpages in
OVS, correct indentation, and so on. Here's the incremental:

---8<---

Deletes the flow from \fIdp\fR's flow table that matches \fIflow\fR.

Joe Stringer <joe@ovn.org> writes:

> On 21 March 2017 at 13:32, Aaron Conole <aconole@redhat.com> wrote:
>> Signed-off-by: Aaron Conole <aconole@redhat.com>
>> ---
>
> Hi Aaron, thanks for the patch. Feedback below, I can apply if you're
> happy with it.

LGTM, thanks!

>>  lib/dpctl.man | 35 +++++++++++++++++++++++++++++++++++
>>  1 file changed, 35 insertions(+)
>>
>> diff --git a/lib/dpctl.man b/lib/dpctl.man
>> index 2fcbc94..c5fdaa7 100644
>> --- a/lib/dpctl.man
>> +++ b/lib/dpctl.man
>> @@ -136,7 +136,42 @@ With \fB\-\-clear\fR, \fBmod\-flow\fR zeros out the flow's
>>  statistics.  The statistics printed if \fB\-s\fR or
>>  \fB\-\-statistics\fR is also specified are those from just before
>>  clearing the statistics.
>> +.IP "NOTE:"
>> +\fIflow\fR and \fIactions\fR do not match the syntax used with the
>> +\fBovs\-ofctl\fR \fBadd\-flow\fR command.
>>  .
>> +.IP "Usage Examples:"
>> +.RS 4
>> +.PP
>> +\fBForward ARP between ports 3 and 4 on datapath myDP\fR
>> +.RS 4
>> +.nf
>> +ovs-dpctl add-flow myDP \\
>> +.
>> +  "in_port(3),eth(),eth_type(0x0806),arp()" 4
>> +.
>> +ovs-dpctl add-flow myDP \\
>> +.
>> +  "in_port(4),eth(),eth_type(0x0806),arp()" 3
>
> It seems that ports 3 and 4 are used here, but 1 and 2 are used in the
> later example.
>
> There's also a bunch of unusual formatting here, things like
> specifically indenting a certain distance using ".RS 4", and a few man
> directives I have no familiarity with.
>
> I tried to make this more consistent with the rest of the manpages in
> OVS, correct indentation, and so on. Here's the incremental:
>
> ---8<---
>
> diff --git a/lib/dpctl.man b/lib/dpctl.man
> index c5fdaa7e8bbd..f7ae311b90cc 100644
> --- a/lib/dpctl.man
> +++ b/lib/dpctl.man
> @@ -136,42 +136,43 @@ With \fB\-\-clear\fR, \fBmod\-flow\fR zeros out the flow's
> statistics.  The statistics printed if \fB\-s\fR or
> \fB\-\-statistics\fR is also specified are those from just before
> clearing the statistics.
> -.IP "NOTE:"
> -\fIflow\fR and \fIactions\fR do not match the syntax used with the
> -\fBovs\-ofctl\fR \fBadd\-flow\fR command.
> +.IP
> +NOTE:
> +\fIflow\fR and \fIactions\fR do not match the syntax used with
> +\fBovs\-ofctl\fR(8)'s \fBadd\-flow\fR command.
> +.
> +.IP
> +\fBUsage Examples\fR
> .
> -.IP "Usage Examples:"
> -.RS 4
> +.RS
> .PP
> -\fBForward ARP between ports 3 and 4 on datapath myDP\fR
> -.RS 4
> -.nf
> +Forward ARP between ports 1 and 2 on datapath myDP:
> +.IP
> ovs-dpctl add-flow myDP \\
> .
> -  "in_port(3),eth(),eth_type(0x0806),arp()" 4
> +  "in_port(1),eth(),eth_type(0x0806),arp()" 2
> .
> +.IP
> ovs-dpctl add-flow myDP \\
> .
> -  "in_port(4),eth(),eth_type(0x0806),arp()" 3
> +  "in_port(2),eth(),eth_type(0x0806),arp()" 1
> .
> -.RE
> -.fi
> .PP
> -\fBForward all IPv4 traffic between two addresses on ports 1 and 2\fR
> -.RS 4
> -.nf
> +Forward all IPv4 traffic between two addresses on ports 1 and 2:
> +.
> +.IP
> ovs-dpctl add-flow myDP \\
> .
>   "in_port(1),eth(),eth_type(0x800),\\
>     ipv4(src=172.31.110.4,dst=172.31.110.5)" 2
> .
> +.IP
> ovs-dpctl add-flow myDP \\
> .
>   "in_port(2),eth(),eth_type(0x800),\\
>    ipv4(src=172.31.110.5,dst=172.31.110.4)" 1
> .
> .RE
> -.RE
> .TP
> .DO "[\fB\-s\fR | \fB\-\-statistics\fR]" "\*(DX\fBdel\-flow\fR"
> "[\fIdp\fR] \fIflow\fR"
> Deletes the flow from \fIdp\fR's flow table that matches \fIflow\fR.

On 13 April 2017 at 13:30, Aaron Conole <aconole@redhat.com> wrote:
> Joe Stringer <joe@ovn.org> writes:
>
>> On 21 March 2017 at 13:32, Aaron Conole <aconole@redhat.com> wrote:
>>> Signed-off-by: Aaron Conole <aconole@redhat.com>
>>> ---
>>
>> Hi Aaron, thanks for the patch. Feedback below, I can apply if you're
>> happy with it.
>
> LGTM, thanks!

Thanks, applied to master.