Menu
▾
▴
openvpn-devel
|
From: stipa (C. Review) <ge...@op...> - 2025-07-18 15:53:28
|
Attention is currently required from: flichtenheld, plaisthos.
Hello plaisthos, flichtenheld,
I'd like you to do a code review.
Please visit
http://gerrit.openvpn.net/c/openvpn/+/1090?usp=email
to review the following change.
Change subject: Fix broken DHCP options
......................................................................
Fix broken DHCP options
Commit
2dfc4f ("dns: deal with --dhcp-options when --dns is active")
broke support for --dhcp-options. It removed the setting of the
DHCP_OPTIONS_DHCP_OPTIONAL flag for some DHCP options. This flag is
required for those options to be applied correctly, as it is used
when building the DHCP options string that is passed to the TAP driver.
This commit fixes the issue by restoring the setting of this flag.
GitHub: #791
Change-Id: I0d75efcceb826d06e74abd003d5377468ff9fe3b
Signed-off-by: Lev Stipakov <le...@op...>
---
M src/openvpn/options.c
1 file changed, 13 insertions(+), 0 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/90/1090/1
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 0662b49..4ba9243 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -8310,9 +8310,12 @@
#endif
VERIFY_PERMISSION(OPT_P_DHCPDNS);
+ bool dhcp_optional = false;
+
if ((streq(p[1], "DOMAIN") || streq(p[1], "ADAPTER_DOMAIN_SUFFIX")) && p[2] && !p[3])
{
dhcp->domain = p[2];
+ dhcp_optional = true;
}
else if (streq(p[1], "DOMAIN-SEARCH") && p[2] && !p[3])
{
@@ -8325,6 +8328,7 @@
msg(msglevel, "--dhcp-option %s: maximum of %d search entries can be specified",
p[1], N_SEARCH_LIST_LEN);
}
+ dhcp_optional = true;
}
else if ((streq(p[1], "DNS") || streq(p[1], "DNS6")) && p[2] && !p[3]
&& (!strstr(p[2], ":") || ipv6_addr_safe(p[2])))
@@ -8336,6 +8340,7 @@
else
{
dhcp_option_address_parse("DNS", p[2], dhcp->dns, &dhcp->dns_len, msglevel);
+ dhcp_optional = true;
}
}
#if defined(_WIN32) || defined(TARGET_ANDROID)
@@ -8359,6 +8364,7 @@
else if (streq(p[1], "WINS") && p[2] && !p[3])
{
dhcp_option_address_parse("WINS", p[2], o->wins, &o->wins_len, msglevel);
+ o->dhcp_options |= DHCP_OPTIONS_DHCP_OPTIONAL;
}
else if (streq(p[1], "NTP") && p[2] && !p[3])
{
@@ -8390,6 +8396,13 @@
#else /* if defined(_WIN32) || defined(TARGET_ANDROID) */
setenv_foreign_option(options, (const char **)p, 3, es);
#endif /* if defined(_WIN32) || defined(TARGET_ANDROID) */
+
+ if (dhcp_optional)
+ {
+#if defined(_WIN32) || defined(TARGET_ANDROID)
+ o->dhcp_options |= DHCP_OPTIONS_DHCP_OPTIONAL;
+#endif
+ }
}
#ifdef _WIN32
else if (streq(p[0], "show-adapters") && !p[1])
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1090?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I0d75efcceb826d06e74abd003d5377468ff9fe3b
Gerrit-Change-Number: 1090
Gerrit-PatchSet: 1
Gerrit-Owner: stipa <lst...@gm...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-MessageType: newchange
|
|
From: cron2 (C. Review) <ge...@op...> - 2025-07-30 18:04:24
|
Attention is currently required from: flichtenheld, plaisthos, stipa. cron2 has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/1090?usp=email ) Change subject: Fix broken DHCP options ...................................................................... Patch Set 1: Code-Review+2 (1 comment) Patchset: PS1: This plays together with the code in tun.c that checks whether or not the TAP driver needs to be informed about DHCP options ``` /* user-supplied DHCP options capability */ if (tt->options.dhcp_options) { .. if (!DeviceIoControl(tt->hand, TAP_WIN_IOCTL_CONFIG_DHCP_SET_OPT, .. ``` ... so if we no longer flag these "it can be done with DHCP or not" options as "well, if we have DHCP, we should set them!" (DHCP_OPTIONS_DHCP_OPTIONAL), and we have no "this can only be done with DHCP!" options (DHCP_OPTIONS_DHCP_REQUIRED), then tun.c just assumes "well, nothing required, nothing to set up" and DNS will not work. I experienced the issue myself with 2.7_alpha2 and a setup that ended up using TAP and DHCP, and the explanation makes sense. -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1090?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I0d75efcceb826d06e74abd003d5377468ff9fe3b Gerrit-Change-Number: 1090 Gerrit-PatchSet: 1 Gerrit-Owner: stipa <lst...@gm...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: stipa <lst...@gm...> Gerrit-Comment-Date: Wed, 30 Jul 2025 18:04:15 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment |
|
From: Gert D. <ge...@gr...> - 2025-07-30 18:05:40
|
From: Lev Stipakov <le...@op...>
Commit
2dfc4f ("dns: deal with --dhcp-options when --dns is active")
broke support for --dhcp-options. It removed the setting of the
DHCP_OPTIONS_DHCP_OPTIONAL flag for some DHCP options. This flag is
required for those options to be applied correctly, as it is used
when building the DHCP options string that is passed to the TAP driver.
This commit fixes the issue by restoring the setting of this flag.
GitHub: fixes OpenVPN/openvpn#791
Change-Id: I0d75efcceb826d06e74abd003d5377468ff9fe3b
Signed-off-by: Lev Stipakov <le...@op...>
Acked-by: Gert Doering <ge...@gr...>
---
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/+/1090
This mail reflects revision 1 of this Change.
Acked-by according to Gerrit (reflected above):
Gert Doering <ge...@gr...>
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 0662b49..4ba9243 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -8310,9 +8310,12 @@
#endif
VERIFY_PERMISSION(OPT_P_DHCPDNS);
+ bool dhcp_optional = false;
+
if ((streq(p[1], "DOMAIN") || streq(p[1], "ADAPTER_DOMAIN_SUFFIX")) && p[2] && !p[3])
{
dhcp->domain = p[2];
+ dhcp_optional = true;
}
else if (streq(p[1], "DOMAIN-SEARCH") && p[2] && !p[3])
{
@@ -8325,6 +8328,7 @@
msg(msglevel, "--dhcp-option %s: maximum of %d search entries can be specified",
p[1], N_SEARCH_LIST_LEN);
}
+ dhcp_optional = true;
}
else if ((streq(p[1], "DNS") || streq(p[1], "DNS6")) && p[2] && !p[3]
&& (!strstr(p[2], ":") || ipv6_addr_safe(p[2])))
@@ -8336,6 +8340,7 @@
else
{
dhcp_option_address_parse("DNS", p[2], dhcp->dns, &dhcp->dns_len, msglevel);
+ dhcp_optional = true;
}
}
#if defined(_WIN32) || defined(TARGET_ANDROID)
@@ -8359,6 +8364,7 @@
else if (streq(p[1], "WINS") && p[2] && !p[3])
{
dhcp_option_address_parse("WINS", p[2], o->wins, &o->wins_len, msglevel);
+ o->dhcp_options |= DHCP_OPTIONS_DHCP_OPTIONAL;
}
else if (streq(p[1], "NTP") && p[2] && !p[3])
{
@@ -8390,6 +8396,13 @@
#else /* if defined(_WIN32) || defined(TARGET_ANDROID) */
setenv_foreign_option(options, (const char **)p, 3, es);
#endif /* if defined(_WIN32) || defined(TARGET_ANDROID) */
+
+ if (dhcp_optional)
+ {
+#if defined(_WIN32) || defined(TARGET_ANDROID)
+ o->dhcp_options |= DHCP_OPTIONS_DHCP_OPTIONAL;
+#endif
+ }
}
#ifdef _WIN32
else if (streq(p[0], "show-adapters") && !p[1])
|
|
From: Gert D. <ge...@gr...> - 2025-07-30 18:15:25
|
Thanks. I've ran into this myself with 2.7_alpha2 and a setup that
ended up doing DHCP on TAP, so thanks for the fix :-)
I have not tested it beyond "does it compile", but looking at the code
flow and finding tt->options.dhcp_options in tun.c, this is "obviously
correct". The dance with the helper variable is unavoidable due to
the platform-dependent nature of the options.c code here... ("or you
get 3 extra #ifdefs").
Your patch has been applied to the master branch.
commit 040e0d1c308a587ae9a26295eeaa070561befa0d
Author: Lev Stipakov
Date: Wed Jul 30 20:04:26 2025 +0200
Fix broken DHCP options
Signed-off-by: Lev Stipakov <le...@op...>
Acked-by: Gert Doering <ge...@gr...>
Message-Id: <202...@gr...>
URL: https://www.mail-archive.com/ope...@li.../msg32427.html
Signed-off-by: Gert Doering <ge...@gr...>
--
kind regards,
Gert Doering
|
|
From: cron2 (C. Review) <ge...@op...> - 2025-07-30 18:15:50
|
cron2 has uploaded a new patch set (#2) to the change originally created by stipa. ( http://gerrit.openvpn.net/c/openvpn/+/1090?usp=email ) The following approvals got outdated and were removed: Code-Review+2 by cron2 Change subject: Fix broken DHCP options ...................................................................... Fix broken DHCP options Commit 2dfc4f ("dns: deal with --dhcp-options when --dns is active") broke support for --dhcp-options. It removed the setting of the DHCP_OPTIONS_DHCP_OPTIONAL flag for some DHCP options. This flag is required for those options to be applied correctly, as it is used when building the DHCP options string that is passed to the TAP driver. This commit fixes the issue by restoring the setting of this flag. GitHub: fixes OpenVPN/openvpn#791 Change-Id: I0d75efcceb826d06e74abd003d5377468ff9fe3b Signed-off-by: Lev Stipakov <le...@op...> Acked-by: Gert Doering <ge...@gr...> Message-Id: <202...@gr...> URL: https://www.mail-archive.com/ope...@li.../msg32427.html Signed-off-by: Gert Doering <ge...@gr...> --- M src/openvpn/options.c 1 file changed, 13 insertions(+), 0 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/90/1090/2 diff --git a/src/openvpn/options.c b/src/openvpn/options.c index b7328d7..53be6f5 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -8712,9 +8712,12 @@ #endif VERIFY_PERMISSION(OPT_P_DHCPDNS); + bool dhcp_optional = false; + if ((streq(p[1], "DOMAIN") || streq(p[1], "ADAPTER_DOMAIN_SUFFIX")) && p[2] && !p[3]) { dhcp->domain = p[2]; + dhcp_optional = true; } else if (streq(p[1], "DOMAIN-SEARCH") && p[2] && !p[3]) { @@ -8727,6 +8730,7 @@ msg(msglevel, "--dhcp-option %s: maximum of %d search entries can be specified", p[1], N_SEARCH_LIST_LEN); } + dhcp_optional = true; } else if ((streq(p[1], "DNS") || streq(p[1], "DNS6")) && p[2] && !p[3] && (!strstr(p[2], ":") || ipv6_addr_safe(p[2]))) @@ -8738,6 +8742,7 @@ else { dhcp_option_address_parse("DNS", p[2], dhcp->dns, &dhcp->dns_len, msglevel); + dhcp_optional = true; } } #if defined(_WIN32) || defined(TARGET_ANDROID) @@ -8761,6 +8766,7 @@ else if (streq(p[1], "WINS") && p[2] && !p[3]) { dhcp_option_address_parse("WINS", p[2], o->wins, &o->wins_len, msglevel); + o->dhcp_options |= DHCP_OPTIONS_DHCP_OPTIONAL; } else if (streq(p[1], "NTP") && p[2] && !p[3]) { @@ -8792,6 +8798,13 @@ #else /* if defined(_WIN32) || defined(TARGET_ANDROID) */ setenv_foreign_option(options, p[1], p[2], es); #endif /* if defined(_WIN32) || defined(TARGET_ANDROID) */ + + if (dhcp_optional) + { +#if defined(_WIN32) || defined(TARGET_ANDROID) + o->dhcp_options |= DHCP_OPTIONS_DHCP_OPTIONAL; +#endif + } } #ifdef _WIN32 else if (streq(p[0], "show-adapters") && !p[1]) -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1090?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I0d75efcceb826d06e74abd003d5377468ff9fe3b Gerrit-Change-Number: 1090 Gerrit-PatchSet: 2 Gerrit-Owner: stipa <lst...@gm...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-MessageType: newpatchset |
|
From: cron2 (C. Review) <ge...@op...> - 2025-07-30 18:15:52
|
cron2 has submitted this change. ( http://gerrit.openvpn.net/c/openvpn/+/1090?usp=email ) Change subject: Fix broken DHCP options ...................................................................... Fix broken DHCP options Commit 2dfc4f ("dns: deal with --dhcp-options when --dns is active") broke support for --dhcp-options. It removed the setting of the DHCP_OPTIONS_DHCP_OPTIONAL flag for some DHCP options. This flag is required for those options to be applied correctly, as it is used when building the DHCP options string that is passed to the TAP driver. This commit fixes the issue by restoring the setting of this flag. GitHub: fixes OpenVPN/openvpn#791 Change-Id: I0d75efcceb826d06e74abd003d5377468ff9fe3b Signed-off-by: Lev Stipakov <le...@op...> Acked-by: Gert Doering <ge...@gr...> Message-Id: <202...@gr...> URL: https://www.mail-archive.com/ope...@li.../msg32427.html Signed-off-by: Gert Doering <ge...@gr...> --- M src/openvpn/options.c 1 file changed, 13 insertions(+), 0 deletions(-) diff --git a/src/openvpn/options.c b/src/openvpn/options.c index b7328d7..53be6f5 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -8712,9 +8712,12 @@ #endif VERIFY_PERMISSION(OPT_P_DHCPDNS); + bool dhcp_optional = false; + if ((streq(p[1], "DOMAIN") || streq(p[1], "ADAPTER_DOMAIN_SUFFIX")) && p[2] && !p[3]) { dhcp->domain = p[2]; + dhcp_optional = true; } else if (streq(p[1], "DOMAIN-SEARCH") && p[2] && !p[3]) { @@ -8727,6 +8730,7 @@ msg(msglevel, "--dhcp-option %s: maximum of %d search entries can be specified", p[1], N_SEARCH_LIST_LEN); } + dhcp_optional = true; } else if ((streq(p[1], "DNS") || streq(p[1], "DNS6")) && p[2] && !p[3] && (!strstr(p[2], ":") || ipv6_addr_safe(p[2]))) @@ -8738,6 +8742,7 @@ else { dhcp_option_address_parse("DNS", p[2], dhcp->dns, &dhcp->dns_len, msglevel); + dhcp_optional = true; } } #if defined(_WIN32) || defined(TARGET_ANDROID) @@ -8761,6 +8766,7 @@ else if (streq(p[1], "WINS") && p[2] && !p[3]) { dhcp_option_address_parse("WINS", p[2], o->wins, &o->wins_len, msglevel); + o->dhcp_options |= DHCP_OPTIONS_DHCP_OPTIONAL; } else if (streq(p[1], "NTP") && p[2] && !p[3]) { @@ -8792,6 +8798,13 @@ #else /* if defined(_WIN32) || defined(TARGET_ANDROID) */ setenv_foreign_option(options, p[1], p[2], es); #endif /* if defined(_WIN32) || defined(TARGET_ANDROID) */ + + if (dhcp_optional) + { +#if defined(_WIN32) || defined(TARGET_ANDROID) + o->dhcp_options |= DHCP_OPTIONS_DHCP_OPTIONAL; +#endif + } } #ifdef _WIN32 else if (streq(p[0], "show-adapters") && !p[1]) -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1090?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I0d75efcceb826d06e74abd003d5377468ff9fe3b Gerrit-Change-Number: 1090 Gerrit-PatchSet: 2 Gerrit-Owner: stipa <lst...@gm...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-MessageType: merged |
×
Want the latest updates on software, tech news, and AI?
Get latest updates about software, tech news, and AI from SourceForge directly in your inbox once a month.
Thanks for helping keep SourceForge clean.
X