diff mbox series

[Openvpn-devel,5/7] tun: extract close_tun_handle into its own fucntion and print correct type

Message ID 20220402070902.30282-6-a@unstable.cc
State Superseded
Headers show
Series Introduce ovpn-dco(-win) support | expand

Commit Message

Antonio Quartulli April 1, 2022, 8:09 p.m. UTC 
From: Arne Schwabe <arne@rfc2549.org>

This moves closing the tun handle into its own function and also prints
the adapter type we are operating on, instead hardcoding it to
tap-windows.

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
Signed-off-by: Antonio Quartulli <a@unstable.cc>
---
 src/openvpn/tun.c | 75 ++++++++++++++++++++++++++---------------------
 1 file changed, 41 insertions(+), 34 deletions(-)

Comments

Frank Lichtenheld April 5, 2022, midnight UTC | #1 
Acked-by: Frank Lichtenheld <frank@lichtenheld.com>

Seems trivial.

> Antonio Quartulli <a@unstable.cc> hat am 02.04.2022 09:09 geschrieben:
[...]
> +    if (tt->hand != NULL)
> +    {
> +        dmsg(D_WIN32_IO_LOW, "Attempting CloseHandle on %s adapter", adaptertype);
> +        if (!CloseHandle(tt->hand))
> +        {
> +            msg(M_WARN | M_ERRNO, "Warning: CloseHandle failed on %s adapter", adaptertype);
> +        }
> +        tt->hand = NULL;

Setting hand to NULL is a slight behavior change, it wasn't there before.
It might be nice to acknowledge it in the commit message just to confirm that
it was a deliberate change.

Regards,
--
Frank Lichtenheld
Antonio Quartulli April 5, 2022, 1:21 a.m. UTC | #2 
Hi,

On 05/04/2022 12:00, Frank Lichtenheld wrote:
> Acked-by: Frank Lichtenheld <frank@lichtenheld.com>
> 
> Seems trivial.
> 
>> Antonio Quartulli <a@unstable.cc> hat am 02.04.2022 09:09 geschrieben:
> [...]
>> +    if (tt->hand != NULL)
>> +    {
>> +        dmsg(D_WIN32_IO_LOW, "Attempting CloseHandle on %s adapter", adaptertype);
>> +        if (!CloseHandle(tt->hand))
>> +        {
>> +            msg(M_WARN | M_ERRNO, "Warning: CloseHandle failed on %s adapter", adaptertype);
>> +        }
>> +        tt->hand = NULL;
> 
> Setting hand to NULL is a slight behavior change, it wasn't there before.
> It might be nice to acknowledge it in the commit message just to confirm that
> it was a deliberate change.

Yes, it was a deliberate change.
This helper function will be invoked by (possibly) multiple places, so 
it's better to set a precondition and prevent a double close.

I agree we should add a sentence to the commit message about this.

Regards,
diff mbox series

Patch

diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
index 8ed7c88b..35b1744c 100644
--- a/src/openvpn/tun.c
+++ b/src/openvpn/tun.c
@@ -6772,6 +6772,46 @@  netsh_delete_address_dns(const struct tuntap *tt, bool ipv6, struct gc_arena *gc
     argv_free(&argv);
 }
 
+static
+void close_tun_handle(struct tuntap* tt)
+{
+    const char* adaptertype = print_windows_driver(tt->windows_driver);
+    if (tt->hand != NULL)
+    {
+        dmsg(D_WIN32_IO_LOW, "Attempting CancelIO on %s adapter", adaptertype);
+        if (!CancelIo(tt->hand))
+        {
+            msg(M_WARN | M_ERRNO, "Warning: CancelIO failed on %s adapter", adaptertype);
+        }
+    }
+
+    dmsg(D_WIN32_IO_LOW, "Attempting close of overlapped read event on %s adapter", adaptertype);
+    overlapped_io_close(&tt->reads);
+
+    dmsg(D_WIN32_IO_LOW, "Attempting close of overlapped write event on %s adapter", adaptertype);
+    overlapped_io_close(&tt->writes);
+
+    if (tt->hand != NULL)
+    {
+        dmsg(D_WIN32_IO_LOW, "Attempting CloseHandle on %s adapter", adaptertype);
+        if (!CloseHandle(tt->hand))
+        {
+            msg(M_WARN | M_ERRNO, "Warning: CloseHandle failed on %s adapter", adaptertype);
+        }
+        tt->hand = NULL;
+    }
+
+    if (tt->windows_driver == WINDOWS_DRIVER_WINTUN)
+    {
+        CloseHandle(tt->rw_handle.read);
+        CloseHandle(tt->rw_handle.write);
+        UnmapViewOfFile(tt->wintun_send_ring);
+        UnmapViewOfFile(tt->wintun_receive_ring);
+        CloseHandle(tt->wintun_send_ring_handle);
+        CloseHandle(tt->wintun_receive_ring_handle);
+    }
+}
+
 void
 close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx)
 {
@@ -6841,43 +6881,10 @@  close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx)
 
     dhcp_release(tt);
 
-    if (tt->hand != NULL)
-    {
-        dmsg(D_WIN32_IO_LOW, "Attempting CancelIO on TAP-Windows adapter");
-        if (!CancelIo(tt->hand))
-        {
-            msg(M_WARN | M_ERRNO, "Warning: CancelIO failed on TAP-Windows adapter");
-        }
-    }
-
-    dmsg(D_WIN32_IO_LOW, "Attempting close of overlapped read event on TAP-Windows adapter");
-    overlapped_io_close(&tt->reads);
-
-    dmsg(D_WIN32_IO_LOW, "Attempting close of overlapped write event on TAP-Windows adapter");
-    overlapped_io_close(&tt->writes);
-
-    if (tt->hand != NULL)
-    {
-        dmsg(D_WIN32_IO_LOW, "Attempting CloseHandle on TAP-Windows adapter");
-        if (!CloseHandle(tt->hand))
-        {
-            msg(M_WARN | M_ERRNO, "Warning: CloseHandle failed on TAP-Windows adapter");
-        }
-    }
+    close_tun_handle(tt);
 
     free(tt->actual_name);
 
-    if (tt->windows_driver == WINDOWS_DRIVER_WINTUN)
-    {
-        CloseHandle(tt->rw_handle.read);
-        CloseHandle(tt->rw_handle.write);
-        UnmapViewOfFile(tt->wintun_send_ring);
-        UnmapViewOfFile(tt->wintun_receive_ring);
-        CloseHandle(tt->wintun_send_ring_handle);
-        CloseHandle(tt->wintun_receive_ring_handle);
-    }
-
-
     clear_tuntap(tt);
     free(tt);
     gc_free(&gc);