[Openvpn-devel,04/14] Add documentation on EVENT_READ/EVENT_WRITE constants

Message ID 20210401131337.3684-5-arne@rfc2549.org
State Under Review
Delegated to: Antonio Quartulli
Headers show
Series
  • Various clean up patches
Related show

Commit Message

Arne Schwabe April 1, 2021, 1:13 p.m.
Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/forward.c |  3 ++-
 src/openvpn/openvpn.h | 12 +++++++++++-
 2 files changed, 13 insertions(+), 2 deletions(-)

Comments

Antonio Quartulli April 3, 2021, 4:22 p.m. | #1
On 01/04/2021 15:13, Arne Schwabe wrote:
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
> ---
>  src/openvpn/forward.c |  3 ++-
>  src/openvpn/openvpn.h | 12 +++++++++++-
>  2 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
> index 6f7a50048..98caf6651 100644
> --- a/src/openvpn/forward.c
> +++ b/src/openvpn/forward.c
> @@ -1880,7 +1880,8 @@ io_wait_dowork(struct context *c, const unsigned int flags)
>      unsigned int tuntap = 0;
>      struct event_set_return esr[4];
>  
> -    /* These shifts all depend on EVENT_READ and EVENT_WRITE */
> +    /* These shifts all depend on EVENT_READ (=1) and EVENT_WRITE (=2) */
> +    /* and are added to the shift. */

I suggest making a classic multiline comment here, instead of two single
lines.

>      static int socket_shift = 0;   /* depends on SOCKET_READ and SOCKET_WRITE */
>      static int tun_shift = 2;      /* depends on TUN_READ and TUN_WRITE */
>      static int err_shift = 4;      /* depends on ES_ERROR */
> diff --git a/src/openvpn/openvpn.h b/src/openvpn/openvpn.h
> index 0ddaeb730..322ab3ee1 100644
> --- a/src/openvpn/openvpn.h
> +++ b/src/openvpn/openvpn.h
> @@ -245,7 +245,17 @@ struct context_2
>      int event_set_max;
>      bool event_set_owned;
>  
> -    /* event flags returned by io_wait */
> +    /* event flags returned by io_wait,

colon or period instead of comma?

> +     * All these event are their respective shift as defined in io_wait_dowork
> +     * adding a shift of 0 for the READ event and 1 for the write event.
> +     *
> +     * E.g. management_shift = 6;
> +     * MANAGMENT_READ = (1<<(6+0)),
> +     * MANAGEMNET_WRITE = (1<<(6+1))
> +     *
> +     * Some shifts (error, file_close) are using read/write for diferent

diferent -> different

> +     * signals.
> +     */
>  #define SOCKET_READ       (1<<0)
>  #define SOCKET_WRITE      (1<<1)
>  #define TUN_READ          (1<<2)
> 

I get what you are trying to describe, but it is not easy to grasp
without jumping back and forth between openvpn.h, forward.c and events.c.

Instead of talking about shifts, why not going high level and explain
how the bitmask is supposed to look like (i.e. 2 bits are associated
with each class of operations. The first bit is a READ event, the second
one is a WRITE event..etc..).

Opinions?

In this patch you could even move the definition of all the *_shift to
openvpn.h, next to the various _READ and _WRITE, since they are all related.

What do you think? Would it make sense?

Cheers,

Patch

diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index 6f7a50048..98caf6651 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -1880,7 +1880,8 @@  io_wait_dowork(struct context *c, const unsigned int flags)
     unsigned int tuntap = 0;
     struct event_set_return esr[4];
 
-    /* These shifts all depend on EVENT_READ and EVENT_WRITE */
+    /* These shifts all depend on EVENT_READ (=1) and EVENT_WRITE (=2) */
+    /* and are added to the shift. */
     static int socket_shift = 0;   /* depends on SOCKET_READ and SOCKET_WRITE */
     static int tun_shift = 2;      /* depends on TUN_READ and TUN_WRITE */
     static int err_shift = 4;      /* depends on ES_ERROR */
diff --git a/src/openvpn/openvpn.h b/src/openvpn/openvpn.h
index 0ddaeb730..322ab3ee1 100644
--- a/src/openvpn/openvpn.h
+++ b/src/openvpn/openvpn.h
@@ -245,7 +245,17 @@  struct context_2
     int event_set_max;
     bool event_set_owned;
 
-    /* event flags returned by io_wait */
+    /* event flags returned by io_wait,
+     * All these event are their respective shift as defined in io_wait_dowork
+     * adding a shift of 0 for the READ event and 1 for the write event.
+     *
+     * E.g. management_shift = 6;
+     * MANAGMENT_READ = (1<<(6+0)),
+     * MANAGEMNET_WRITE = (1<<(6+1))
+     *
+     * Some shifts (error, file_close) are using read/write for diferent
+     * signals.
+     */
 #define SOCKET_READ       (1<<0)
 #define SOCKET_WRITE      (1<<1)
 #define TUN_READ          (1<<2)