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 <[email protected]>
> >>> ---
> >>> 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/[email protected]/
> >>
> >
> > 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.
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
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
