diff mbox series

[Openvpn-devel,v5] lladdr: Clean up code and BSD support

Message ID 20260724220839.26402-1-gert@greenie.muc.de
State New
Headers
Series [Openvpn-devel,v5] lladdr: Clean up code and BSD support |

Commit Message

Gert Doering July 24, 2026, 10:08 p.m. UTC 
  From: Frank Lichtenheld <frank@lichtenheld.com>

cppcheck complained about the creation of a useless
variable on Windows. With the old code this was not
fixable in a good way. So rewrite the whole code to
hopefully be much more readable.

While testing this we also found out that the old
code was not really working on Solaris, so disable
support for that.

Gert Doering contributed support for NetBSD.

Github: closes #1034
Change-Id: Ibe7b3c17176c97a1cbb4d9cc87735b05e62c4f43
Signed-off-by: Frank Lichtenheld <frank@lichtenheld.com>
Acked-by: Gert Doering <gert@greenie.muc.de>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1675
---

This change was reviewed on Gerrit and approved by at least one
developer. I request to merge it to master.

Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1675
This mail reflects revision 5 of this Change.

Acked-by according to Gerrit (reflected above):
Gert Doering <gert@greenie.muc.de>

Comments

Gert Doering July 27, 2026, 10:04 a.m. UTC | #1 
This is a mixture of "code cleanup", "platform support" (add this to
NetBSD) and "platform depreciation" (remove --lladdr support from 
OpenSolaris, because it does not work the way we use it, and probably
didn't ever...).  So "master" it is.

Tested some months ago, re-tested Linux, FreeBSD and NetBSD today.

Side note: Platforms nowadays actually tests for "is this a valid MAC"
(I guess looking at the multicast or the U/L bit) so 01:02:03:04:05:ff
is not :-) and will be reported properly.

Note 2: FreeBSD does actually show "built-in" and "configured" ether
addresses these days

$ ifconfig tap0
tap0: flags=1008843<UP,BROADCAST,RUNNING,SIMPLEX,MULTICAST,LOWER_UP> metric 0 mtu 1500
        ether 58:9c:fc:03:03:03
        hwaddr 58:9c:fc:10:ff:ce

.. which is nice :-)

Note 3: *some* of the OSes base their TAP MAC on "something guid determined
at boot time", so OpenVPN gets the same MAC address every time - which helps
t_client tests (random MACs tend to cause delays due to ARP cache on the
other side).  Others randomize -> --lladdr can help with that...

Note 4: I can see Antonio jump up and down and ask for proper support
for net_addr_ll_set() on more platforms... ;-)

Your patch has been applied to the master branch.

commit 1e918232e20c4d09b4e0dcdbf91fbb5d5cb9cb23 (master)
Author: Frank Lichtenheld
Date:   Sat Jul 25 00:08:32 2026 +0200

     lladdr: Clean up code and BSD support

     Signed-off-by: Frank Lichtenheld <frank@lichtenheld.com>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1675
     Message-Id: <20260724220839.26402-1-gert@greenie.muc.de>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg37863.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/lladdr.c b/src/openvpn/lladdr.c
index d8bcad9..195245f 100644
--- a/src/openvpn/lladdr.c
+++ b/src/openvpn/lladdr.c
@@ -13,42 +13,55 @@ 
 #include "lladdr.h"
 #include "proto.h"
 
+#ifdef TARGET_LINUX
+static int
+set_lladdr_linux(openvpn_net_ctx_t *ctx, const char *ifname, const char *lladdr)
+{
+    uint8_t addr[OPENVPN_ETH_ALEN];
+
+    sscanf(lladdr, MAC_FMT, MAC_SCAN_ARG(addr));
+    return (net_addr_ll_set(ctx, ifname, addr) == 0);
+}
+#else /* TARGET_LINUX */
+
+#if defined(TARGET_OPENBSD) || defined(TARGET_FREEBSD) || defined(TARGET_DARWIN)
+#define IFCONFIG_LLADDR_FMT "%s %s lladdr %s"
+#elif defined(TARGET_NETBSD)
+#define IFCONFIG_LLADDR_FMT "%s %s link %s active"
+#endif
+static int
+set_lladdr_ifconfig(const char *ifname, const char *lladdr, const struct env_set *es)
+{
+#ifdef IFCONFIG_LLADDR_FMT
+    struct argv argv = argv_new();
+    argv_printf(&argv, IFCONFIG_LLADDR_FMT, IFCONFIG_PATH, ifname, lladdr);
+    argv_msg(M_INFO, &argv);
+    int r = openvpn_execve_check(&argv, es, M_WARN, "ERROR: Unable to set link layer address.");
+    argv_free(&argv);
+    return r;
+#else
+    msg(M_WARN,
+        "Sorry, but I don't know how to configure link layer addresses on this operating system.");
+    return -1;
+#endif
+}
+#endif /* TARGET_LINUX */
+
 int
 set_lladdr(openvpn_net_ctx_t *ctx, const char *ifname, const char *lladdr, const struct env_set *es)
 {
-    int r;
-
     if (!ifname || !lladdr)
     {
         return -1;
     }
 
 #if defined(TARGET_LINUX)
-    uint8_t addr[OPENVPN_ETH_ALEN];
+    int r = set_lladdr_linux(ctx, ifname, lladdr);
+#else
+    int r = set_lladdr_ifconfig(ifname, lladdr, es);
+#endif
 
-    sscanf(lladdr, MAC_FMT, MAC_SCAN_ARG(addr));
-    r = (net_addr_ll_set(ctx, ifname, addr) == 0);
-#else /* if defined(TARGET_LINUX) */
-    struct argv argv = argv_new();
-#if defined(TARGET_SOLARIS)
-    argv_printf(&argv, "%s %s ether %s", IFCONFIG_PATH, ifname, lladdr);
-#elif defined(TARGET_OPENBSD)
-    argv_printf(&argv, "%s %s lladdr %s", IFCONFIG_PATH, ifname, lladdr);
-#elif defined(TARGET_DARWIN)
-    argv_printf(&argv, "%s %s lladdr %s", IFCONFIG_PATH, ifname, lladdr);
-#elif defined(TARGET_FREEBSD)
-    argv_printf(&argv, "%s %s ether %s", IFCONFIG_PATH, ifname, lladdr);
-#else  /* if defined(TARGET_SOLARIS) */
-    msg(M_WARN,
-        "Sorry, but I don't know how to configure link layer addresses on this operating system.");
-    return -1;
-#endif /* if defined(TARGET_SOLARIS) */
-    argv_msg(M_INFO, &argv);
-    r = openvpn_execve_check(&argv, es, M_WARN, "ERROR: Unable to set link layer address.");
-    argv_free(&argv);
-#endif /* if defined(TARGET_LINUX) */
-
-    if (r)
+    if (r > 0)
     {
         msg(M_INFO, "TUN/TAP link layer address set to %s", lladdr);
     }