diff mbox series

[Openvpn-devel,3/3,v2] doc/options: clean up documentation for --proto and related options

Message ID 1155495853.307825.1644488506153@office.mailbox.org
State Superseded
Headers show
Series None | expand

Commit Message

Frank Lichtenheld Feb. 9, 2022, 11:21 p.m. UTC 
The family specific options were generally omitted.

Signed-off-by: Frank Lichtenheld <frank@lichtenheld.com>
---
 doc/man-sections/client-options.rst |  5 +++++
 doc/man-sections/link-options.rst   |  5 ++++-
 src/openvpn/options.c               | 17 +++++++++--------
 3 files changed, 18 insertions(+), 9 deletions(-)

v2: move #define around

--
2.30.2

Comments

David Sommerseth Feb. 11, 2022, 9:39 a.m. UTC | #1 
On 10/02/2022 11:21, Frank Lichtenheld wrote:
> The family specific options were generally omitted.

> 

> Signed-off-by: Frank Lichtenheld <frank@lichtenheld.com>

> ---

>   doc/man-sections/client-options.rst |  5 +++++

>   doc/man-sections/link-options.rst   |  5 ++++-

>   src/openvpn/options.c               | 17 +++++++++--------

>   3 files changed, 18 insertions(+), 9 deletions(-)

> 

> v2: move #define around

> 

> diff --git a/doc/man-sections/client-options.rst b/doc/man-sections/client-options.rst

> index 73f1ea51..4c4a8707 100644

> --- a/doc/man-sections/client-options.rst

> +++ b/doc/man-sections/client-options.rst

> @@ -198,6 +198,11 @@ configuration.

>     When iterating through connection profiles, only consider profiles using

>     protocol ``p`` (:code:`tcp` \| :code:`udp`).

> 

> +  Note that this specifically only affects the protocol, not the inet

> +  family (i.e. IPv4 vs. IPv6). While the option actually accepts

> +  values like :code:`udp6`, there is no difference to specifying

> +  :code:`udp`.

> +


I'm sorry I didn't catch this at the previous round; somehow I mixed it 
with the --proto change below.  The section above is related to
--proto-force.

May I suggest we clarify this text a bit more?  As we have multiple 
definitions of protocol - whether it is the IP layer protocol or the 
TCP/UDP protocol on top of the IP layer.  And in --proto we do indeed 
mix them up into a single option.

Perhaps something like the suggestion below might be somewhat clearer?

    Note that this specifically only affects the TCP/IP transport layer
    protocol (UDP/TCP), not the TCP/IP network layers (IPv4/IPv6).  In
    practice it means it will only consider connection profiles using
    either TCP or UDP.  This does not affect whether IPv4 or IPv6 is used
    as IP protocols. In this context, :code:`udp`, :code:`udp4` and
    :code:`udp6` are all considered the same. And similar with
    :code:`tcp`, :code:`tcp4` and :code:`tcp6`


The rest of the changes looks good now, and the relocation of the 
#define is better as well.


-- 
kind regards,

David Sommerseth
OpenVPN Inc
Frank Lichtenheld Feb. 14, 2022, 1:41 a.m. UTC | #2 
> David Sommerseth <openvpn@sf.lists.topphemmelig.net> hat am 11.02.2022 21:39 geschrieben:
> 
>  
> On 10/02/2022 11:21, Frank Lichtenheld wrote:
> > The family specific options were generally omitted.
> > 
> > Signed-off-by: Frank Lichtenheld <frank@lichtenheld.com>
> > ---
> >   doc/man-sections/client-options.rst |  5 +++++
> >   doc/man-sections/link-options.rst   |  5 ++++-
> >   src/openvpn/options.c               | 17 +++++++++--------
> >   3 files changed, 18 insertions(+), 9 deletions(-)
> > 
> > v2: move #define around
> > 
> > diff --git a/doc/man-sections/client-options.rst b/doc/man-sections/client-options.rst
> > index 73f1ea51..4c4a8707 100644
> > --- a/doc/man-sections/client-options.rst
> > +++ b/doc/man-sections/client-options.rst
> > @@ -198,6 +198,11 @@ configuration.
> >     When iterating through connection profiles, only consider profiles using
> >     protocol ``p`` (:code:`tcp` \| :code:`udp`).
> > 
> > +  Note that this specifically only affects the protocol, not the inet
> > +  family (i.e. IPv4 vs. IPv6). While the option actually accepts
> > +  values like :code:`udp6`, there is no difference to specifying
> > +  :code:`udp`.
> > +
> 
[...]
> Perhaps something like the suggestion below might be somewhat clearer?
> 
>     Note that this specifically only affects the TCP/IP transport layer
>     protocol (UDP/TCP), not the TCP/IP network layers (IPv4/IPv6).  In
>     practice it means it will only consider connection profiles using
>     either TCP or UDP.  This does not affect whether IPv4 or IPv6 is used
>     as IP protocols. In this context, :code:`udp`, :code:`udp4` and
>     :code:`udp6` are all considered the same. And similar with
>     :code:`tcp`, :code:`tcp4` and :code:`tcp6`
> 

