openvpn-devel

[Openvpn-devel] [M] Change in openvpn[master]: options: Clean up function setenv_foreign_option

[flichtenheld (C. Review) <ge...@op...>]

Attention is currently required from: plaisthos.

Hello plaisthos,

I'd like you to do a code review.
Please visit

    http://gerrit.openvpn.net/c/openvpn/+/1112?usp=email

to review the following change.


Change subject: options: Clean up function setenv_foreign_option
......................................................................

options: Clean up function setenv_foreign_option

- Remove unnecessary len > 0 check by reordering
  the code slightly.
- Remove -Wconversion warning by making the len argument
  size_t.

Change-Id: I7efc77f63734501dfa8a8f5bed17b1a1b4e9e201
Signed-off-by: Frank Lichtenheld <fr...@li...>
---
M src/openvpn/options.c
1 file changed, 25 insertions(+), 29 deletions(-)



  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/12/1112/1

diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 70b5799..4d94211 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -1067,41 +1067,37 @@
 
 #ifndef _WIN32
 static void
-setenv_foreign_option(struct options *o, const char *argv[], int len, struct env_set *es)
+setenv_foreign_option(struct options *o, const char *argv[], size_t len, struct env_set *es)
 {
-    if (len > 0)
-    {
-        struct gc_arena gc = gc_new();
-        struct buffer name = alloc_buf_gc(OPTION_PARM_SIZE, &gc);
-        struct buffer value = alloc_buf_gc(OPTION_PARM_SIZE, &gc);
-        int i;
-        bool first = true;
-        bool good = true;
+    struct gc_arena gc = gc_new();
+    struct buffer name = alloc_buf_gc(OPTION_PARM_SIZE, &gc);
+    struct buffer value = alloc_buf_gc(OPTION_PARM_SIZE, &gc);
+    bool first = true;
+    bool good = true;
 
-        good &= buf_printf(&name, "foreign_option_%d", o->foreign_option_index + 1);
-        ++o->foreign_option_index;
-        for (i = 0; i < len; ++i)
+    good &= buf_printf(&name, "foreign_option_%d", o->foreign_option_index + 1);
+    for (size_t i = 0; i < len; ++i)
+    {
+        if (argv[i])
         {
-            if (argv[i])
+            if (!first)
             {
-                if (!first)
-                {
-                    good &= buf_printf(&value, " ");
-                }
-                good &= buf_printf(&value, "%s", argv[i]);
-                first = false;
+                good &= buf_printf(&value, " ");
             }
+            good &= buf_printf(&value, "%s", argv[i]);
+            first = false;
         }
-        if (good)
-        {
-            setenv_str(es, BSTR(&name), BSTR(&value));
-        }
-        else
-        {
-            msg(M_WARN, "foreign_option: name/value overflow");
-        }
-        gc_free(&gc);
     }
+    if (good)
+    {
+        setenv_str(es, BSTR(&name), BSTR(&value));
+        ++o->foreign_option_index;
+    }
+    else
+    {
+        msg(M_WARN, "foreign_option: name/value overflow");
+    }
+    gc_free(&gc);
 }
 #endif /* ifndef _WIN32 */
 
@@ -3679,7 +3675,7 @@
     {
         /* Set foreign option env vars from --dns config */
         const char *p[] = { "dhcp-option", NULL, NULL };
-        size_t p_len = sizeof(p) / sizeof(p[0]);
+        const size_t p_len = sizeof(p) / sizeof(p[0]);
 
         p[1] = "DOMAIN";
         const struct dns_domain *d = dns->search_domains;

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1112?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: I7efc77f63734501dfa8a8f5bed17b1a1b4e9e201
Gerrit-Change-Number: 1112
Gerrit-PatchSet: 1
Gerrit-Owner: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-MessageType: newchange

[Openvpn-devel] [M] Change in openvpn[master]: options: Clean up function setenv_foreign_option

[cron2 (C. Review) <ge...@op...>]

Attention is currently required from: flichtenheld, plaisthos.

cron2 has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/1112?usp=email )

Change subject: options: Clean up function setenv_foreign_option
......................................................................


Patch Set 1:

(1 comment)

Patchset:

PS1: 
I'm not convinced we couldn't do something more radical, like, getting rid of the loop and `first` and all that, since all callers call `setenv_foreign_option()` with `len == 3`...  (unless I misread something)



-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1112?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: I7efc77f63734501dfa8a8f5bed17b1a1b4e9e201
Gerrit-Change-Number: 1112
Gerrit-PatchSet: 1
Gerrit-Owner: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: cron2 <ge...@gr...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-Comment-Date: Fri, 25 Jul 2025 18:19:35 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[Openvpn-devel] [M] Change in openvpn[master]: options: Simplify function setenv_foreign_option

[flichtenheld (C. Review) <ge...@op...>]

Attention is currently required from: flichtenheld, plaisthos.

Hello plaisthos, 

I'd like you to reexamine a change. Please visit

    http://gerrit.openvpn.net/c/openvpn/+/1112?usp=email

to look at the new patch set (#2).


Change subject: options: Simplify function setenv_foreign_option
......................................................................

options: Simplify function setenv_foreign_option

This was relatively complex for the actual usage.
Looked at the code because of -Wconversion warnings
related to the len argument. So this should also be
gone.

Change-Id: I7efc77f63734501dfa8a8f5bed17b1a1b4e9e201
Signed-off-by: Frank Lichtenheld <fr...@li...>
---
M src/openvpn/options.c
1 file changed, 33 insertions(+), 45 deletions(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/12/1112/2

diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 70b5799..8757581 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -1067,41 +1067,32 @@
 
 #ifndef _WIN32
 static void
-setenv_foreign_option(struct options *o, const char *argv[], int len, struct env_set *es)
+setenv_foreign_option(struct options *o, const char *option, const char *value, struct env_set *es)
 {
-    if (len > 0)
-    {
-        struct gc_arena gc = gc_new();
-        struct buffer name = alloc_buf_gc(OPTION_PARM_SIZE, &gc);
-        struct buffer value = alloc_buf_gc(OPTION_PARM_SIZE, &gc);
-        int i;
-        bool first = true;
-        bool good = true;
+    struct gc_arena gc = gc_new();
+    struct buffer env_name = alloc_buf_gc(OPTION_PARM_SIZE, &gc);
+    struct buffer env_value = alloc_buf_gc(OPTION_PARM_SIZE, &gc);
+    bool good = true;
 
-        good &= buf_printf(&name, "foreign_option_%d", o->foreign_option_index + 1);
-        ++o->foreign_option_index;
-        for (i = 0; i < len; ++i)
-        {
-            if (argv[i])
-            {
-                if (!first)
-                {
-                    good &= buf_printf(&value, " ");
-                }
-                good &= buf_printf(&value, "%s", argv[i]);
-                first = false;
-            }
-        }
-        if (good)
-        {
-            setenv_str(es, BSTR(&name), BSTR(&value));
-        }
-        else
-        {
-            msg(M_WARN, "foreign_option: name/value overflow");
-        }
-        gc_free(&gc);
+    good &= buf_printf(&env_name, "foreign_option_%d", o->foreign_option_index + 1);
+    if (value)
+    {
+        good &= buf_printf(&env_value, "dhcp-option %s %s", option, value);
     }
+    else
+    {
+        good &= buf_printf(&env_value, "dhcp-option %s", option);
+    }
+    if (good)
+    {
+        setenv_str(es, BSTR(&env_name), BSTR(&env_value));
+        ++o->foreign_option_index;
+    }
+    else
+    {
+        msg(M_WARN, "foreign_option: name/value overflow");
+    }
+    gc_free(&gc);
 }
 #endif /* ifndef _WIN32 */
 
@@ -3678,15 +3669,10 @@
     else if (o->up_script && !dns_updown_user_set(dns) && !dns_updown_forced(dns))
     {
         /* Set foreign option env vars from --dns config */
-        const char *p[] = { "dhcp-option", NULL, NULL };
-        size_t p_len = sizeof(p) / sizeof(p[0]);
-
-        p[1] = "DOMAIN";
         const struct dns_domain *d = dns->search_domains;
         while (d)
         {
-            p[2] = d->name;
-            setenv_foreign_option(o, (const char **)p, p_len, es);
+            setenv_foreign_option(o, "DOMAIN", d->name, es);
             d = d->next;
         }
 
@@ -3713,17 +3699,19 @@
             {
                 for (int i = 0; i < s->addr_count; ++i)
                 {
+                    const char *option;
+                    const char *value;
                     if (s->addr[i].family == AF_INET)
                     {
-                        p[1] = "DNS";
-                        p[2] = print_in_addr_t(s->addr[i].in.a4.s_addr, IA_NET_ORDER, &gc);
+                        option = "DNS";
+                        value = print_in_addr_t(s->addr[i].in.a4.s_addr, IA_NET_ORDER, &gc);
                     }
                     else
                     {
-                        p[1] = "DNS6";
-                        p[2] = print_in6_addr(s->addr[i].in.a6, 0, &gc);
+                        option = "DNS6";
+                        value = print_in6_addr(s->addr[i].in.a6, 0, &gc);
                     }
-                    setenv_foreign_option(o, (const char **)p, p_len, es);
+                    setenv_foreign_option(o, option, value, es);
                 }
                 break;
             }
@@ -8388,7 +8376,7 @@
             goto err;
         }
 #else /* if defined(_WIN32) || defined(TARGET_ANDROID) */
-        setenv_foreign_option(options, (const char **)p, 3, es);
+        setenv_foreign_option(options, p[1], p[2], es);
 #endif /* if defined(_WIN32) || defined(TARGET_ANDROID) */
     }
 #ifdef _WIN32
@@ -8530,7 +8518,7 @@
     else if (streq(p[0], "dhcp-option") && p[1] && !p[3])
     {
         VERIFY_PERMISSION(OPT_P_DHCPDNS);
-        setenv_foreign_option(options, (const char **)p, 3, es);
+        setenv_foreign_option(options, p[1], p[2], es);
     }
     else if (streq(p[0], "route-method") && p[1] && !p[2])
     {

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1112?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: I7efc77f63734501dfa8a8f5bed17b1a1b4e9e201
Gerrit-Change-Number: 1112
Gerrit-PatchSet: 2
Gerrit-Owner: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: cron2 <ge...@gr...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-MessageType: newpatchset

[Openvpn-devel] [M] Change in openvpn[master]: options: Simplify function setenv_foreign_option

[flichtenheld (C. Review) <ge...@op...>]

Attention is currently required from: cron2, plaisthos.

flichtenheld has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/1112?usp=email )

Change subject: options: Simplify function setenv_foreign_option
......................................................................


Patch Set 2:

(1 comment)

Patchset:

PS1: 
> I'm not convinced we couldn't do something more radical, like, getting rid of the loop and `first` a […]
Done



-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1112?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: I7efc77f63734501dfa8a8f5bed17b1a1b4e9e201
Gerrit-Change-Number: 1112
Gerrit-PatchSet: 2
Gerrit-Owner: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: cron2 <ge...@gr...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: cron2 <ge...@gr...>
Gerrit-Comment-Date: Mon, 28 Jul 2025 12:29:20 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: cron2 <ge...@gr...>
Gerrit-MessageType: comment

[Openvpn-devel] [M] Change in openvpn[master]: options: Simplify function setenv_foreign_option

[cron2 (C. Review) <ge...@op...>]

Attention is currently required from: flichtenheld, plaisthos.

cron2 has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/1112?usp=email )

Change subject: options: Simplify function setenv_foreign_option
......................................................................


Patch Set 2: Code-Review+2


-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1112?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: I7efc77f63734501dfa8a8f5bed17b1a1b4e9e201
Gerrit-Change-Number: 1112
Gerrit-PatchSet: 2
Gerrit-Owner: flichtenheld <fr...@li...>
Gerrit-Reviewer: cron2 <ge...@gr...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-Comment-Date: Mon, 28 Jul 2025 12:55:55 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[Openvpn-devel] [PATCH v2] options: Simplify function setenv_foreign_option

[Gert D. <ge...@gr...>]

From: Frank Lichtenheld <fr...@li...>

This was relatively complex for the actual usage.
Looked at the code because of -Wconversion warnings
related to the len argument. So this should also be
gone.

Change-Id: I7efc77f63734501dfa8a8f5bed17b1a1b4e9e201
Signed-off-by: Frank Lichtenheld <fr...@li...>
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/+/1112
This mail reflects revision 2 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 70b5799..8757581 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -1067,41 +1067,32 @@
 
 #ifndef _WIN32
 static void
-setenv_foreign_option(struct options *o, const char *argv[], int len, struct env_set *es)
+setenv_foreign_option(struct options *o, const char *option, const char *value, struct env_set *es)
 {
-    if (len > 0)
-    {
-        struct gc_arena gc = gc_new();
-        struct buffer name = alloc_buf_gc(OPTION_PARM_SIZE, &gc);
-        struct buffer value = alloc_buf_gc(OPTION_PARM_SIZE, &gc);
-        int i;
-        bool first = true;
-        bool good = true;
+    struct gc_arena gc = gc_new();
+    struct buffer env_name = alloc_buf_gc(OPTION_PARM_SIZE, &gc);
+    struct buffer env_value = alloc_buf_gc(OPTION_PARM_SIZE, &gc);
+    bool good = true;
 
-        good &= buf_printf(&name, "foreign_option_%d", o->foreign_option_index + 1);
-        ++o->foreign_option_index;
-        for (i = 0; i < len; ++i)
-        {
-            if (argv[i])
-            {
-                if (!first)
-                {
-                    good &= buf_printf(&value, " ");
-                }
-                good &= buf_printf(&value, "%s", argv[i]);
-                first = false;
-            }
-        }
-        if (good)
-        {
-            setenv_str(es, BSTR(&name), BSTR(&value));
-        }
-        else
-        {
-            msg(M_WARN, "foreign_option: name/value overflow");
-        }
-        gc_free(&gc);
+    good &= buf_printf(&env_name, "foreign_option_%d", o->foreign_option_index + 1);
+    if (value)
+    {
+        good &= buf_printf(&env_value, "dhcp-option %s %s", option, value);
     }
+    else
+    {
+        good &= buf_printf(&env_value, "dhcp-option %s", option);
+    }
+    if (good)
+    {
+        setenv_str(es, BSTR(&env_name), BSTR(&env_value));
+        ++o->foreign_option_index;
+    }
+    else
+    {
+        msg(M_WARN, "foreign_option: name/value overflow");
+    }
+    gc_free(&gc);
 }
 #endif /* ifndef _WIN32 */
 
@@ -3678,15 +3669,10 @@
     else if (o->up_script && !dns_updown_user_set(dns) && !dns_updown_forced(dns))
     {
         /* Set foreign option env vars from --dns config */
-        const char *p[] = { "dhcp-option", NULL, NULL };
-        size_t p_len = sizeof(p) / sizeof(p[0]);
-
-        p[1] = "DOMAIN";
         const struct dns_domain *d = dns->search_domains;
         while (d)
         {
-            p[2] = d->name;
-            setenv_foreign_option(o, (const char **)p, p_len, es);
+            setenv_foreign_option(o, "DOMAIN", d->name, es);
             d = d->next;
         }
 
@@ -3713,17 +3699,19 @@
             {
                 for (int i = 0; i < s->addr_count; ++i)
                 {
+                    const char *option;
+                    const char *value;
                     if (s->addr[i].family == AF_INET)
                     {
-                        p[1] = "DNS";
-                        p[2] = print_in_addr_t(s->addr[i].in.a4.s_addr, IA_NET_ORDER, &gc);
+                        option = "DNS";
+                        value = print_in_addr_t(s->addr[i].in.a4.s_addr, IA_NET_ORDER, &gc);
                     }
                     else
                     {
-                        p[1] = "DNS6";
-                        p[2] = print_in6_addr(s->addr[i].in.a6, 0, &gc);
+                        option = "DNS6";
+                        value = print_in6_addr(s->addr[i].in.a6, 0, &gc);
                     }
-                    setenv_foreign_option(o, (const char **)p, p_len, es);
+                    setenv_foreign_option(o, option, value, es);
                 }
                 break;
             }
@@ -8388,7 +8376,7 @@
             goto err;
         }
 #else /* if defined(_WIN32) || defined(TARGET_ANDROID) */
-        setenv_foreign_option(options, (const char **)p, 3, es);
+        setenv_foreign_option(options, p[1], p[2], es);
 #endif /* if defined(_WIN32) || defined(TARGET_ANDROID) */
     }
 #ifdef _WIN32
@@ -8530,7 +8518,7 @@
     else if (streq(p[0], "dhcp-option") && p[1] && !p[3])
     {
         VERIFY_PERMISSION(OPT_P_DHCPDNS);
-        setenv_foreign_option(options, (const char **)p, 3, es);
+        setenv_foreign_option(options, p[1], p[2], es);
     }
     else if (streq(p[0], "route-method") && p[1] && !p[2])
     {

[Openvpn-devel] [PATCH applied] Re: options: Simplify function setenv_foreign_option

[Gert D. <ge...@gr...>]

Stared at code (simple is good ;-) ).  Tested with manually connecting
to a server that pushes --dns and --dhcp-option <things> (DNS and others),
and verifying the output before/after.  Same.

Your patch has been applied to the master branch.

commit 654671a10b8f26ed0041ce434016fecb11438aae
Author: Frank Lichtenheld
Date:   Mon Jul 28 14:56:41 2025 +0200

     options: Simplify function setenv_foreign_option

     Signed-off-by: Frank Lichtenheld <fr...@li...>
     Acked-by: Gert Doering <ge...@gr...>
     Message-Id: <202...@gr...>
     URL: https://www.mail-archive.com/ope...@li.../msg32396.html
     Signed-off-by: Gert Doering <ge...@gr...>


--
kind regards,

Gert Doering

[Openvpn-devel] [M] Change in openvpn[master]: options: Simplify function setenv_foreign_option

[cron2 (C. Review) <ge...@op...>]

cron2 has uploaded a new patch set (#4) to the change originally created by flichtenheld. ( http://gerrit.openvpn.net/c/openvpn/+/1112?usp=email )

The following approvals got outdated and were removed:
Code-Review+2 by cron2


Change subject: options: Simplify function setenv_foreign_option
......................................................................

options: Simplify function setenv_foreign_option

This was relatively complex for the actual usage.
Looked at the code because of -Wconversion warnings
related to the len argument. So this should also be
gone.

Change-Id: I7efc77f63734501dfa8a8f5bed17b1a1b4e9e201
Signed-off-by: Frank Lichtenheld <fr...@li...>
Acked-by: Gert Doering <ge...@gr...>
Message-Id: <202...@gr...>
URL: https://www.mail-archive.com/ope...@li.../msg32396.html
Signed-off-by: Gert Doering <ge...@gr...>
---
M src/openvpn/options.c
1 file changed, 33 insertions(+), 45 deletions(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/12/1112/4

diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 0662b49..d44c102 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -1067,41 +1067,32 @@
 
 #ifndef _WIN32
 static void
-setenv_foreign_option(struct options *o, const char *argv[], int len, struct env_set *es)
+setenv_foreign_option(struct options *o, const char *option, const char *value, struct env_set *es)
 {
-    if (len > 0)
-    {
-        struct gc_arena gc = gc_new();
-        struct buffer name = alloc_buf_gc(OPTION_PARM_SIZE, &gc);
-        struct buffer value = alloc_buf_gc(OPTION_PARM_SIZE, &gc);
-        int i;
-        bool first = true;
-        bool good = true;
+    struct gc_arena gc = gc_new();
+    struct buffer env_name = alloc_buf_gc(OPTION_PARM_SIZE, &gc);
+    struct buffer env_value = alloc_buf_gc(OPTION_PARM_SIZE, &gc);
+    bool good = true;
 
-        good &= buf_printf(&name, "foreign_option_%d", o->foreign_option_index + 1);
-        ++o->foreign_option_index;
-        for (i = 0; i < len; ++i)
-        {
-            if (argv[i])
-            {
-                if (!first)
-                {
-                    good &= buf_printf(&value, " ");
-                }
-                good &= buf_printf(&value, "%s", argv[i]);
-                first = false;
-            }
-        }
-        if (good)
-        {
-            setenv_str(es, BSTR(&name), BSTR(&value));
-        }
-        else
-        {
-            msg(M_WARN, "foreign_option: name/value overflow");
-        }
-        gc_free(&gc);
+    good &= buf_printf(&env_name, "foreign_option_%d", o->foreign_option_index + 1);
+    if (value)
+    {
+        good &= buf_printf(&env_value, "dhcp-option %s %s", option, value);
     }
+    else
+    {
+        good &= buf_printf(&env_value, "dhcp-option %s", option);
+    }
+    if (good)
+    {
+        setenv_str(es, BSTR(&env_name), BSTR(&env_value));
+        ++o->foreign_option_index;
+    }
+    else
+    {
+        msg(M_WARN, "foreign_option: name/value overflow");
+    }
+    gc_free(&gc);
 }
 #endif /* ifndef _WIN32 */
 
@@ -3678,15 +3669,10 @@
     else if (o->up_script && !dns_updown_user_set(dns) && !dns_updown_forced(dns))
     {
         /* Set foreign option env vars from --dns config */
-        const char *p[] = { "dhcp-option", NULL, NULL };
-        size_t p_len = sizeof(p) / sizeof(p[0]);
-
-        p[1] = "DOMAIN";
         const struct dns_domain *d = dns->search_domains;
         while (d)
         {
-            p[2] = d->name;
-            setenv_foreign_option(o, (const char **)p, p_len, es);
+            setenv_foreign_option(o, "DOMAIN", d->name, es);
             d = d->next;
         }
 
@@ -3713,17 +3699,19 @@
             {
                 for (int i = 0; i < s->addr_count; ++i)
                 {
+                    const char *option;
+                    const char *value;
                     if (s->addr[i].family == AF_INET)
                     {
-                        p[1] = "DNS";
-                        p[2] = print_in_addr_t(s->addr[i].in.a4.s_addr, IA_NET_ORDER, &gc);
+                        option = "DNS";
+                        value = print_in_addr_t(s->addr[i].in.a4.s_addr, IA_NET_ORDER, &gc);
                     }
                     else
                     {
-                        p[1] = "DNS6";
-                        p[2] = print_in6_addr(s->addr[i].in.a6, 0, &gc);
+                        option = "DNS6";
+                        value = print_in6_addr(s->addr[i].in.a6, 0, &gc);
                     }
-                    setenv_foreign_option(o, (const char **)p, p_len, es);
+                    setenv_foreign_option(o, option, value, es);
                 }
                 break;
             }
@@ -8388,7 +8376,7 @@
             goto err;
         }
 #else /* if defined(_WIN32) || defined(TARGET_ANDROID) */
-        setenv_foreign_option(options, (const char **)p, 3, es);
+        setenv_foreign_option(options, p[1], p[2], es);
 #endif /* if defined(_WIN32) || defined(TARGET_ANDROID) */
     }
 #ifdef _WIN32
@@ -8530,7 +8518,7 @@
     else if (streq(p[0], "dhcp-option") && p[1] && !p[3])
     {
         VERIFY_PERMISSION(OPT_P_DHCPDNS);
-        setenv_foreign_option(options, (const char **)p, 3, es);
+        setenv_foreign_option(options, p[1], p[2], es);
     }
     else if (streq(p[0], "route-method") && p[1] && !p[2])
     {

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1112?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: I7efc77f63734501dfa8a8f5bed17b1a1b4e9e201
Gerrit-Change-Number: 1112
Gerrit-PatchSet: 4
Gerrit-Owner: flichtenheld <fr...@li...>
Gerrit-Reviewer: cron2 <ge...@gr...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-MessageType: newpatchset

[Openvpn-devel] [M] Change in openvpn[master]: options: Simplify function setenv_foreign_option

[cron2 (C. Review) <ge...@op...>]

cron2 has submitted this change. ( http://gerrit.openvpn.net/c/openvpn/+/1112?usp=email )

Change subject: options: Simplify function setenv_foreign_option
......................................................................

options: Simplify function setenv_foreign_option

This was relatively complex for the actual usage.
Looked at the code because of -Wconversion warnings
related to the len argument. So this should also be
gone.

Change-Id: I7efc77f63734501dfa8a8f5bed17b1a1b4e9e201
Signed-off-by: Frank Lichtenheld <fr...@li...>
Acked-by: Gert Doering <ge...@gr...>
Message-Id: <202...@gr...>
URL: https://www.mail-archive.com/ope...@li.../msg32396.html
Signed-off-by: Gert Doering <ge...@gr...>
---
M src/openvpn/options.c
1 file changed, 33 insertions(+), 45 deletions(-)




diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 0662b49..d44c102 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -1067,41 +1067,32 @@
 
 #ifndef _WIN32
 static void
-setenv_foreign_option(struct options *o, const char *argv[], int len, struct env_set *es)
+setenv_foreign_option(struct options *o, const char *option, const char *value, struct env_set *es)
 {
-    if (len > 0)
-    {
-        struct gc_arena gc = gc_new();
-        struct buffer name = alloc_buf_gc(OPTION_PARM_SIZE, &gc);
-        struct buffer value = alloc_buf_gc(OPTION_PARM_SIZE, &gc);
-        int i;
-        bool first = true;
-        bool good = true;
+    struct gc_arena gc = gc_new();
+    struct buffer env_name = alloc_buf_gc(OPTION_PARM_SIZE, &gc);
+    struct buffer env_value = alloc_buf_gc(OPTION_PARM_SIZE, &gc);
+    bool good = true;
 
-        good &= buf_printf(&name, "foreign_option_%d", o->foreign_option_index + 1);
-        ++o->foreign_option_index;
-        for (i = 0; i < len; ++i)
-        {
-            if (argv[i])
-            {
-                if (!first)
-                {
-                    good &= buf_printf(&value, " ");
-                }
-                good &= buf_printf(&value, "%s", argv[i]);
-                first = false;
-            }
-        }
-        if (good)
-        {
-            setenv_str(es, BSTR(&name), BSTR(&value));
-        }
-        else
-        {
-            msg(M_WARN, "foreign_option: name/value overflow");
-        }
-        gc_free(&gc);
+    good &= buf_printf(&env_name, "foreign_option_%d", o->foreign_option_index + 1);
+    if (value)
+    {
+        good &= buf_printf(&env_value, "dhcp-option %s %s", option, value);
     }
+    else
+    {
+        good &= buf_printf(&env_value, "dhcp-option %s", option);
+    }
+    if (good)
+    {
+        setenv_str(es, BSTR(&env_name), BSTR(&env_value));
+        ++o->foreign_option_index;
+    }
+    else
+    {
+        msg(M_WARN, "foreign_option: name/value overflow");
+    }
+    gc_free(&gc);
 }
 #endif /* ifndef _WIN32 */
 
@@ -3678,15 +3669,10 @@
     else if (o->up_script && !dns_updown_user_set(dns) && !dns_updown_forced(dns))
     {
         /* Set foreign option env vars from --dns config */
-        const char *p[] = { "dhcp-option", NULL, NULL };
-        size_t p_len = sizeof(p) / sizeof(p[0]);
-
-        p[1] = "DOMAIN";
         const struct dns_domain *d = dns->search_domains;
         while (d)
         {
-            p[2] = d->name;
-            setenv_foreign_option(o, (const char **)p, p_len, es);
+            setenv_foreign_option(o, "DOMAIN", d->name, es);
             d = d->next;
         }
 
@@ -3713,17 +3699,19 @@
             {
                 for (int i = 0; i < s->addr_count; ++i)
                 {
+                    const char *option;
+                    const char *value;
                     if (s->addr[i].family == AF_INET)
                     {
-                        p[1] = "DNS";
-                        p[2] = print_in_addr_t(s->addr[i].in.a4.s_addr, IA_NET_ORDER, &gc);
+                        option = "DNS";
+                        value = print_in_addr_t(s->addr[i].in.a4.s_addr, IA_NET_ORDER, &gc);
                     }
                     else
                     {
-                        p[1] = "DNS6";
-                        p[2] = print_in6_addr(s->addr[i].in.a6, 0, &gc);
+                        option = "DNS6";
+                        value = print_in6_addr(s->addr[i].in.a6, 0, &gc);
                     }
-                    setenv_foreign_option(o, (const char **)p, p_len, es);
+                    setenv_foreign_option(o, option, value, es);
                 }
                 break;
             }
@@ -8388,7 +8376,7 @@
             goto err;
         }
 #else /* if defined(_WIN32) || defined(TARGET_ANDROID) */
-        setenv_foreign_option(options, (const char **)p, 3, es);
+        setenv_foreign_option(options, p[1], p[2], es);
 #endif /* if defined(_WIN32) || defined(TARGET_ANDROID) */
     }
 #ifdef _WIN32
@@ -8530,7 +8518,7 @@
     else if (streq(p[0], "dhcp-option") && p[1] && !p[3])
     {
         VERIFY_PERMISSION(OPT_P_DHCPDNS);
-        setenv_foreign_option(options, (const char **)p, 3, es);
+        setenv_foreign_option(options, p[1], p[2], es);
     }
     else if (streq(p[0], "route-method") && p[1] && !p[2])
     {

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1112?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: I7efc77f63734501dfa8a8f5bed17b1a1b4e9e201
Gerrit-Change-Number: 1112
Gerrit-PatchSet: 4
Gerrit-Owner: flichtenheld <fr...@li...>
Gerrit-Reviewer: cron2 <ge...@gr...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-MessageType: merged