Re: [Openvpn-devel] [PATCH 2/4] argv: do fewer memory re-allocations

[Arne Schwabe]

Am 06.02.20 um 14:21 schrieb David Sommerseth:
> From: Heiko Hund 
> 
> Prevent the re-allocations of memory when the internal argv grows
> beyond 2 and 4 arguments by initially allocating argv to hold up to
> 7 (+ trailing NULL) pointers.
> 
> While at it rename argv_reset to argv_free to actually express
> what's going on. Redo the argv_reset functionality so that it can
> be used to actually reset the argv without re-allocation.
> 
> Signed-off-by: Heiko Hund 
> Signed-off-by: David Sommerseth 
> ---

Acked-By: Arne Schwabe 





signature.asc
Description: OpenPGP digital signature
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [PATCH 2/4] argv: do fewer memory re-allocations

[David Sommerseth]

From: Heiko Hund 

Prevent the re-allocations of memory when the internal argv grows
beyond 2 and 4 arguments by initially allocating argv to hold up to
7 (+ trailing NULL) pointers.

While at it rename argv_reset to argv_free to actually express
what's going on. Redo the argv_reset functionality so that it can
be used to actually reset the argv without re-allocation.

Signed-off-by: Heiko Hund 
Signed-off-by: David Sommerseth 
---
 src/openvpn/argv.c   | 81 ++--
 src/openvpn/argv.h   |  2 +-
 src/openvpn/console_systemd.c|  2 +-
 src/openvpn/init.c   | 15 ++
 src/openvpn/lladdr.c |  2 +-
 src/openvpn/multi.c  | 10 ++--
 src/openvpn/networking_iproute2.c| 23 
 src/openvpn/options.c|  2 +-
 src/openvpn/plugin.c |  2 +-
 src/openvpn/route.c  |  8 +--
 src/openvpn/socket.c |  4 +-
 src/openvpn/ssl_verify.c |  6 +--
 src/openvpn/tls_crypt.c  |  2 +-
 src/openvpn/tun.c| 38 ++---
 tests/unit_tests/openvpn/test_argv.c | 43 ++-
 15 files changed, 124 insertions(+), 116 deletions(-)

diff --git a/src/openvpn/argv.c b/src/openvpn/argv.c
index fcf61ec5..4f7aa4e5 100644
--- a/src/openvpn/argv.c
+++ b/src/openvpn/argv.c
@@ -40,34 +40,6 @@
 #include "env_set.h"
 #include "options.h"
 
-static void
-argv_init(struct argv *a)
-{
-a->capacity = 0;
-a->argc = 0;
-a->argv = NULL;
-}
-
-struct argv
-argv_new(void)
-{
-struct argv ret;
-argv_init();
-return ret;
-}
-
-void
-argv_reset(struct argv *a)
-{
-size_t i;
-for (i = 0; i < a->argc; ++i)
-{
-free(a->argv[i]);
-}
-free(a->argv);
-argv_init(a);
-}
-
 static void
 argv_extend(struct argv *a, const size_t newcap)
 {
@@ -86,6 +58,46 @@ argv_extend(struct argv *a, const size_t newcap)
 }
 }
 
