Hi,
Please bear with me for making a few more comments. This close to final so
only
a few minor issues.
On Thu, May 4, 2017 at 1:36 PM, ValdikSS <valdi...@gmail.com> wrote:
>
> Windows 10 before Creators Update used to resolve DNS using all available
> adapters and IP addresses in parallel.
> Now it still resolves addresses using all available adapters but in a
> round-robin way, beginning with random adapter.
> This behaviour introduces significant delay when block-outside-dns is in
> use.
> Fortunately, setting low metric for the TAP interface solves this issue,
> making Windows always pick with TAP adapter first.
>
Nitpicking here, but anyway a new version is required (see below), so
it would be nice if you wrap lines in commit message to something
like about 70 to 80 at most. Not sure whether we have a policy on this..
diff --git a/src/openvpn/block_dns.c b/src/openvpn/block_dns.c
> index e31765e..9d49355 100644
> --- a/src/openvpn/block_dns.c
> +++ b/src/openvpn/block_dns.c
> @@ -341,4 +341,79 @@ delete_block_dns_filters(HANDLE engine_handle)
> return err;
> }
> +/*
> + * Returns interface metric value for specified interface index.
> + *
> + * Arguments:
> + * index : The index of TAP adapter.
> + * family : Address family (AF_INET for IPv4 and AF_INET6 for
> IPv6).
> + * Returns positive metric value or zero for automatic metric on success,
> + * a less then zero error code on failure.
> + */
> +
> +int
> +get_interface_metric(const NET_IFINDEX index, const ADDRESS_FAMILY family)
> +{
> + DWORD err = 0;
> + MIB_IPINTERFACE_ROW ipiface;
> + InitializeIpInterfaceEntry(&ipiface);
InitializeIpInterfaceEntry() is missing in all but very recent mingw32
versions
(their commit logs show it was added in early 2015) so we may need
to declare it in block_dns.c. I use Debian jessie (8.7) -- mingw gcc 4.9.1
and its
not there. I believe Samuli's build system uses an even older version.
Note that 64 bit build still succeeds with a warning but 32 bit will fail
because
of stdcall. Yes, its stdcall though MSDN doesn't mention it as such.
> typedef void (*block_dns_msg_handler_t) (DWORD err, const char *msg);
>
> DWORD
> @@ -36,5 +39,32 @@ DWORD
> add_block_dns_filters(HANDLE *engine, int iface_index, const WCHAR
> *exe_path,
> block_dns_msg_handler_t msg_handler_callback);
>
> +/**
> + * Sets interface metric value for specified interface index
>
This would be "Get" or "Return" not "Set"
> + *
> + * @param index The index of TAP adapter
> + * @param family Address family (AF_INET for IPv4 and AF_INET6 for IPv6)
> + *
> + * @return positive metric value or zero for automatic metric on success,
> + * a less then zero error code on failure.
> + */
> +
> +int
> +get_interface_metric(const NET_IFINDEX index, const ADDRESS_FAMILY
> family);
>
>
..
> diff --git a/src/openvpn/win32.c b/src/openvpn/win32.c
> index 18e7aee..dc411fc 100644
> --- a/src/openvpn/win32.c
> +++ b/src/openvpn/win32.c
> @@ -61,6 +61,11 @@
> static HANDLE m_hEngineHandle = NULL; /* GLOBAL */
>
> /*
> + * TAP adapter original metric value
> + */
> +static int tap_metric = -1; /* GLOBAL */
> +
> +/*
> * Windows internal socket API state (opaque).
> */
> static struct WSAData wsa_state; /* GLOBAL */
> @@ -1337,6 +1342,21 @@ win_wfp_block_dns(const NET_IFINDEX index, const
> HANDLE msg_channel)
>
> status = add_block_dns_filters(&m_hEngineHandle, index, openvpnpath,
> block_dns_msg_handler);
> + if (status == 0)
> + {
> + tap_metric = get_interface_metric(index, AF_INET);
>
The current metric is queried and saved only for v4 while the metric is
set and restored for both v4 and v6. Isn't it necessary to also save the
metric for both independently? I'm not sure whether anyone really uses
different metric for v4 and v6 but Windows allows it.
> + if (tap_metric < 0)
> + {
> + /* error, should not restore metric */
> + tap_metric = -1;
> + }
> + status = set_interface_metric(index, AF_INET,
> BLOCK_DNS_IFACE_METRIC);
> + if (!status)
> + {
> + set_interface_metric(index, AF_INET6, BLOCK_DNS_IFACE_METRIC);
> + }
> + }
> +
> ret = (status == 0);
>
> out:
>
>
> @@ -1325,6 +1368,7 @@ static VOID
> Undo(undo_lists_t *lists)
> {
> undo_type_t type;
> + block_dns_data_t *interface_data;
> for (type = 0; type < _undo_type_max; type++)
> {
> list_item_t **pnext = &(*lists)[type];
> @@ -1350,8 +1394,15 @@ Undo(undo_lists_t *lists)
> break;
>
> case block_dns:
> - delete_block_dns_filters(item->data);
> - item->data = NULL;
> + interface_data = (block_dns_data_t*)(item->data);
> + delete_block_dns_filters(interface_data->engine);
> + if (interface_data->metric > 0)
>
Here ">" should be ">=0", else automatic metric is not restored.
> + {
> + set_interface_metric(interface_data->index,
> AF_INET,
> + interface_data->metric);
> + set_interface_metric(interface_data->index,
> AF_INET6,
> + interface_data->metric);
> + }
> break;
> }
>
Finally there are some trailing white space errors (6 places). This is a
good
opportunity to fix those too.
Thanks,
Selva
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
