[Openvpn-devel] Refactor get_interface_metric to return metric and auto flag separately

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
Related show

Commit Message

Selva Nair Dec. 6, 2017, 4:28 a.m.
From: Selva Nair <selva.nair@gmail.com>

- 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.

Signed-off-by: Selva Nair <selva.nair@gmail.com>
---
 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(-)

Comments

Simon Rozman Dec. 6, 2017, 5:18 p.m. | #1
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
Selva Nair Dec. 6, 2017, 5:49 p.m. | #2
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">&lt;<a href="mailto:simon@rozman.si" target="_blank">simon@rozman.si</a>&gt;</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&#39;ll notice exactly the same repeating pattern:<br>
<span><br>
        tap_metric_v4 = get_interface_metric(index, AF_INET, &amp;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, &amp;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&#39;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
Simon Rozman Dec. 10, 2017, 12:25 p.m. | #3
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
Gert Doering Jan. 25, 2018, 2:41 p.m. | #4
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

Patch

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)