Thanks, I hate it ;)

Seriously though, I find this too clunky. Yes, TCP/IP is
technically the correct name, but isn't that even more confusing?

Maybe we can find a compromise:

     Note that this specifically only filters by the transport layer
     protocol, i.e. UDP or TCP.  This does not affect whether IPv4 or
     IPv6 is used as IP protocol.

     For implementation reasons the option accepts the :code:`4` and :code:`6`
     suffixes when specifying the protocol
     (i.e. :code:`udp4` / :code:`udp6` / :code:`tcp4` / :code:`tcp6`).
     However, these behave the same as without the suffix and should be avoided
     to prevent confusion.

Regards,
--
Frank Lichtenheld
David Sommerseth Feb. 15, 2022, 2:36 a.m. UTC | #3 
On 14/02/2022 13:41, Frank Lichtenheld wrote:
>> David Sommerseth <openvpn@sf.lists.topphemmelig.net> hat am 11.02.2022 21:39 geschrieben:

>>

>>   

>> On 10/02/2022 11:21, Frank Lichtenheld wrote:

>>> The family specific options were generally omitted.

>>>

>>> Signed-off-by: Frank Lichtenheld <frank@lichtenheld.com>

>>> ---

>>>    doc/man-sections/client-options.rst |  5 +++++

>>>    doc/man-sections/link-options.rst   |  5 ++++-

>>>    src/openvpn/options.c               | 17 +++++++++--------

>>>    3 files changed, 18 insertions(+), 9 deletions(-)

>>>

>>> v2: move #define around

>>>

>>> diff --git a/doc/man-sections/client-options.rst b/doc/man-sections/client-options.rst

>>> index 73f1ea51..4c4a8707 100644

>>> --- a/doc/man-sections/client-options.rst

>>> +++ b/doc/man-sections/client-options.rst

>>> @@ -198,6 +198,11 @@ configuration.

>>>      When iterating through connection profiles, only consider profiles using

>>>      protocol ``p`` (:code:`tcp` \| :code:`udp`).

>>>

>>> +  Note that this specifically only affects the protocol, not the inet

>>> +  family (i.e. IPv4 vs. IPv6). While the option actually accepts

>>> +  values like :code:`udp6`, there is no difference to specifying

>>> +  :code:`udp`.

>>> +

>>

> [...]

>> Perhaps something like the suggestion below might be somewhat clearer?

>>

>>      Note that this specifically only affects the TCP/IP transport layer

>>      protocol (UDP/TCP), not the TCP/IP network layers (IPv4/IPv6).  In

>>      practice it means it will only consider connection profiles using

>>      either TCP or UDP.  This does not affect whether IPv4 or IPv6 is used

>>      as IP protocols. In this context, :code:`udp`, :code:`udp4` and

>>      :code:`udp6` are all considered the same. And similar with

>>      :code:`tcp`, :code:`tcp4` and :code:`tcp6`

>>

> 

> Thanks, I hate it ;)

> 

> Seriously though, I find this too clunky. Yes, TCP/IP is

> technically the correct name, but isn't that even more confusing?

> 

> Maybe we can find a compromise:

> 

>       Note that this specifically only filters by the transport layer

>       protocol, i.e. UDP or TCP.  This does not affect whether IPv4 or

>       IPv6 is used as IP protocol.

> 

>       For implementation reasons the option accepts the :code:`4` and :code:`6`

>       suffixes when specifying the protocol

>       (i.e. :code:`udp4` / :code:`udp6` / :code:`tcp4` / :code:`tcp6`).

>       However, these behave the same as without the suffix and should be avoided

>       to prevent confusion.

Thanks, that's better!  Let's roll with that.


-- 
kind regards,

David Sommerseth
OpenVPN Inc
diff mbox series

Patch