+static void
+argv_init(struct argv *a)
+{
+a->capacity = 0;
+a->argc = 0;
+a->argv = NULL;
+argv_extend(a, 8);
+}
+
+struct argv
+argv_new(void)
+{
+struct argv ret;
+argv_init();
+return ret;
+}
+
+void
+argv_free(struct argv *a)
+{
+size_t i;
+for (i = 0; i < a->argc; ++i)
+{
+free(a->argv[i]);
+}
+free(a->argv);
+}
+
+static void
+argv_reset(struct argv *a)
+{
+size_t i;
+for (i = 0; i < a->argc; ++i)
+{
+free(a->argv[i]);
+a->argv[i] = NULL;
+}
+a->argc = 0;
+}
+
 static void
 argv_grow(struct argv *a, const size_t add)
 {
@@ -133,14 +145,7 @@ argv_insert_head(const struct argv *a, const char *head)
 const char *
 argv_str(const struct argv *a, struct gc_arena *gc, const unsigned int flags)
 {
-if (a->argv)
-{
-return print_argv((const char **)a->argv, gc, flags);
-}
-else
-{
-return "";
-}
+return print_argv((const char **)a->argv, gc, flags);
 }
 
 void
@@ -221,8 +226,6 @@ argv_printf_arglist(struct argv *a, const char *format, 
va_list arglist)
 const char delim = 0x1D;  /* ASCII Group Separator (GS) */
 bool res = false;
 
-argv_extend(a, 1); /* ensure trailing NULL */
-
 /*
  * Prepare a format string which will be used by vsnprintf() later on.
  *
@@ -279,7 +282,6 @@ argv_printf_arglist(struct argv *a, const char *format, 
va_list arglist)
 {
 /* Someone snuck in a GS (0x1D), fail gracefully */
 argv_reset(a);
-argv_extend(a, 1); /* ensure trailing NULL */
 goto out;
 }
 res = true;
@@ -318,7 +320,6 @@ void
 argv_parse_cmd(struct argv *a, const char *s)
 {
 argv_reset(a);
-argv_extend(a, 1); /* ensure trailing NULL */
 
 struct gc_arena gc = gc_new();
 char *parms[MAX_PARMS + 1] = { 0 };
diff --git a/src/openvpn/argv.h b/src/openvpn/argv.h
index b9105a43..989cd297 100644
--- a/src/openvpn/argv.h
+++ b/src/openvpn/argv.h
@@ -40,7 +40,7 @@ struct argv {
 
 struct argv argv_new(void);
 
-void argv_reset(struct argv *a);
+void argv_free(struct argv *a);
 
 const char *argv_str(const struct argv *a, struct gc_arena *gc, const unsigned 
int flags);
 
diff --git a/src/openvpn/console_systemd.c b/src/openvpn/console_systemd.c
index 8d9e825b..c7cf1ada 100644
--- a/src/openvpn/console_systemd.c
+++ b/src/openvpn/console_systemd.c
@@ -85,7 +85,7 @@ get_console_input_systemd(const char *prompt, const bool 
echo, char *input, cons
 }
 close(std_out);
 
-argv_reset();
+argv_free();
 
 return ret;
 }
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 04207b61..db7d1216 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -164,7 +164,7 @@ run_up_down(const char *command,
 msg(M_FATAL, "ERROR: up/down plugin call failed");
 }
 
-argv_reset();
+argv_free();
 }
 
 if (command)
@@ -177,7 +177,7 @@ run_up_down(const char *command,
 ifconfig_local, ifconfig_remote, 

[Openvpn-devel] [PATCH 2/4] argv: do fewer memory re-allocations

[David Sommerseth]

From: Heiko Hund 

Prevent the re-allocations of memory when the internal argv grows
beyond 2 and 4 arguments by initially allocating argv to hold up to
7 (+ trailing NULL) pointers.

While at it rename argv_reset to argv_free to actually express
what's going on. Redo the argv_reset functionality so that it can
be used to actually reset the argv without re-allocation.

Signed-off-by: Heiko Hund 
Signed-off-by: David Sommerseth 
---
 src/openvpn/argv.c   | 81 ++--
 src/openvpn/argv.h   |  2 +-
 src/openvpn/console_systemd.c|  2 +-
 src/openvpn/init.c   | 15 ++
 src/openvpn/lladdr.c |  2 +-
 src/openvpn/multi.c  | 10 ++--
 src/openvpn/networking_iproute2.c| 23 
 src/openvpn/options.c|  2 +-
 src/openvpn/plugin.c |  2 +-
 src/openvpn/route.c  |  8 +--
 src/openvpn/socket.c |  4 +-
 src/openvpn/ssl_verify.c |  6 +--
 src/openvpn/tls_crypt.c  |  2 +-
 src/openvpn/tun.c| 38 ++---
 tests/unit_tests/openvpn/test_argv.c | 43 ++-
 15 files changed, 124 insertions(+), 116 deletions(-)

diff --git a/src/openvpn/argv.c b/src/openvpn/argv.c
index fcf61ec5..4f7aa4e5 100644
--- a/src/openvpn/argv.c
+++ b/src/openvpn/argv.c
@@ -40,34 +40,6 @@
 #include "env_set.h"
 #include "options.h"
 
-static void
-argv_init(struct argv *a)
-{
-a->capacity = 0;
-a->argc = 0;
-a->argv = NULL;
-}
-
-struct argv
-argv_new(void)
-{
-struct argv ret;
-argv_init();
-return ret;
-}
-
-void
-argv_reset(struct argv *a)
-{
-size_t i;
-for (i = 0; i < a->argc; ++i)
-{
-free(a->argv[i]);
-}
-free(a->argv);
-argv_init(a);
-}
-
 static void
 argv_extend(struct argv *a, const size_t newcap)
 {
@@ -86,6 +58,46 @@ argv_extend(struct argv *a, const size_t newcap)
 }
 }
 
