Refactor get_interface_metric to return metric and auto flag separately
PATCH 1/1 Refactor get_interface_metric to return metric and auto flag separately
From: Selva Nair <[email protected]...>
- Instead of returning metric = 0 when automatic metric is in use return the actual metric and flag automatic metric through a parameter. This makes the function reusable elsewhere.
- Ensure return value can be correctly cast to int and return -1 on error.
--- src/openvpn/block_dns.c | 30 ++++++++++++++++++++---------- src/openvpn/block_dns.h | 14 +++++++------- src/openvpn/win32.c | 15 +++++++-------- src/openvpnserv/interactive.c | 13 +++++++------ 4 files changed, 41 insertions(+), 31 deletions(-)
src/openvpn/block_dns.c +20 -10
@@ -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 ; }/*
src/openvpn/block_dns.h +7 -7
@@ -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.
src/openvpn/win32.c +7 -8
@@ -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)
src/openvpnserv/interactive.c +7 -6
@@ -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)
Signature · 1 line
2.1.4
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
Hi,
Quoted 17 lines · Simon Rozman
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; }
Quoted 5 lines
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
Simon Rozman
Acked-byHi,
Quoted 10 lines
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.
Best regards,
Simon
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 <[email protected]...>
Acked-by: Simon Rozman <[email protected]...>
Message-Id: <[email protected]...>
URL: https://www.mail-archive.com/[email protected]/msg16039.html
Signed-off-by: Gert Doering <[email protected]...>--
kind regards,
Gert Doering