diff --git a/doc/man-sections/client-options.rst b/doc/man-sections/client-options.rst
index 73f1ea51..4c4a8707 100644
--- a/doc/man-sections/client-options.rst
+++ b/doc/man-sections/client-options.rst
@@ -198,6 +198,11 @@  configuration.
   When iterating through connection profiles, only consider profiles using
   protocol ``p`` (:code:`tcp` \| :code:`udp`).

+  Note that this specifically only affects the protocol, not the inet
+  family (i.e. IPv4 vs. IPv6). While the option actually accepts
+  values like :code:`udp6`, there is no difference to specifying
+  :code:`udp`.
+
 --pull
   This option must be used on a client which is connecting to a
   multi-client server. It indicates to OpenVPN that it should accept
diff --git a/doc/man-sections/link-options.rst b/doc/man-sections/link-options.rst
index cbc1e97b..0457727e 100644
--- a/doc/man-sections/link-options.rst
+++ b/doc/man-sections/link-options.rst
@@ -276,7 +276,10 @@  the local and the remote host.

 --proto p
   Use protocol ``p`` for communicating with remote host. ``p`` can be
-  :code:`udp`, :code:`tcp-client`, or :code:`tcp-server`.
+  :code:`udp`, :code:`tcp-client`, or :code:`tcp-server`. You can also
+  limit OpenVPN to use only IPv4 or only IPv6 by specifying ``p`` as
+  :code:`udp4`, :code:`tcp4-client`, :code:`tcp4-server` or :code:`udp6`,
+  :code:`tcp6-client`, :code:`tcp6-server`, respectively.

   The default protocol is :code:`udp` when ``--proto`` is not specified.

diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 1176a7c8..e7eba341 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -126,9 +126,11 @@  static const char usage_message[] =
     "--remote-random-hostname : Add a random string to remote DNS name.\n"
     "--mode m        : Major mode, m = 'p2p' (default, point-to-point) or 'server'.\n"
     "--proto p       : Use protocol p for communicating with peer.\n"
-    "                  p = udp (default), tcp-server, or tcp-client\n"
+    "                  p = udp (default), tcp-server, tcp-client\n"
+    "                      udp4, tcp4-server, tcp4-client\n"
+    "                      udp6, tcp6-server, tcp6-client\n"
     "--proto-force p : only consider protocol p in list of connection profiles.\n"
-    "                  p = udp6, tcp6-server, or tcp6-client (ipv6)\n"
+    "                  p = udp or tcp\n"
     "--connect-retry n [m] : For client, number of seconds to wait between\n"
     "                  connection retries (default=%d). On repeated retries\n"
     "                  the wait time is exponentially increased to a maximum of m\n"
@@ -2298,6 +2300,8 @@  options_postprocess_verify_ce(const struct options *options,
      */
     if (options->mode == MODE_SERVER)
     {
+#define USAGE_VALID_SERVER_PROTOS "--mode server currently only supports " \
+      "--proto values of udp, tcp-server, tcp4-server, or tcp6-server"
 #ifdef TARGET_ANDROID
         msg(M_FATAL, "--mode server not supported on Android");
 #endif
@@ -2315,15 +2319,14 @@  options_postprocess_verify_ce(const struct options *options,
         }
         if (!(proto_is_udp(ce->proto) || ce->proto == PROTO_TCP_SERVER))
         {
-            msg(M_USAGE, "--mode server currently only supports "
-                "--proto udp or --proto tcp-server or proto tcp6-server");
+            msg(M_USAGE, USAGE_VALID_SERVER_PROTOS);
         }
 #if PORT_SHARE
         if ((options->port_share_host || options->port_share_port)
             && (ce->proto != PROTO_TCP_SERVER))
         {
             msg(M_USAGE, "--port-share only works in TCP server mode "
-                "(--proto tcp-server or tcp6-server)");
+                "(--proto values of tcp-server, tcp4-server, or tcp6-server)");
         }
 #endif
         if (!options->tls_server)
@@ -2367,9 +2370,7 @@  options_postprocess_verify_ce(const struct options *options,
         }
         if (!(proto_is_dgram(ce->proto) || ce->proto == PROTO_TCP_SERVER))
         {
-            msg(M_USAGE,
-                "--mode server currently only supports --proto udp or --proto "
-                "tcp-server or --proto tcp6-server");
+            msg(M_USAGE, USAGE_VALID_SERVER_PROTOS);
         }
         if (!proto_is_udp(ce->proto) && (options->cf_max || options->cf_per))
         {