+static void
+argv_init(struct argv *a)
+{
+a->capacity = 0;
+a->argc = 0;
+a->argv = NULL;
+argv_extend(a, 8);
+}
+
+struct argv
+argv_new(void)
+{
+struct argv ret;
+argv_init();
+return ret;
+}
+
+void
+argv_free(struct argv *a)
+{
+size_t i;
+for (i = 0; i < a->argc; ++i)
+{
+free(a->argv[i]);
+}
+free(a->argv);
+}
+
+static void
+argv_reset(struct argv *a)
+{
+size_t i;
+for (i = 0; i < a->argc; ++i)
+{
+free(a->argv[i]);
+a->argv[i] = NULL;
+}
+a->argc = 0;
+}
+
 static void
 argv_grow(struct argv *a, const size_t add)
 {
@@ -133,14 +145,7 @@ argv_insert_head(const struct argv *a, const char *head)
 const char *
 argv_str(const struct argv *a, struct gc_arena *gc, const unsigned int flags)
 {
-if (a->argv)
-{
-return print_argv((const char **)a->argv, gc, flags);
-}
-else
-{
-return "";
-}
+return print_argv((const char **)a->argv, gc, flags);
 }
 
 void
@@ -221,8 +226,6 @@ argv_printf_arglist(struct argv *a, const char *format, 
va_list arglist)
 const char delim = 0x1D;  /* ASCII Group Separator (GS) */
 bool res = false;
 
-argv_extend(a, 1); /* ensure trailing NULL */
-
 /*
  * Prepare a format string which will be used by vsnprintf() later on.
  *
@@ -279,7 +282,6 @@ argv_printf_arglist(struct argv *a, const char *format, 
va_list arglist)
 {
 /* Someone snuck in a GS (0x1D), fail gracefully */
 argv_reset(a);
-argv_extend(a, 1); /* ensure trailing NULL */
 goto out;
 }
 res = true;
@@ -318,7 +320,6 @@ void
 argv_parse_cmd(struct argv *a, const char *s)
 {
 argv_reset(a);
-argv_extend(a, 1); /* ensure trailing NULL */
 
 struct gc_arena gc = gc_new();
 char *parms[MAX_PARMS + 1] = { 0 };
diff --git a/src/openvpn/argv.h b/src/openvpn/argv.h
index b9105a43..989cd297 100644
--- a/src/openvpn/argv.h
+++ b/src/openvpn/argv.h
@@ -40,7 +40,7 @@ struct argv {
 
 struct argv argv_new(void);
 
-void argv_reset(struct argv *a);
+void argv_free(struct argv *a);
 
 const char *argv_str(const struct argv *a, struct gc_arena *gc, const unsigned 
int flags);
 
diff --git a/src/openvpn/console_systemd.c b/src/openvpn/console_systemd.c
index 8d9e825b..c7cf1ada 100644
--- a/src/openvpn/console_systemd.c
+++ b/src/openvpn/console_systemd.c
@@ -85,7 +85,7 @@ get_console_input_systemd(const char *prompt, const bool 
echo, char *input, cons
 }
 close(std_out);
 
-argv_reset();
+argv_free();
 
 return ret;
 }
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index ae7bd639..8846854f 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -164,7 +164,7 @@ run_up_down(const char *command,
 msg(M_FATAL, "ERROR: up/down plugin call failed");
 }
 
-argv_reset();
+argv_free();
 }
 
 if (command)
