On Thu, 2017-04-13 at 21:43 -0700, Ben Pfaff wrote:
> From: Stephen Finucane <[email protected]>
> 
> Let's start with a simple one that lets us focus on setting up most
> of
> the required "infrastructure" for building man pages using Sphinx.
> 
> This changes the 'check-htmldocs' target to 'check-docs' as its now
> responsible for building man page docs too.
> 
> Other than that, hurrah for (mostly) legible syntaxes.
> 
> [1] http://www.tldp.org/HOWTO/Man-Page/q2.html

This mostly looks good to me. Some comments below but nothing I'd block
on.

Acked-by: Stephen Finucane <[email protected]>

> Signed-off-by: Stephen Finucane <[email protected]>
> Signed-off-by: Ben Pfaff <[email protected]>
> ---
>  Documentation/automake.mk                          |  84
> +++++++++++++--
>  Documentation/conf.py                              |   5 +-
>  .../internals/contributing/documentation-style.rst |   3 +-
>  Documentation/intro/install/documentation.rst      |   4 +-
>  Documentation/ref/index.rst                        |  16 ++-
>  Documentation/ref/ovs-vlan-test.8.rst              | 115
> +++++++++++++++++++++
>  debian/openvswitch-switch.manpages                 |   1 -
>  manpages.mk                                        |  10 --
>  utilities/automake.mk                              |   3 -
>  utilities/ovs-vlan-test.8.in                       |  96 ---------
> --------
>  10 files changed, 211 insertions(+), 126 deletions(-)
>  create mode 100644 Documentation/ref/ovs-vlan-test.8.rst
>  delete mode 100644 utilities/ovs-vlan-test.8.in
> 
> diff --git a/Documentation/automake.mk b/Documentation/automake.mk
> index 9911668c1ca9..762255277102 100644
> --- a/Documentation/automake.mk
> +++ b/Documentation/automake.mk
> @@ -90,7 +90,8 @@ DOC_SOURCE = \
>       Documentation/internals/contributing/documentation-style.rst 
> \
>       Documentation/internals/contributing/libopenvswitch-abi.rst
> \
>       Documentation/internals/contributing/submitting-patches.rst
> \
> -     Documentation/requirements.txt
> +     Documentation/requirements.txt \
> +     $(addprefix Documentation/ref/,$(RST_MANPAGES))
>  
>  EXTRA_DIST += $(DOC_SOURCE)
>  
> @@ -110,20 +111,89 @@ sphinx_verbose_ = $(sphinx_verbose_@AM_DEFAULT_
> V@)
>  sphinx_verbose_0 = -q
>  
>  if HAVE_SPHINX
> -htmldocs-check: $(DOC_SOURCE)
> +docs-check: $(DOC_SOURCE)
>       $(AM_V_GEN)$(SPHINXBUILD) $(sphinx_verbose) -b html
> $(ALLSPHINXOPTS) $(SPHINXBUILDDIR)/html && touch $@
> -ALL_LOCAL += htmldocs-check
> -CLEANFILES += htmldocs-check
> +     $(AM_V_GEN)$(SPHINXBUILD) $(sphinx_verbose) -b man
> $(ALLSPHINXOPTS) $(SPHINXBUILDDIR)/man && touch $@
> +ALL_LOCAL += docs-check
> +CLEANFILES += docs-check
>  
>  check-docs:
>       $(SPHINXBUILD) -b linkcheck $(ALLSPHINXOPTS)
> $(SPHINXBUILDDIR)/linkcheck
>  
>  clean-docs:
> -     rm -rf $(SPHINXBUILDDIR)/doctrees
> -     rm -rf $(SPHINXBUILDDIR)/html
> -     rm -rf $(SPHINXBUILDDIR)/linkcheck
> +     rm -rf $(SPHINXBUILDDIR)
>       rm -f docs-check
>  CLEAN_LOCAL += clean-docs
>  endif
>  .PHONY: check-docs
>  .PHONY: clean-docs
> +

I might have done something funky applying this, but I can see a '^L'
character (linefeed?) on this line. Might be worth watching for.

