On Mon, Sep 23, 2024 at 02:54:18PM GMT, Adrián Moreno wrote:
> On Mon, Sep 23, 2024 at 01:00:50PM GMT, Ilya Maximets wrote:
> > On 9/23/24 12:39, Adrián Moreno wrote:
> > > On Tue, Aug 20, 2024 at 12:15:52AM GMT, Ilya Maximets wrote:
> > >> On 7/10/24 19:05, Adrian Moreno wrote:
> > >>> Add a man page for ovs-flowviz as well as a topic page with some more
> > >>> detailed examples.
> > >>>
> > >>> Signed-off-by: Adrian Moreno <amore...@redhat.com>
> > >>> ---
> > >>> Documentation/automake.mk | 4 +-
> > >>> Documentation/conf.py | 2 +
> > >>> Documentation/ref/index.rst | 1 +
> > >>> Documentation/ref/ovs-flowviz.8.rst | 535 ++++++++++++++++++++
> > >>> Documentation/topics/flow-visualization.rst | 314 ++++++++++++
> > >>> Documentation/topics/index.rst | 1 +
> > >>> rhel/openvswitch-fedora.spec.in | 1 +
> > >>> rhel/openvswitch.spec.in | 1 +
> > >>> 8 files changed, 858 insertions(+), 1 deletion(-)
> > >>> create mode 100644 Documentation/ref/ovs-flowviz.8.rst
> > >>> create mode 100644 Documentation/topics/flow-visualization.rst
> > >>
> > >> Hi, Adrian. Thanks for the update! I didn't make a full review, since
> > >> Eelco did, but I have a couple comments for the docs and the output
> > >> format below.
> > >>
> > >> Best regards, Ilya Maximets.
> > >>
> > >>>
> > >>> diff --git a/Documentation/automake.mk b/Documentation/automake.mk
> > >>> index 47d2e336a..539870aa2 100644
> > >>> --- a/Documentation/automake.mk
> > >>> +++ b/Documentation/automake.mk
> > >>> @@ -45,7 +45,7 @@ DOC_SOURCE = \
> > >>> Documentation/topics/fuzzing/ovs-fuzzing-infrastructure.rst \
> > >>> Documentation/topics/fuzzing/ovs-fuzzers.rst \
> > >>>
> > >>> Documentation/topics/fuzzing/security-analysis-of-ovs-fuzzers.rst \
> > >>> - Documentation/topics/testing.rst \
> > >>> + Documentation/topics/flow-visualization.rst \
> > >>> Documentation/topics/integration.rst \
> > >>> Documentation/topics/language-bindings.rst \
> > >>> Documentation/topics/networking-namespaces.rst \
> > >>> @@ -55,6 +55,7 @@ DOC_SOURCE = \
> > >>> Documentation/topics/ovsdb-replication.rst \
> > >>> Documentation/topics/porting.rst \
> > >>> Documentation/topics/record-replay.rst \
> > >>> + Documentation/topics/testing.rst \
> > >>> Documentation/topics/tracing.rst \
> > >>> Documentation/topics/usdt-probes.rst \
> > >>> Documentation/topics/userspace-checksum-offloading.rst \
> > >>> @@ -162,6 +163,7 @@ RST_MANPAGES = \
> > >>> ovs-actions.7.rst \
> > >>> ovs-appctl.8.rst \
> > >>> ovs-ctl.8.rst \
> > >>> + ovs-flowviz.8.rst \
> > >>> ovs-l3ping.8.rst \
> > >>> ovs-parse-backtrace.8.rst \
> > >>> ovs-pki.8.rst \
> > >>> diff --git a/Documentation/conf.py b/Documentation/conf.py
> > >>> index 15785605a..3a82f23a7 100644
> > >>> --- a/Documentation/conf.py
> > >>> +++ b/Documentation/conf.py
> > >>> @@ -120,6 +120,8 @@ _man_pages = [
> > >>> u'utility for configuring running Open vSwitch daemons'),
> > >>> ('ovs-ctl.8',
> > >>> u'OVS startup helper script'),
> > >>> + ('ovs-flowviz.8',
> > >>> + u'utility for visualizing OpenFlow and datapath flows'),
> > >>> ('ovs-l3ping.8',
> > >>> u'check network deployment for L3 tunneling problems'),
> > >>> ('ovs-parse-backtrace.8',
> > >>> diff --git a/Documentation/ref/index.rst b/Documentation/ref/index.rst
> > >>> index 03ada932f..7f2fe6177 100644
> > >>> --- a/Documentation/ref/index.rst
> > >>> +++ b/Documentation/ref/index.rst
> > >>> @@ -42,6 +42,7 @@ time:
> > >>> ovs-actions.7
> > >>> ovs-appctl.8
> > >>> ovs-ctl.8
> > >>> + ovs-flowviz.8
> > >>> ovs-l3ping.8
> > >>> ovs-pki.8
> > >>> ovs-sim.1
> > >>> diff --git a/Documentation/ref/ovs-flowviz.8.rst
> > >>> b/Documentation/ref/ovs-flowviz.8.rst
> > >>> new file mode 100644
> > >>> index 000000000..969fda9be
> > >>> --- /dev/null
> > >>> +++ b/Documentation/ref/ovs-flowviz.8.rst
> > >>> @@ -0,0 +1,535 @@
> > >>> +..
> > >>> + Licensed under the Apache License, Version 2.0 (the "License");
> > >>> you may
> > >>> + not use this file except in compliance with the License. You may
> > >>> obtain
> > >>> + a copy of the License at
> > >>> +
> > >>> + http://www.apache.org/licenses/LICENSE-2.0
> > >>> +
> > >>> + Unless required by applicable law or agreed to in writing,
> > >>> software
> > >>> + distributed under the License is distributed on an "AS IS"
> > >>> BASIS, WITHOUT
> > >>> + WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> > >>> See the
> > >>> + License for the specific language governing permissions and
> > >>> limitations
> > >>> + under the License.
> > >>> +
> > >>> + Convention for heading levels in Open vSwitch documentation:
> > >>> +
> > >>> + ======= Heading 0 (reserved for the title in a document)
> > >>> + ------- Heading 1
> > >>> + ~~~~~~~ Heading 2
> > >>> + +++++++ Heading 3
> > >>> + ''''''' Heading 4
> > >>> +
> > >>> + Avoid deeper levels because they do not render well.
> > >>> +
> > >>> +===========
> > >>> +ovs-flowviz
> > >>> +===========
> > >>> +
> > >>> +Synopsis
> > >>> +========
> > >>> +
> > >>> +``ovs-flowviz``
> > >>> +[``[-i | --input] <[alias,]file>``]
> > >>> +[``[-c | --config] <file>``]
> > >>> +[``[-f | --filter] <filter>``]
> > >>> +[``[-h | --highlight] <filter>``]
> > >>> +[``--style <style>``]
> > >>> +*<flow_type>* *<format>* [<arg>...]
> > >>
> > >>
> > >> We should be consistent with the way arguments are formatted.
> > >> This one is not rendered well. See the following patch as an
> > >> example of how to fix the argument highlighting here and in
> > >> other places in the doc:
> > >>
> > >> https://patchwork.ozlabs.org/project/openvswitch/patch/20240819194301.1828180-1-i.maxim...@ovn.org/
> > >>
> > >
> > > Hmm... I see the arguments "well" rendered, in the sense that I don't
> > > see the unwanted characters that did show up on the previous version
> > > of "ovs-appctl" (before your patch).
> >
> > Before my appctl patch the man page looked like this:
> >
> > ovs-appctl [--target=``<target> | ``-t <target>] [--time‐
> > out=``<secs> | ``-T <secs>] [--format=``<format> | ``-f <for‐
> > mat>] [--pretty] <command> [<arg>…]
> >
> > See all the "``" quotes in the rendered page. After the change, they
> > are gone:
> >
> > ovs-appctl [--target=target | -t target] [--timeout=secs | -T
> > secs] [--format=format | -f format] [--pretty] command [arg
> > ...]
> >
>
> > >
> > > It seems "``--option=``<value>" does not work but "``--option`` value"
> > > does.
> >
> > Yes, because `` requires a space afterwards. A non-rendered escaped
> > space ('\ ') works as well.
> >
> > >
> > > However, the current patch will render the entire option (option name
> > > and value) in the same quote block. This is not consistent with
> > > "ovs-appctl", but it is with others like "ovs-ctl" or "ovs-tcpdump".
> > >
> > > OTOH, your patch also changes "--option=<value>" to "--option=*value*"
> > > i.e: "value" in italic style. That also doesn't seem consistent with
> > > other commands.
> >
> > It is consistent with how man pages are normally written.
> >
>
> Although I don't think there is a consensus over style in man pages
> (e.g: see git-log(1), tcpdump(8) and ip(8) for 3 completely different
> styles), what is fairly common is the use of _only_ bold and italics.
>
> Maybe it's easier to replace quote blocks with bolds to avoid them
> including the value as well as problems like the one you fixed.
>
Also, we need to change
`Documentation/internals/contributing/documentation-style.rst` which
specifies the use of "<>". It also links to the sphinx doc where options
are written that way. We would probably also have to enable emphasize
placeholders [2].
[1] https://www.sphinx-doc.org/en/master/usage/domains/standard.html
[2]
https://www.sphinx-doc.org/en/master/usage/configuration.html#confval-option_emphasise_placeholders
> Thanks.
> Adrián
>
> > >
> > > So, should we move "ovs-appctl" back to using "<value>" inside quote
> > > blocks or should we move others to "*value*"?
> >
> > We should change other pages as well, so they look like typical man pages,
> > but it is outside of scope for this patch set.
> >
> > Best regards, Ilya Maximets.
> >
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