@@ -177,7 +177,7 @@ run_up_down(const char *command,
 ifconfig_local, ifconfig_remote, 

Re: [Openvpn-devel] [PATCH 2/4] argv: do fewer memory re-allocations

[David Sommerseth]

On 19/10/18 17:56, David Sommerseth wrote:
> From: Heiko Hund 
> 
> Prevent the re-allocations of memory when the internal argv grows
> beyond 2 and 4 arguments by initially allocating argv to hold up to
> 7 (+ trailing NULL) pointers.
> 
> While at it rename argv_reset to argv_free to actually express
> what's going on. Redo the argv_reset functionality so that it can
> be used to actually reset the argv without re-allocation.
> 
> Signed-off-by: Heiko Hund 
> ---
>  src/openvpn/argv.c   | 81 ++--
>  src/openvpn/argv.h   |  2 +-
>  src/openvpn/console_systemd.c|  2 +-
>  src/openvpn/init.c   | 15 ++
>  src/openvpn/lladdr.c |  2 +-
>  src/openvpn/multi.c  | 10 ++--
>  src/openvpn/options.c|  2 +-
>  src/openvpn/plugin.c |  2 +-
>  src/openvpn/route.c  |  8 +--
>  src/openvpn/socket.c |  4 +-
>  src/openvpn/ssl_verify.c |  6 +--
>  src/openvpn/tun.c| 38 ++---
>  tests/unit_tests/openvpn/test_argv.c | 41 +-
>  13 files changed, 110 insertions(+), 103 deletions(-)

This is a pure rebase of [0, 1].  In my initial review, I questioned the need
to keep argv_str().  I decided to keep the argv_str() as it makes the usage of
struct argv based processing simpler; no need for the caller to know inner
details of struct argv.

[0] 
[1] <20171112082736.15578-1-heiko.hund@sophos.com">http://mid.mail-archive.com/20171112082736.15578-1-heiko.hund@sophos.com>


-- 
kind regards,

David Sommerseth
OpenVPN Inc




signature.asc
Description: OpenPGP digital signature
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [PATCH 2/4] argv: do fewer memory re-allocations

[David Sommerseth]

From: Heiko Hund 

Prevent the re-allocations of memory when the internal argv grows
beyond 2 and 4 arguments by initially allocating argv to hold up to
7 (+ trailing NULL) pointers.

While at it rename argv_reset to argv_free to actually express
what's going on. Redo the argv_reset functionality so that it can
be used to actually reset the argv without re-allocation.

Signed-off-by: Heiko Hund 
---
 src/openvpn/argv.c   | 81 ++--
 src/openvpn/argv.h   |  2 +-
 src/openvpn/console_systemd.c|  2 +-
 src/openvpn/init.c   | 15 ++
 src/openvpn/lladdr.c |  2 +-
 src/openvpn/multi.c  | 10 ++--
 src/openvpn/options.c|  2 +-
 src/openvpn/plugin.c |  2 +-
 src/openvpn/route.c  |  8 +--
 src/openvpn/socket.c |  4 +-
 src/openvpn/ssl_verify.c |  6 +--
 src/openvpn/tun.c| 38 ++---
 tests/unit_tests/openvpn/test_argv.c | 41 +-
 13 files changed, 110 insertions(+), 103 deletions(-)

diff --git a/src/openvpn/argv.c b/src/openvpn/argv.c
index 9606f382..ff6d0b46 100644
--- a/src/openvpn/argv.c
+++ b/src/openvpn/argv.c
@@ -40,34 +40,6 @@
 #include "env_set.h"
 #include "options.h"
 
-static void
-argv_init(struct argv *a)
-{
-a->capacity = 0;
-a->argc = 0;
-a->argv = NULL;
-}
-
-struct argv
-argv_new(void)
-{
-struct argv ret;
-argv_init();
-return ret;
-}
-
-void
-argv_reset(struct argv *a)
-{
-size_t i;
-for (i = 0; i < a->argc; ++i)
-{
-free(a->argv[i]);
-}
-free(a->argv);
-argv_init(a);
-}
-
 static void
 argv_extend(struct argv *a, const size_t newcap)
 {
@@ -86,6 +58,46 @@ argv_extend(struct argv *a, const size_t newcap)
 }
 }
 
