On 10/21/24 11:23, Ilya Maximets wrote: > 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.
It's probably doesn't worth spending a lot of time on this though, so I'll go ahead and apply v6 with these changes and we can re-work the documentation together with other pages later if necessary. Best regards, Ilya Maximets. > >> >> [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