> +# Installing manpages based on rST.
> +#
> +# The docs-check target converts the rST files listed in
> RST_MANPAGES
> +# into nroff manpages in Documentation/_build/man.  The easiest way
> to
> +# get these installed by "make install" is to write our own helper
> +# rules.
> +
> +# rST formatted manpages under Documentation/ref.
> +RST_MANPAGES = \
> +     ovs-vlan-test.8.rst
> +
> +# The GNU standards say that these variables should control
> +# installation directories for manpages in each section.  Automake
> +# will define them for us only if it sees that a manpage in the
> +# appropriate section is to be installed through its built-in
> feature.
> +# Since we're working independently, for best safety, we need to
> +# define them ourselves.
> +man1dir = $(mandir)/man1
> +man2dir = $(mandir)/man2
> +man3dir = $(mandir)/man3
> +man4dir = $(mandir)/man4
> +man5dir = $(mandir)/man5
> +man6dir = $(mandir)/man6
> +man7dir = $(mandir)/man7
> +man8dir = $(mandir)/man8
> +man9dir = $(mandir)/man9
> +
> +# Set a shell variable for each manpage directory.
> +set_mandirs = \
> +     man1dir='$(man1dir)' \
> +     man2dir='$(man2dir)' \
> +     man3dir='$(man3dir)' \
> +     man4dir='$(man4dir)' \
> +     man5dir='$(man5dir)' \
> +     man6dir='$(man6dir)' \
> +     man7dir='$(man7dir)' \
> +     man8dir='$(man8dir)' \
> +     man9dir='$(man9dir)'
> +
> +# Given an $rst of "ovs-vlan-test.8.rst", sets $stem to
> +# "ovs-vlan-test", $section to "8", and $mandir to $man8dir.
> +extract_stem_and_section = \
> +     stem=`echo "$$rst" | sed -n 's/^\(.*\)\.\([0-
> 9]\).rst$$/\1/p'`; \
> +     section=`echo "$$rst" | sed -n 's/^\(.*\)\.\([0-
> 9]\).rst$$/\2/p'`; \
> +     test -n "$$section" || { echo "$$rst: cannot infer manpage
> section from filename" 2>&1; continue; }; \
> +     eval "mandir=\$$man$${section}dir"; \
> +     test -n "$$mandir" || { echo "unknown directory for manpage
> section $$section"; continue; }
> +
> +if HAVE_SPHINX
> +INSTALL_DATA_LOCAL += install-man-rst
> +install-man-rst: docs-check
> +     @$(set_mandirs); \
> +     for rst in $(RST_MANPAGES); do \
> +         $(extract_stem_and_section); \
> +         echo " $(MKDIR_P) '$(DESTDIR)'\"$$mandir\""; \
> +         $(MKDIR_P) '$(DESTDIR)'"$$mandir"; \
> +         echo " $(INSTALL_DATA)
> $(SPHINXBUILDDIR)/man/$$stem.$$section
> '$(DESTDIR)'\"$$mandir/$$stem.$$section\""; \
> +         $(INSTALL_DATA) $(SPHINXBUILDDIR)/man/$$stem.$$section
> '$(DESTDIR)'"$$mandir/$$stem.$$section"; \
> +     done
> +endif
> +
> +UNINSTALL_LOCAL += uninstall-man-rst
> +uninstall-man-rst:
> +     @$(set_mandirs); \
> +     for rst in $(RST_MANPAGES); do \
> +         $(extract_stem_and_section); \
> +         echo "rm -f '$(DESTDIR)'\"$$mandir/$$stem.$$section\"";
> \
> +         rm -f '$(DESTDIR)'"$$mandir/$$stem.$$section"; \
> +     done

This whole section is still mostly noise to me and I would love to
simplify it somehow. I'm not ready to do that yet though and so long as
I don't have to maintain this (:)), I can live with it as-is. Might be
good to get someone else with more autotools know-how to look at it
though?

> diff --git a/Documentation/conf.py b/Documentation/conf.py
> index bfd7f33d88ba..8671a097d17e 100644
> --- a/Documentation/conf.py
> +++ b/Documentation/conf.py
> @@ -328,8 +328,9 @@ latex_documents = [
>  # One entry per manual page. List of tuples
>  # (source start file, name, description, authors, manual section).
>  man_pages = [
> -    (master_doc, 'openvswitch', u'Open vSwitch Documentation',
> -     [author], 1)
> +    ('ref/ovs-vlan-test.8', 'ovs-vlan-test',
> +     u'Check Linux drivers for problems with vlan traffic',
> +     [author], 8)
>  ]
>  
>  # If true, show URL addresses after external links.
> diff --git a/Documentation/internals/contributing/documentation-
> style.rst b/Documentation/internals/contributing/documentation-
> style.rst
> index 0ba5e544f14e..0e561ec23f10 100644
> --- a/Documentation/internals/contributing/documentation-style.rst
> +++ b/Documentation/internals/contributing/documentation-style.rst
> @@ -347,7 +347,8 @@ In addition to the above, man pages have some
> specific requirements:
>  - The man page must be included in the list of man page documents
> found in
>    `conf.py`__
>  
> -Refer to existing man pages for worked examples.
> +Refer to existing man pages, such as :doc:`/ref/ovs-vlan-test.8` for
> a worked
> +example.
>  
>  __ http://www.sphinx-doc.org/en/stable/domains.html#directive-progra
> m
>  __ http://www.sphinx-doc.org/en/stable/domains.html#directive-option
> diff --git a/Documentation/intro/install/documentation.rst
> b/Documentation/intro/install/documentation.rst
> index 94af950afc3e..0eeeab15226f 100644
> --- a/Documentation/intro/install/documentation.rst
> +++ b/Documentation/intro/install/documentation.rst
> @@ -74,11 +74,11 @@ Building
>  Once Sphinx installed, the documentation can be built using the
> provided
>  Makefile targets::
>  
> -    $ make htmldocs-check
> +    $ make docs-check
>  
>  .. important::
>  
> -   The ``htmldocs-check`` target will fail if there are any syntax
> errors.
> +   The ``docs-check`` target will fail if there are any syntax
> errors.
>     However, it won't catch more succint issues such as style or
> grammar issues.
>     As a result, you should always inspect changes visually to ensure
> the result
>     is as intended.
> diff --git a/Documentation/ref/index.rst
> b/Documentation/ref/index.rst
> index 49121b7599fd..6b368809c11b 100644
> --- a/Documentation/ref/index.rst
> +++ b/Documentation/ref/index.rst
> @@ -30,10 +30,18 @@ Reference Guide
>  Man Pages
>  ---------
>  
> -.. TODO(stephenfin): Investigate some way to get the manpages into
> rST format.
> -   The most viable option seems to be writing them all in rST,
> converting them
> -   to roff format and storing both the rST and roff formatted docs
> in version
> -   control.
> +.. TODO(stephenfin): Remove the below notice once everything is
> converted to
> +   rST
> +
> +The following man pages are written in rST and converted to roff at
> compile
> +time:
> +
> +.. toctree::
> +   :maxdepth: 3
> +
> +   ovs-vlan-test.8
> +
> +The remainder are still in roff format can be found below:
>  
>  .. list-table::
>  
> diff --git a/Documentation/ref/ovs-vlan-test.8.rst
> b/Documentation/ref/ovs-vlan-test.8.rst
> new file mode 100644
> index 000000000000..c3066917f00d
> --- /dev/null
> +++ b/Documentation/ref/ovs-vlan-test.8.rst
> @@ -0,0 +1,115 @@
> +=============
> +ovs-vlan-test
> +=============
> +
> +Synopsis
> +========
> +
> +**ovs-vlan-test** [**-s** | **--server**] *control_ip* *vlan_ip*

This results in better man page output at the expense of the HTML
output (which is no longer monospaced as you'd expect). The 'code-
block' directive would probably help here, but that causes issues for
some users. Guess we'll just live with it for now.

> +Description
> +===========
> +
> +.. TODO(stephenfin): Add the `:program:` prefixes to `ovs-test` once
> that doc
> +   is converted
> +
> +The :program:`ovs-vlan-test` utility has some limitations, for
> example, it does
> +not use TCP in its tests. Also it does not take into account MTU to
> detect
> +potential edge cases. To overcome those limitations a new tool was
> developed -
> +`ovs-test`. `ovs-test` is currently supported only on Debian so, if
> possible,
> +try to use that on instead of :program:`ovs-vlan-test`.
> +
> +The :program:`ovs-vlan-test` program may be used to check for
> problems sending
> +802.1Q traffic which may occur when running Open vSwitch. These
> problems can
> +occur when Open vSwitch is used to send 802.1Q traffic through
> physical
> +interfaces running certain drivers of certain Linux kernel versions.
> To run a
> +test, configure Open vSwitch to tag traffic originating from
> `vlan_ip` and
> +forward it out the target interface. Then run the :program:`ovs-
> vlan-test` in
> +client mode connecting to an :program:`ovs-vlan-test` server.
> +:program:`ovs-vlan-test` will display "OK" if it did not detect
> problems.
> +
> +Some examples of the types of problems that may be encountered are:
> +
> +- When NICs use VLAN stripping on receive they must pass a pointer
> to a
> +  `vlan_group` when reporting the stripped tag to the networking
> core. If no
> +  `vlan_group` is in use then some drivers just drop the extracted
> tag.
> +  Drivers are supposed to only enable stripping if a `vlan_group` is
> registered
> +  but not all of them do that.
> +
> +- On receive, some drivers handle priority tagged packets specially
> and don't
> +  pass the tag onto the network stack at all, so Open vSwitch never
> has a
> +  chance to see it.
> +
> +- Some drivers size their receive buffers based on whether a
> `vlan_group` is
> +  enabled, meaning that a maximum size packet with a VLAN tag will
> not fit if
> +  no `vlan_group` is configured.
> +
> +- On transmit, some drivers expect that VLAN acceleration will be
> used if it is
> +  available, which can only be done if a `vlan_group` is configured.
> In these
> +  cases, the driver may fail to parse the packet and correctly setup
> checksum
> +  offloading or TSO.
> +
> +Client Mode
> +  An :program:`ovs-vlan-test` client may be run on a host to check
> for VLAN
> +  connectivity problems. The client must be able to establish HTTP
> connections
> +  with an :program:`ovs-vlan-test` server located at the specified
> `control_ip`
> +  address. UDP traffic sourced at `vlan_ip` should be tagged and
> directed out
> +  the interface whose connectivity is being tested.
> +
> +Server Mode
> +  To conduct tests, an :program:`ovs-vlan-test` server must be
> running on a
> +  host known not to have VLAN connectivity problems. The server must
> have a
> +  `control_ip` on a non-VLAN network which clients can establish
> connectivity
> +  with. It must also have a `vlan_ip` address on a VLAN network
> which clients
> +  will use to test their VLAN connectivity. Multiple clients may
> test against a
> +  single :program:`ovs-vlan-test` server concurrently.
> +
> +Options
> +=======
> +
> +.. program:: ovs-vlan-test
> +
> +.. option:: -s, --server
> +
> +    Run in server mode.
> +
> +.. option:: -h, --help
> +
> +    Prints a brief help message to the console.
> +
> +.. option:: -V, --version
> +
> +    Prints version information to the console.
> +
> +Examples
> +========
> +
> +Display the Linux kernel version and driver of `eth1`::
> +
> +   uname -r
> +   ethtool -i eth1
> +
> +Set up a bridge which forwards traffic originating from `1.2.3.4`
> out `eth1`
> +with VLAN tag 10::
> +
> +    ovs-vsctl -- add-br vlan-br \
> +      -- add-port vlan-br eth1 \
> +      -- add-port vlan-br vlan-br-tag tag=10 \
> +      -- set Interface vlan-br-tag type=internal
> +    ifconfig vlan-br-tag up 1.2.3.4
> +
> +Run an :program:`ovs-vlan-test` server listening for client control
> traffic on
> +`172.16.0.142` port `8080` and VLAN traffic on the default port of
> `1.2.3.3`::
> +
> +    ovs-vlan-test -s 172.16.0.142:8080 1.2.3.3
> +
> +Run an :program:`ovs-vlan-test` client with a control server located
> at
> +`172.16.0.142` port `8080` and a local VLAN IP of `1.2.3.4`::
> +
> +    ovs-vlan-test 172.16.0.142:8080 1.2.3.4
> +
> +See Also
> +========
> +
> +`ovs-vswitchd(8)`, `ovs-ofctl(8)`, `ovs-vsctl(8)`, `ovs-test(8)`,
> `ethtool(8)`,
> +`uname(1)`
> diff --git a/debian/openvswitch-switch.manpages b/debian/openvswitch-
> switch.manpages
> index a3aae642d8d1..a2f661a3e58c 100644
> --- a/debian/openvswitch-switch.manpages
> +++ b/debian/openvswitch-switch.manpages
> @@ -5,7 +5,6 @@ utilities/ovs-dpctl.8
>  utilities/ovs-pcap.1
>  utilities/ovs-tcpdump.8
>  utilities/ovs-tcpundump.1
> -utilities/ovs-vlan-test.8
>  utilities/ovs-vsctl.8
>  vswitchd/ovs-vswitchd.8
>  vswitchd/ovs-vswitchd.conf.db.5
> diff --git a/manpages.mk b/manpages.mk
> index 742bd66cdc32..372d06cad0a4 100644
> --- a/manpages.mk
> +++ b/manpages.mk
> @@ -244,16 +244,6 @@ utilities/ovs-vlan-bug-workaround.8.in:
>  lib/common.man:
>  utilities/ovs-vlan-bugs.man:
>  
> -utilities/ovs-vlan-test.8: \
> -     utilities/ovs-vlan-test.8.in \
> -     lib/common-syn.man \
> -     lib/common.man \
> -     utilities/ovs-vlan-bugs.man
> -utilities/ovs-vlan-test.8.in:
> -lib/common-syn.man:
> -lib/common.man:
> -utilities/ovs-vlan-bugs.man:
> -
>  utilities/ovs-vsctl.8: \
>       utilities/ovs-vsctl.8.in \
>       lib/common.man \
> diff --git a/utilities/automake.mk b/utilities/automake.mk
> index 169388fda5ec..02531f6dd3d6 100644
> --- a/utilities/automake.mk
> +++ b/utilities/automake.mk
> @@ -72,7 +72,6 @@ MAN_ROOTS += \
>       utilities/ovs-tcpundump.1.in \
>       utilities/ovs-vlan-bug-workaround.8.in \
>       utilities/ovs-test.8.in \
> -     utilities/ovs-vlan-test.8.in \
>       utilities/ovs-vsctl.8.in
>  MAN_FRAGMENTS += utilities/ovs-vlan-bugs.man
>  CLEANFILES += \
> @@ -101,7 +100,6 @@ CLEANFILES += \
>       utilities/ovs-test \
>       utilities/ovs-test.8 \
>       utilities/ovs-vlan-test \
> -     utilities/ovs-vlan-test.8 \
>       utilities/ovs-vlan-bug-workaround.8 \
>       utilities/ovs-vsctl.8
>  
> @@ -120,7 +118,6 @@ man_MANS += \
>       utilities/ovs-tcpundump.1 \
>       utilities/ovs-vlan-bug-workaround.8 \
>       utilities/ovs-test.8 \
> -     utilities/ovs-vlan-test.8 \
>       utilities/ovs-vsctl.8
>  
>  utilities_ovs_appctl_SOURCES = utilities/ovs-appctl.c
> diff --git a/utilities/ovs-vlan-test.8.in b/utilities/ovs-vlan-
> test.8.in
> deleted file mode 100644
> index 2cdeff832104..000000000000
> --- a/utilities/ovs-vlan-test.8.in
> +++ /dev/null
> @@ -1,96 +0,0 @@
> -.de IQ
> -.  br
> -.  ns
> -.  IP "\\$1"
> -..
> -.TH ovs\-vlan\-test 1 "@VERSION@" "Open vSwitch" "Open vSwitch
> Manual"
> -.
> -.SH NAME
> -\fBovs\-vlan\-test\fR \- check Linux drivers for problems with vlan
> traffic
> -.
> -.SH SYNOPSIS
> -\fBovs\-vlan\-test\fR [\fB\-s\fR | \fB\-\-server\fR]
> \fIcontrol_ip\fR \fIvlan_ip\fR
> -.so lib/common-syn.man
> -.
> -.SH DESCRIPTION
> -The \fBovs\-vlan\-test\fR utility has some limitations, for example,
> it does
> -not use TCP in its tests. Also it does not take into account MTU to
> detect
> -potential edge cases. To overcome those limitations a new tool was
> -developed \- \fBovs\-test\fR. \fBovs\-test\fR is currently supported
> only
> -on Debian so, if possible try to use that on instead of \fBovs\-
> vlan\-test\fR.
> -.PP
> -The \fBovs\-vlan\-test\fR program may be used to check for problems
> sending
> -802.1Q traffic which may occur when running Open vSwitch. These
> problems can
> -occur when Open vSwitch is used to send 802.1Q traffic through
> physical
> -interfaces running certain drivers of certain Linux kernel versions.
> To run a
> -test, configure Open vSwitch to tag traffic originating from
> \fIvlan_ip\fR and
> -forward it out the target interface. Then run the \fBovs\-vlan\-
> test\fR in
> -client mode connecting to an \fBovs\-vlan\-test\fR server.
> -\fBovs\-vlan\-test\fR will display "OK" if it did not detect
> problems.
> -.PP
> -Some examples of the types of problems that may be encountered are:
> -.so utilities/ovs-vlan-bugs.man
> -.
> -.SS "Client Mode"
> -An \fBovs\-vlan\-test\fR client may be run on a host to check for
> VLAN
> -connectivity problems.  The client must be able to establish HTTP
> connections
> -with an \fBovs\-vlan\-test\fR server located at the specified
> \fIcontrol_ip\fR
> -address.  UDP traffic sourced at \fIvlan_ip\fR should be tagged and
> directed out
> -the interface whose connectivity is being tested.
> -.
> -.SS "Server Mode"
> -To conduct tests, an \fBovs\-vlan\-test\fR server must be running on
> a host
> -known not to have VLAN connectivity problems.  The server must have
> a
> -\fIcontrol_ip\fR on a non\-VLAN network which clients can establish
> -connectivity with.  It must also have a \fIvlan_ip\fR address on a
> VLAN network
> -which clients will use to test their VLAN connectivity.  Multiple
> clients may
> -test against a single \fBovs\-vlan\-test\fR server concurrently.
> -.
> -.SH OPTIONS
> -.
> -.IP "\fB\-s\fR"
> -.IQ "\fB\-\-server\fR"
> -Run in server mode.
> -.
> -.so lib/common.man
> -.SH EXAMPLES
> -Display the Linux kernel version and driver of \fBeth1\fR.
> -.IP
> -.B uname \-r
> -.IP
> -.B ethtool \-i eth1
> -.
> -.PP
> -Set up a bridge which forwards traffic originating from
> \fB1.2.3.4\fR out
> -\fBeth1\fR with VLAN tag 10.
> -.IP
> -.B ovs\-vsctl \-\- add\-br vlan\-br \(rs
> -.IP
> -.B \-\- add\-port vlan\-br eth1 \(rs
> -.IP
> -.B \-\- add\-port vlan\-br vlan\-br\-tag tag=10 \(rs
> -.IP
> -.B \-\- set Interface vlan\-br\-tag type=internal
> -.IP
> -.B ifconfig vlan\-br\-tag up 1.2.3.4
> -.
> -.PP
> -Run an \fBovs\-vlan\-test\fR server listening for client control
> traffic on
> -172.16.0.142 port 8080 and VLAN traffic on the default port of
> 1.2.3.3.
> -.IP
> -.B ovs\-vlan\-test \-s 172.16.0.142:8080 1.2.3.3
> -.
> -.PP
> -Run an \fBovs\-vlan\-test\fR client with a control server located at
> -172.16.0.142 port 8080 and a local VLAN ip of 1.2.3.4.
> -.IP
> -.B ovs\-vlan\-test 172.16.0.142:8080 1.2.3.4
> -.
> -.SH SEE ALSO
> -.
> -.BR ovs\-vswitchd (8),
> -.BR ovs\-ofctl (8),
> -.BR ovs\-vsctl (8),
> -.BR ovs\-test (8),
> -.BR ethtool (8),
> -.BR uname (1)

This is all fine by me.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to