Re: [Openvpn-devel] [PATCH] wintun: refactor code to use enum driver type

2020-01-19 Thread Gert Doering
Hi,

On Thu, Jan 16, 2020 at 03:19:00PM +0100, Simon Rozman wrote:
>  {
> @@ -3676,14 +3692,16 @@ get_tap_reg(struct gc_arena *gc)
>  
>  if (status == ERROR_SUCCESS && data_type == REG_SZ)
>  {
> -if (!strcmp(component_id, TAP_WIN_COMPONENT_ID) ||
> -!strcmp(component_id, "root\\" TAP_WIN_COMPONENT_ID) 
> ||
> -!strcmp(component_id, WINTUN_COMPONENT_ID))
> +enum windows_driver_type windows_driver = 
> WINDOWS_DRIVER_UNSPECIFIED;
> +
> +if ((windows_driver = WINDOWS_DRIVER_TAP_WINDOWS6, 
> !strcmp(component_id, TAP_WIN_COMPONENT_ID))
> +|| (windows_driver = WINDOWS_DRIVER_TAP_WINDOWS6, 
> !strcmp(component_id, "root\\" TAP_WIN_COMPONENT_ID))
> +|| (windows_driver = WINDOWS_DRIVER_WINTUN, 
> !strcmp(component_id, WINTUN_COMPONENT_ID)))
>  {

I could propably figure out what this if() statement with three intentional
assignments in it does, but I'm fairly sure I do not *want* to spend the
time, and I think nobody else should.

Lev has ACKed the patch, and it's WIN32 only - but I would very much
prefer are more readable variant of this.  Like

if ( strcmp(component_id, TAP_WIN_COMPONENT_ID) == 0 ||
 strcmp(component_id, "root\\" TAP_WIN_COMPONENT_ID) )
{
windows_driver = WINDOWS_DRIVER_TAP_WINDOWS6;
}
else if ( strcmp(component_id, WINTUN_COMPONENT_ID) == 0 )
{
windows_driver = WINDOWS_DRIVER_WINTUN:
}

if ( windows_driver != WINDOWS_DRIVER_UNSPECIFIED )
{
... pre-existing code in the if() clause


not many more lines, but way easier to read than on-the-fly assignments
in if(), and also easier to read than the old code.


Pretty please :)

gert
-- 
"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
 Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany g...@greenie.muc.de


signature.asc
Description: PGP signature
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH] wintun: refactor code to use enum driver type

2020-01-17 Thread Lev Stipakov
Hi,

While we've discussed this during hackathon, it would
nevertheless be nice to explain in commit message
why this is needed (something like make code more device type agnostic).

Also, this line

  +msg(M_FATAL, "Adapter '%s' is using %s driver, %s expected.
If you want to use this device, adjust --windows-driver.", dev_node,
print_windows_driver(windows_driver),
print_windows_driver(tt->windows_driver));

is a bit too long, it would be nice to split it.

Other than that, looks good.

Built and tested with MSVC.

Acked-by: Lev Stipakov 

-- 
-Lev
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


[Openvpn-devel] [PATCH] wintun: refactor code to use enum driver type

2020-01-16 Thread Simon Rozman
Signed-off-by: Simon Rozman 
---
 src/openvpn/forward.c |   4 +-
 src/openvpn/init.c|   2 +-
 src/openvpn/options.c |  16 +++
 src/openvpn/options.h |   2 +-
 src/openvpn/tun.c | 108 --
 src/openvpn/tun.h |  16 +--
 6 files changed, 75 insertions(+), 73 deletions(-)

diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index 6b823613..ea10f0bf 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -1258,7 +1258,7 @@ read_incoming_tun(struct context *c)
 c->c2.buf = c->c2.buffers->read_tun_buf;
 
 #ifdef _WIN32
-if (c->c1.tuntap->wintun)
+if (c->c1.tuntap->windows_driver == WINDOWS_DRIVER_WINTUN)
 {
 read_wintun(c->c1.tuntap, >c2.buf);
 if (c->c2.buf.len == -1)
@@ -1274,7 +1274,7 @@ read_incoming_tun(struct context *c)
 {
 read_tun_buffered(c->c1.tuntap, >c2.buf);
 }
-#else
+#else  /* ifdef _WIN32 */
 ASSERT(buf_init(>c2.buf, FRAME_HEADROOM(>c2.frame)));
 ASSERT(buf_safe(>c2.buf, MAX_RW_SIZE_TUN(>c2.frame)));
 c->c2.buf.len = read_tun(c->c1.tuntap, BPTR(>c2.buf), 
MAX_RW_SIZE_TUN(>c2.frame));
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 0bdb0a9c..ec444f47 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -1734,7 +1734,7 @@ do_init_tun(struct context *c)
 >net_ctx);
 
 #ifdef _WIN32
-c->c1.tuntap->wintun = c->options.wintun;
+c->c1.tuntap->windows_driver = c->options.windows_driver;
 #endif
 
 init_tun_post(c->c1.tuntap,
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index a6f40e10..709ba4bb 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -854,7 +854,7 @@ init_options(struct options *o, const bool init_gc)
 o->tuntap_options.dhcp_masq_offset = 0; /* use network address as 
internal DHCP server address */
 o->route_method = ROUTE_METHOD_ADAPTIVE;
 o->block_outside_dns = false;
-o->wintun = false;
+o->windows_driver = WINDOWS_DRIVER_TAP_WINDOWS6;
 #endif
 o->vlan_accept = VLAN_ALL;
 o->vlan_pvid = 1;
@@ -3002,7 +3002,7 @@ options_postprocess_mutate_invariant(struct options 
*options)
 
 #ifdef _WIN32
 /* when using wintun, kernel doesn't send DHCP requests, so use netsh to 
set IP address and netmask */
-if (options->wintun)
+if (options->windows_driver == WINDOWS_DRIVER_WINTUN)
 {
 options->tuntap_options.ip_win32_type = IPW32_SET_NETSH;
 }
@@ -4076,23 +4076,23 @@ foreign_option(struct options *o, char *argv[], int 
len, struct env_set *es)
  *
  * @param str   value of --windows-driver option
  * @param msglevel  msglevel to report parsing error
- * @return bool true if --windows-driver is wintun, false otherwise
+ * @return enum windows_driver_type  driver type, WINDOWS_DRIVER_UNSPECIFIED 
on unknown --windows-driver value
  */
-static bool
+static enum windows_driver_type
 parse_windows_driver(const char *str, const int msglevel)
 {
 if (streq(str, "tap-windows6"))
 {
-return false;
+return WINDOWS_DRIVER_TAP_WINDOWS6;
 }
 else if (streq(str, "wintun"))
 {
-return true;
+return WINDOWS_DRIVER_WINTUN;
 }
 else
 {
 msg(msglevel, "--windows-driver must be tap-windows6 or wintun");
-return false;
+return WINDOWS_DRIVER_UNSPECIFIED;
 }
 }
 #endif
@@ -5367,7 +5367,7 @@ add_option(struct options *options,
 else if (streq(p[0], "windows-driver") && p[1] && !p[2])
 {
 VERIFY_PERMISSION(OPT_P_GENERAL);
-options->wintun = parse_windows_driver(p[1], M_FATAL);
+options->windows_driver = parse_windows_driver(p[1], M_FATAL);
 }
 #endif
 else if (streq(p[0], "dev-node") && p[1] && !p[2])
diff --git a/src/openvpn/options.h b/src/openvpn/options.h
index 7fd2c00f..84d05f26 100644
--- a/src/openvpn/options.h
+++ b/src/openvpn/options.h
@@ -634,7 +634,7 @@ struct options
 bool show_net_up;
 int route_method;
 bool block_outside_dns;
-bool wintun;
+enum windows_driver_type windows_driver;
 #endif
 
 bool use_peer_id;
diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
index 32a3f756..af09e676 100644
--- a/src/openvpn/tun.c
+++ b/src/openvpn/tun.c
@@ -786,7 +786,7 @@ init_tun_post(struct tuntap *tt,
 overlapped_io_init(>writes, frame, TRUE, true);
 tt->adapter_index = TUN_ADAPTER_INDEX_INVALID;
 
-if (tt->wintun)
+if (tt->windows_driver == WINDOWS_DRIVER_WINTUN)
 {
 tt->wintun_send_ring_handle = CreateFileMapping(INVALID_HANDLE_VALUE, 
NULL,
 PAGE_READWRITE,
@@ -1388,7 +1388,7 @@ do_ifconfig_ipv4(struct tuntap *tt, const char *ifname, 
int tun_mtu,
 {
 ASSERT(ifname != NULL);
 
-if (tt->options.msg_channel && tt->wintun)
+if (tt->options.msg_channel && tt->windows_driver == 
WINDOWS_DRIVER_WINTUN)
 {
 do_address_service(true, AF_INET, tt);