+static void
+argv_init(struct argv *a)
+{
+a->capacity = 0;
+a->argc = 0;
+a->argv = NULL;
+argv_extend(a, 8);
+}
+
+struct argv
+argv_new(void)
+{
+struct argv ret;
+argv_init();
+return ret;
+}
+
+void
+argv_free(struct argv *a)
+{
+size_t i;
+for (i = 0; i < a->argc; ++i)
+{
+free(a->argv[i]);
+}
+free(a->argv);
+}
+
+static void
+argv_reset(struct argv *a)
+{
+size_t i;
+for (i = 0; i < a->argc; ++i)
+{
+free(a->argv[i]);
+a->argv[i] = NULL;
+}
+a->argc = 0;
+}
+
 static void
 argv_grow(struct argv *a, const size_t add)
 {
@@ -134,14 +146,7 @@ argv_insert_head(const struct argv *a, const char *head)
 const char *
 argv_str(const struct argv *a, struct gc_arena *gc, const unsigned int flags)
 {
-if (a->argv)
-{
-return print_argv((const char **)a->argv, gc, flags);
-}
-else
-{
-return "";
-}
+return print_argv((const char **)a->argv, gc, flags);
 }
 
 void
@@ -229,8 +234,6 @@ argv_printf_arglist(struct argv *a, const char *format, 
va_list arglist)
 va_list tmplist;
 int len;
 
-argv_extend(a, 1); /* ensure trailing NULL */
-
 argc = a->argc;
 f = argv_prep_format(format, delim, , );
 if (f == NULL)
@@ -270,7 +273,6 @@ argv_printf_arglist(struct argv *a, const char *format, 
va_list arglist)
 {
 /* Someone snuck in a GS (0x1D), fail gracefully */
 argv_reset(a);
-argv_extend(a, 1); /* ensure trailing NULL */
 goto out;
 }
 
@@ -315,7 +317,6 @@ argv_parse_cmd(struct argv *a, const char *s)
 struct gc_arena gc = gc_new();
 
 argv_reset(a);
-argv_extend(a, 1); /* ensure trailing NULL */
 
 nparms = parse_line(s, parms, MAX_PARMS, "SCRIPT-ARGV", 0, 
D_ARGV_PARSE_CMD, );
 if (nparms)
diff --git a/src/openvpn/argv.h b/src/openvpn/argv.h
index b9105a43..989cd297 100644
--- a/src/openvpn/argv.h
+++ b/src/openvpn/argv.h
@@ -40,7 +40,7 @@ struct argv {
 
 struct argv argv_new(void);
 
-void argv_reset(struct argv *a);
+void argv_free(struct argv *a);
 
 const char *argv_str(const struct argv *a, struct gc_arena *gc, const unsigned 
int flags);
 
diff --git a/src/openvpn/console_systemd.c b/src/openvpn/console_systemd.c
index 8d9e825b..c7cf1ada 100644
--- a/src/openvpn/console_systemd.c
+++ b/src/openvpn/console_systemd.c
@@ -85,7 +85,7 @@ get_console_input_systemd(const char *prompt, const bool 
echo, char *input, cons
 }
 close(std_out);
 
-argv_reset();
+argv_free();
 
 return ret;
 }
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index e5e6e85f..7dab4f21 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -163,7 +163,7 @@ run_up_down(const char *command,
 msg(M_FATAL, "ERROR: up/down plugin call failed");
 }
 
-argv_reset();
+argv_free();
 }
 
 if (command)
@@ -176,7 +176,7 @@ run_up_down(const char *command,
 ifconfig_local, ifconfig_remote, context);
 argv_msg(M_INFO, );
 openvpn_run_script(, es, S_FATAL, "--up/--down");
-argv_reset();
+argv_free();
 }