Re: [Openvpn-devel] [PATCH] Prevent memory drain for long lasting floating sessions

2014-12-08 Thread Steffan Karger 
ACK. Less memory usage (even without float), and got rid of a dynamic
allocation, nice! Code looks good and passes my local tests.

-Steffan

On 08-12-14 17:48, Lev Stipakov wrote:
> For every float event we generate prefix, which allocates 256 + 64
> bytes. That memory is reclaimed when client disconnects, so long lasting
> and constantly floating sessions drain memory.
> 
> As a fix use preallocated buffer inside multi_instance for storing
> multi_prefix.
> 
> Signed-off-by: Lev Stipakov 
> ---
>  src/openvpn/mudp.c  |  4 
>  src/openvpn/multi.c | 14 ++
>  src/openvpn/multi.h |  8 +---
>  3 files changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/src/openvpn/mudp.c b/src/openvpn/mudp.c
> index 853c08c..3e3f750 100644
> --- a/src/openvpn/mudp.c
> +++ b/src/openvpn/mudp.c
> @@ -111,6 +111,10 @@ multi_get_create_instance_udp (struct multi_context *m, 
> bool *floated)
> break;
>   }
>   }
> +
> +   /* should not really end up here, since 
> multi_create_instance returns null
> +* if amount of clients exceeds max_clients */
> +   ASSERT(i < m->max_clients);
>   }
>   }
> else
> diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
> index 538f4f1..fd0d0fe 100644
> --- a/src/openvpn/multi.c
> +++ b/src/openvpn/multi.c
> @@ -403,7 +403,7 @@ multi_instance_string (const struct multi_instance *mi, 
> bool null, struct gc_are
>  {
>if (mi)
>  {
> -  struct buffer out = alloc_buf_gc (256, gc);
> +  struct buffer out = alloc_buf_gc (MULTI_PREFIX_MAX_LENGTH, gc);
>const char *cn = tls_common_name (mi->context.c2.tls_multi, true);
>  
>if (cn)
> @@ -420,21 +420,27 @@ multi_instance_string (const struct multi_instance *mi, 
> bool null, struct gc_are
>  void
>  generate_prefix (struct multi_instance *mi)
>  {
> -  mi->msg_prefix = multi_instance_string (mi, true, >gc);
> +  struct gc_arena gc = gc_new();
> +  const char *prefix = multi_instance_string (mi, true, );
> +  if (prefix)
> +strncpynt(mi->msg_prefix, prefix, sizeof(mi->msg_prefix));
> +  else
> +mi->msg_prefix[0] = '\0';
>set_prefix (mi);
> +  gc_free();
>  }
>  
>  void
>  ungenerate_prefix (struct multi_instance *mi)
>  {
> -  mi->msg_prefix = NULL;
> +  mi->msg_prefix[0] = '\0';
>set_prefix (mi);
>  }
>  
>  static const char *
>  mi_prefix (const struct multi_instance *mi)
>  {
> -  if (mi && mi->msg_prefix)
> +  if (mi && mi->msg_prefix[0])
>  return mi->msg_prefix;
>else
>  return "UNDEF_I";
> diff --git a/src/openvpn/multi.h b/src/openvpn/multi.h
> index ad7f700..32b89d2 100644
> --- a/src/openvpn/multi.h
> +++ b/src/openvpn/multi.h
> @@ -42,6 +42,8 @@
>  #include "mtcp.h"
>  #include "perf.h"
>  
> +#define MULTI_PREFIX_MAX_LENGTH 256
> +
>  /*
>   * Walk (don't run) through the routing table,
>   * deleting old entries, and possibly multi_instance
> @@ -80,7 +82,7 @@ struct multi_instance {
>struct mroute_addr real;  /**< External network address of the
>   *   remote peer. */
>ifconfig_pool_handle vaddr_handle;
> -  const char *msg_prefix;
> +  char msg_prefix[MULTI_PREFIX_MAX_LENGTH];
>  
>/* queued outgoing data in Server/TCP mode */
>unsigned int tcp_rwflags;
> @@ -445,10 +447,10 @@ static inline void
>  set_prefix (struct multi_instance *mi)
>  {
>  #ifdef MULTI_DEBUG_EVENT_LOOP
> -  if (mi->msg_prefix)
> +  if (mi->msg_prefix[0])
>  printf ("[%s]\n", mi->msg_prefix);
>  #endif
> -  msg_set_prefix (mi->msg_prefix);
> +  msg_set_prefix (mi->msg_prefix[0] ? mi->msg_prefix : NULL);
>  }
>  
>  static inline void
>

[Openvpn-devel] [PATCH] Prevent memory drain for long lasting floating sessions

2014-12-08 Thread Lev Stipakov 
For every float event we generate prefix, which allocates 256 + 64
bytes. That memory is reclaimed when client disconnects, so long lasting
and constantly floating sessions drain memory.

As a fix use preallocated buffer inside multi_instance for storing
multi_prefix.

Signed-off-by: Lev Stipakov 
---
 src/openvpn/mudp.c  |  4 
 src/openvpn/multi.c | 14 ++
 src/openvpn/multi.h |  8 +---
 3 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/src/openvpn/mudp.c b/src/openvpn/mudp.c
index 853c08c..3e3f750 100644
--- a/src/openvpn/mudp.c
+++ b/src/openvpn/mudp.c
@@ -111,6 +111,10 @@ multi_get_create_instance_udp (struct multi_context *m, 
bool *floated)
  break;
}
}
+
+ /* should not really end up here, since 
multi_create_instance returns null
+  * if amount of clients exceeds max_clients */
+ ASSERT(i < m->max_clients);
}
}
  else
diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 538f4f1..fd0d0fe 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -403,7 +403,7 @@ multi_instance_string (const struct multi_instance *mi, 
bool null, struct gc_are
 {
   if (mi)
 {
-  struct buffer out = alloc_buf_gc (256, gc);
+  struct buffer out = alloc_buf_gc (MULTI_PREFIX_MAX_LENGTH, gc);
   const char *cn = tls_common_name (mi->context.c2.tls_multi, true);

   if (cn)
@@ -420,21 +420,27 @@ multi_instance_string (const struct multi_instance *mi, 
bool null, struct gc_are
 void
 generate_prefix (struct multi_instance *mi)
 {
-  mi->msg_prefix = multi_instance_string (mi, true, >gc);
+  struct gc_arena gc = gc_new();
+  const char *prefix = multi_instance_string (mi, true, );
+  if (prefix)
+strncpynt(mi->msg_prefix, prefix, sizeof(mi->msg_prefix));
+  else
+mi->msg_prefix[0] = '\0';
   set_prefix (mi);
+  gc_free();
 }

 void
 ungenerate_prefix (struct multi_instance *mi)
 {
-  mi->msg_prefix = NULL;
+  mi->msg_prefix[0] = '\0';
   set_prefix (mi);
 }

 static const char *
 mi_prefix (const struct multi_instance *mi)
 {
-  if (mi && mi->msg_prefix)
+  if (mi && mi->msg_prefix[0])
 return mi->msg_prefix;
   else
 return "UNDEF_I";
diff --git a/src/openvpn/multi.h b/src/openvpn/multi.h
index ad7f700..32b89d2 100644
--- a/src/openvpn/multi.h
+++ b/src/openvpn/multi.h
@@ -42,6 +42,8 @@
 #include "mtcp.h"
 #include "perf.h"

+#define MULTI_PREFIX_MAX_LENGTH 256
+
 /*
  * Walk (don't run) through the routing table,
  * deleting old entries, and possibly multi_instance
@@ -80,7 +82,7 @@ struct multi_instance {
   struct mroute_addr real;  /**< External network address of the
  *   remote peer. */
   ifconfig_pool_handle vaddr_handle;
-  const char *msg_prefix;
+  char msg_prefix[MULTI_PREFIX_MAX_LENGTH];

   /* queued outgoing data in Server/TCP mode */
   unsigned int tcp_rwflags;
@@ -445,10 +447,10 @@ static inline void
 set_prefix (struct multi_instance *mi)
 {
 #ifdef MULTI_DEBUG_EVENT_LOOP
-  if (mi->msg_prefix)
+  if (mi->msg_prefix[0])
 printf ("[%s]\n", mi->msg_prefix);
 #endif
-  msg_set_prefix (mi->msg_prefix);
+  msg_set_prefix (mi->msg_prefix[0] ? mi->msg_prefix : NULL);
 }

 static inline void
-- 
1.9.1