diff mbox series

[Openvpn-devel,13/14] log file descriptor in more socket related error messages

Message ID 20210401131337.3684-14-arne@rfc2549.org
State Changes Requested
Headers
Series Various clean up patches |

Commit Message

Arne Schwabe April 1, 2021, 2:13 a.m. UTC 
  This add the fd to the epoll event error message and the x_check_status
message. This helps debugging when thing go wrong with event handling.

Also add logging when ep_del fails to remove a socket from the structure.
In constract to ep_ctl that has this as a FATAL message (M_ERR), we only
log here since the code has been ignoring the status forever there might
be corner cases where a FATAL message could trigger an unintened regression.

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/error.c | 8 ++++----
 src/openvpn/event.c | 8 ++++++--
 2 files changed, 10 insertions(+), 6 deletions(-)

Comments

Gert Doering April 2, 2021, 4:20 a.m. UTC | #1 
Hi,

On Thu, Apr 01, 2021 at 03:13:36PM +0200, Arne Schwabe wrote:
>              if (extended_msg)
>              {
> -                msg(x_cs_info_level, "%s %s [%s]: %s (code=%d)", description,
> +                msg(x_cs_info_level, "%s %s [%s]: %s (fd=%d,code=%d)", description,
>                      sock ? proto2ascii(sock->info.proto, sock->info.af, true) : "",
> -                    extended_msg, strerror(my_errno), my_errno);
> +                    extended_msg, strerror(my_errno), my_errno, sock ? sock->sd : -1);

I'm not sure if I'm misreading this, but "fd=%d" is added before "code=%d",
but the "sock->fd" thingie is added at the end.

So I tentatively NAK this.

> --- a/src/openvpn/event.c
> +++ b/src/openvpn/event.c
> @@ -555,7 +555,10 @@ ep_del(struct event_set *es, event_t event)
>  
>      ASSERT(!eps->fast);
>      CLEAR(ev);
> -    epoll_ctl(eps->epfd, EPOLL_CTL_DEL, event, &ev);
> +    if (epoll_ctl(eps->epfd, EPOLL_CTL_DEL, event, &ev) < 0)
> +    {
> +        msg(M_WARN|M_ERRNO, "EVENT: epoll_ctl EPOLL_CTL_DEL failed, sd=%d", (int)event);
> +    }
>  }

The rest of the patch looks reasonable.

gert
diff mbox series

Patch

diff --git a/src/openvpn/error.c b/src/openvpn/error.c
index e6f7ff0ff..4eebf41a9 100644
--- a/src/openvpn/error.c
+++ b/src/openvpn/error.c
@@ -690,15 +690,15 @@  x_check_status(int status,
         {
             if (extended_msg)
             {
-                msg(x_cs_info_level, "%s %s [%s]: %s (code=%d)", description,
+                msg(x_cs_info_level, "%s %s [%s]: %s (fd=%d,code=%d)", description,
                     sock ? proto2ascii(sock->info.proto, sock->info.af, true) : "",
-                    extended_msg, strerror(my_errno), my_errno);
+                    extended_msg, strerror(my_errno), my_errno, sock ? sock->sd : -1);
             }
             else
             {
-                msg(x_cs_info_level, "%s %s: %s (code=%d)", description,
+                msg(x_cs_info_level, "%s %s: %s (fd=%d,code=%d)", description,
                     sock ? proto2ascii(sock->info.proto, sock->info.af, true) : "",
-                    strerror(my_errno), my_errno);
+                    strerror(my_errno), my_errno, sock ? sock->sd : -1);
             }
 
             if (x_cs_err_delay_ms)
diff --git a/src/openvpn/event.c b/src/openvpn/event.c
index 49dfa861c..14a25155c 100644
--- a/src/openvpn/event.c
+++ b/src/openvpn/event.c
@@ -555,7 +555,10 @@  ep_del(struct event_set *es, event_t event)
 
     ASSERT(!eps->fast);
     CLEAR(ev);
-    epoll_ctl(eps->epfd, EPOLL_CTL_DEL, event, &ev);
+    if (epoll_ctl(eps->epfd, EPOLL_CTL_DEL, event, &ev) < 0)
+    {
+        msg(M_WARN|M_ERRNO, "EVENT: epoll_ctl EPOLL_CTL_DEL failed, sd=%d", (int)event);
+    }
 }
 
 static void
@@ -844,7 +847,8 @@  po_wait(struct event_set *es, const struct timeval *tv, struct event_set_return
             }
             else if (pfdp->revents)
             {
-                msg(D_EVENT_ERRORS, "Error: poll: unknown revents=0x%04x", (unsigned int)pfdp->revents);
+                msg(D_EVENT_ERRORS, "Error: poll: unknown revents=0x%04x for fd=%d",
+                    (unsigned int)pfdp->revents, pfdp->fd);
             }
             ++pfdp;
         }