| Message ID | 1512534521-14760-1-git-send-email-selva.nair@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel] Refactor get_interface_metric to return metric and auto flag separately | expand |
Hi,
I have briefly reviewed this patch. If you look at each get_interface_metric() call you'll notice exactly the same repeating pattern:
tap_metric_v4 = get_interface_metric(index, AF_INET, &is_auto);
if (is_auto)
{
tap_metric_v4 = 0;
}
tap_metric_v6 = get_interface_metric(index, AF_INET6, &is_auto);
if (is_auto)
{
tap_metric_v6 = 0;
}
Should the get_interface_metric() be rewritten to return:
0 = automatic metric
n = manual metric
-1 = error
...all the get_interface_metric() would simplify to:
tap_metric_v4 = get_interface_metric(index, AF_INET);
tap_metric_v6 = get_interface_metric(index, AF_INET6);
Just a suggestion.
Best regards,
Simon
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Hi, On Wed, Dec 6, 2017 at 12:18 PM, Simon Rozman <simon@rozman.si> wrote: > Hi, > > I have briefly reviewed this patch. If you look at each > get_interface_metric() call you'll notice exactly the same repeating > pattern: > > tap_metric_v4 = get_interface_metric(index, AF_INET, &is_auto); > if (is_auto) > { > tap_metric_v4 = 0; > } > tap_metric_v6 = get_interface_metric(index, AF_INET6, &is_auto); > if (is_auto) > { > tap_metric_v6 = 0; > } > > Should the get_interface_metric() be rewritten to return: > 0 = automatic metric > n = manual metric > > That's exactly what the original version does and the purpose of the patch is to not do so. Else this function is not useful when someone really wants to find what the metric is as the value of 0 hides that info. Note that even when automatic metric is in use the interface has some metric set. I want to use it in another patch where the actual metric is required even if automatic metric is in use. Selva <div dir="ltr">Hi,<br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Dec 6, 2017 at 12:18 PM, Simon Rozman <span dir="ltr"><<a href="mailto:simon@rozman.si" target="_blank">simon@rozman.si</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi,<br> <br> I have briefly reviewed this patch. If you look at each get_interface_metric() call you'll notice exactly the same repeating pattern:<br> <span><br> tap_metric_v4 = get_interface_metric(index, AF_INET, &is_auto);<br> </span> if (is_auto)<br> {<br> tap_metric_v4 = 0;<br> <span> }<br> tap_metric_v6 = get_interface_metric(index, AF_INET6, &is_auto);<br> </span> if (is_auto)<br> {<br> tap_metric_v6 = 0;<br> }<br> <br> Should the get_interface_metric() be rewritten to return:<br> 0 = automatic metric<br> n = manual metric<br> <br></blockquote><div><br></div><div>That's exactly what the original version does and the purpose of the patch</div><div>is to not do so. Else this function is not useful when someone really wants to</div><div>find what the metric is as the value of 0 hides that info. Note that even when</div><div>automatic metric is in use the interface has some metric set.</div><div><br></div><div>I want to use it in another patch where the actual metric is required even</div><div>if automatic metric is in use.</div><div><br></div><div>Selva</div></div></div></div> ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Hi, > That's exactly what the original version does and the purpose of the patch > is > to not do so. Else this function is not useful when someone really wants to > find what the metric is as the value of 0 hides that info. Note that even > when > automatic metric is in use the interface has some metric set. > > I want to use it in another patch where the actual metric is required even > if > automatic metric is in use. Makes sense now. Sorry for the delay. Acked-by: Simon Rozman <simon@rozman.si> Best regards, Simon ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Your patch has been applied to the master and release/2.4 branch.
Sorry for taking so long.
Test built on my new and shiny ubuntu 16.04 test rig :-)
commit 4229243563bcb22990f71d50e25be9ea6d44f519 (master)
commit 92b4c0e96f797775df18269bc7295dc163688cb4 (release/2.4)
Author: Selva Nair
Date: Tue Dec 5 23:28:41 2017 -0500
Refactor get_interface_metric to return metric and auto flag separately
Signed-off-by: Selva Nair <selva.nair@gmail.com>
Acked-by: Simon Rozman <simon@rozman.si>
Message-Id: <1512534521-14760-1-git-send-email-selva.nair@gmail.com>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg16039.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
--
kind regards,
Gert Doering
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
diff --git a/src/openvpn/block_dns.c b/src/openvpn/block_dns.c index d43cbcf..8d5011d 100644 --- a/src/openvpn/block_dns.c +++ b/src/openvpn/block_dns.c @@ -344,33 +344,43 @@ delete_block_dns_filters(HANDLE engine_handle) } /* - * Returns interface metric value for specified interface index. + * Return interface metric value for the 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. + * is_auto : On return set to true if automatic metric is in use. + * Unused if NULL. + * + * Returns positive metric value or -1 on error. */ - int -get_interface_metric(const NET_IFINDEX index, const ADDRESS_FAMILY family) +get_interface_metric(const NET_IFINDEX index, const ADDRESS_FAMILY family, int *is_auto) { DWORD err = 0; MIB_IPINTERFACE_ROW ipiface; InitializeIpInterfaceEntry(&ipiface); ipiface.Family = family; ipiface.InterfaceIndex = index; + + if (is_auto) + { + *is_auto = 0; + } err = GetIpInterfaceEntry(&ipiface); - if (err == NO_ERROR) + + /* On Windows metric is never > INT_MAX so return value of int is ok. + * But we check for overflow nevertheless. + */ + if (err == NO_ERROR && ipiface.Metric <= INT_MAX) { - if (ipiface.UseAutomaticMetric) + if (is_auto) { - return 0; + *is_auto = ipiface.UseAutomaticMetric; } - return ipiface.Metric; + return (int)ipiface.Metric; } - return -err; + return -1; } /* diff --git a/src/openvpn/block_dns.h b/src/openvpn/block_dns.h index c9a9d70..50b383f 100644 --- a/src/openvpn/block_dns.h +++ b/src/openvpn/block_dns.h @@ -39,17 +39,17 @@ add_block_dns_filters(HANDLE *engine, int iface_index, const WCHAR *exe_path, block_dns_msg_handler_t msg_handler_callback); /** - * Returns interface metric value for specified interface index. + * Return interface metric value for the specified interface index. * - * @param index The index of TAP adapter - * @param family Address family (AF_INET for IPv4 and AF_INET6 for IPv6) + * @param index The index of TAP adapter. + * @param family Address family (AF_INET for IPv4 and AF_INET6 for IPv6). + * @param is_auto On return set to true if automatic metric is in use. + * Unused if NULL. * - * @return positive metric value or zero for automatic metric on success, - * a less then zero error code on failure. + * @return positive interface metric on success or -1 on error */ - int -get_interface_metric(const NET_IFINDEX index, const ADDRESS_FAMILY family); +get_interface_metric(const NET_IFINDEX index, const ADDRESS_FAMILY family, int *is_auto); /** * Sets interface metric value for specified interface index. diff --git a/src/openvpn/win32.c b/src/openvpn/win32.c index 95fea5d..4b20298 100644 --- a/src/openvpn/win32.c +++ b/src/openvpn/win32.c @@ -1344,17 +1344,16 @@ win_wfp_block_dns(const NET_IFINDEX index, const HANDLE msg_channel) block_dns_msg_handler); if (status == 0) { - tap_metric_v4 = get_interface_metric(index, AF_INET); - tap_metric_v6 = get_interface_metric(index, AF_INET6); - if (tap_metric_v4 < 0) + int is_auto = 0; + tap_metric_v4 = get_interface_metric(index, AF_INET, &is_auto); + if (is_auto) { - /* error, should not restore metric */ - tap_metric_v4 = -1; + tap_metric_v4 = 0; } - if (tap_metric_v6 < 0) + tap_metric_v6 = get_interface_metric(index, AF_INET6, &is_auto); + if (is_auto) { - /* error, should not restore metric */ - tap_metric_v6 = -1; + tap_metric_v6 = 0; } status = set_interface_metric(index, AF_INET, BLOCK_DNS_IFACE_METRIC); if (!status) diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c index aab784e..3907438 100644 --- a/src/openvpnserv/interactive.c +++ b/src/openvpnserv/interactive.c @@ -804,17 +804,18 @@ HandleBlockDNSMessage(const block_dns_message_t *msg, undo_lists_t *lists) } interface_data->engine = engine; interface_data->index = msg->iface.index; + int is_auto = 0; interface_data->metric_v4 = get_interface_metric(msg->iface.index, - AF_INET); - if (interface_data->metric_v4 < 0) + AF_INET, &is_auto); + if (is_auto) { - interface_data->metric_v4 = -1; + interface_data->metric_v4 = 0; } interface_data->metric_v6 = get_interface_metric(msg->iface.index, - AF_INET6); - if (interface_data->metric_v6 < 0) + AF_INET6, &is_auto); + if (is_auto) { - interface_data->metric_v6 = -1; + interface_data->metric_v6 = 0; } err = AddListItem(&(*lists)[block_dns], interface_data); if (!err)