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.
>
> 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
