On 9/23/24 16:50, Adrián Moreno wrote:
> 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 <[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.
>>
>
> 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].
I think there is a bit of a confusion on both sides.
The doc is talking about '.. option:' and it seems this directive doesn't
support any internal formatting. Instead it is just using bold text.
The [2] may be used for emphasizing, but the arguments still need to be
enclosed in angular brackets to be more visible. And that is what the doc
style document is talking about.
The ovs-appctl page doesn't seem to use the '.. option:' directive...
OTOH, the options in the synopsis part of the pages in most OVS man pages
have italics/underlined options, including the ovs-test.8 that was the first
one to be converted to rST along with creation of the documentation style
document. So we should keep that this way, IMO.
Also, according to the man-pages(7):
SYNOPSIS
A brief summary of the command or function's interface.
For commands, this shows the syntax of the command and
its arguments (including options); boldface is used for
as-is text and italics are used to indicate replaceable
arguments.
...
Formatting conventions for manual pages describing commands
For manual pages that describe a command (typically in Sections
1 and 8), the arguments are always specified using italics,
even in the SYNOPSIS section.
The name of the command, and its options, should always be for‐
matted in bold.
Unfortunately, sphinx doesn't seem to follow these guidelines for
the '.. option:' directive.
If you agree, I'll move the '.. option:' parts of this patch back to
angular brackets while applying v6.
>
> [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
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
