Re: [Openvpn-devel] [PATCH 3/6] networking: extend API for better memory management

2019-08-16 Thread Gert Doering
Hi,

On Mon, Aug 05, 2019 at 11:25:26AM +0200, Antonio Quartulli wrote:
> From: Antonio Quartulli 
> 
> Networking backend implementations may need to allocate dynamic
> resources that require an explicit free/release.
> Since these cleanup are perfomed not very often, and only at specific
> times, it makes sense to have the upper layer signal when it's the right
> time to do so, by means of a new API call.

While I generally like this patch ("0.9 ACKs"), there is one thing that
rubs me totally the wrong way and that must go:

> +char *addr_str = (char *)print_in_addr_t(*addr, 0, &ctx->gc);
> +char *brd_str = (char *)print_in_addr_t(*broadcast, 0, &ctx->gc);

These casts - print_in_addr_t() returns a "const char *", it gets 
assigned to a temporary variable of type "char *", and handed over to
a printf() variant.  Such code should never ever have a cast.

If the compiler warns about loss of const (in which case I would
ask the question why the return value of print_in_addr_t() is declared
const at all, if it's a gc_malloc()ed string which *could* be written
to just fine...), then let's make "addr_str" and "brd_str" a const
as well.

But no casts in for "basic types assigned to single-use variables", ever.

gert

-- 
"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
 Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany g...@greenie.muc.de


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


[Openvpn-devel] [PATCH 3/6] networking: extend API for better memory management

2019-08-05 Thread Antonio Quartulli
From: Antonio Quartulli 

Networking backend implementations may need to allocate dynamic
resources that require an explicit free/release.
Since these cleanup are perfomed not very often, and only at specific
times, it makes sense to have the upper layer signal when it's the right
time to do so, by means of a new API call.

For this purpose two news APIs have been implemented:
- net_ctx_free() to release all backend specific resources. Expected to
  be called at application cleanup time;
- net_ctx_reset() to let backends release temporary resources (i.e.
  reset garbage collectors). To be invoked after routines that
  are expected to allocate memory (i.e. tun setup or shutdown).

In this patch related implementations for iproute2 and sitnl are also
provided.

Signed-off-by: Antonio Quartulli 
---
 src/openvpn/networking.h  | 26 +++
 src/openvpn/networking_iproute2.c | 74 ---
 src/openvpn/networking_iproute2.h |  1 +
 src/openvpn/networking_sitnl.c| 12 +
 src/openvpn/openvpn.c |  1 +
 src/openvpn/route.c   |  8 
 src/openvpn/tun.c |  5 +++
 7 files changed, 82 insertions(+), 45 deletions(-)

diff --git a/src/openvpn/networking.h b/src/openvpn/networking.h
index cf967116..4075ed73 100644
--- a/src/openvpn/networking.h
+++ b/src/openvpn/networking.h
@@ -39,6 +39,18 @@ net_ctx_init(struct context *c, openvpn_net_ctx_t *ctx)
 {
 return 0;
 }
+
+static inline void
+net_ctx_reset(openvpn_net_ctx_t *ctx)
+{
+(void)ctx;
+}
+
+static inline void
+net_ctx_free(openvpn_net_ctx_t *ctx)
+{
+(void)ctx;
+}
 #endif
 
 #if defined(ENABLE_SITNL) || defined(ENABLE_IPROUTE)
@@ -53,6 +65,20 @@ net_ctx_init(struct context *c, openvpn_net_ctx_t *ctx)
  */
 int net_ctx_init(struct context *c, openvpn_net_ctx_t *ctx);
 
+/**
+ * Release resources allocated by the internal garbage collector
+ *
+ * @param ctx   the implementation specific context
+ */
+void net_ctx_reset(openvpn_net_ctx_t *ctx);
+
+/**
+ * Release all resources allocated within the platform specific context object
+ *
+ * @param ctx   the implementation specific context to release
+ */
+void net_ctx_free(openvpn_net_ctx_t *ctx);
+
 /**
  * Bring interface up or down.
  *
diff --git a/src/openvpn/networking_iproute2.c 
b/src/openvpn/networking_iproute2.c
index 5db9a78b..f06af724 100644
--- a/src/openvpn/networking_iproute2.c
+++ b/src/openvpn/networking_iproute2.c
@@ -43,10 +43,23 @@ net_ctx_init(struct context *c, openvpn_net_ctx_t *ctx)
 ctx->es = NULL;
 if (c)
 ctx->es = c->es;
+ctx->gc = gc_new();
 
 return 0;
 }
 
+void
+net_ctx_reset(openvpn_net_ctx_t *ctx)
+{
+gc_reset(&ctx->gc);
+}
+
+void
+net_ctx_free(openvpn_net_ctx_t *ctx)
+{
+gc_free(&ctx->gc);
+}
+
 int
 net_iface_up(openvpn_net_ctx_t *ctx, const char *iface, bool up)
 {
@@ -82,17 +95,14 @@ net_addr_v4_add(openvpn_net_ctx_t *ctx, const char *iface,
 {
 struct argv argv = argv_new();
 
-char *addr_str = (char *)print_in_addr_t(*addr, 0, NULL);
-char *brd_str = (char *)print_in_addr_t(*broadcast, 0, NULL);
+char *addr_str = (char *)print_in_addr_t(*addr, 0, &ctx->gc);
+char *brd_str = (char *)print_in_addr_t(*broadcast, 0, &ctx->gc);
 
 argv_printf(&argv, "%s addr add dev %s %s/%d broadcast %s", iproute_path,
 iface, addr_str, prefixlen, brd_str);
 argv_msg(M_INFO, &argv);
 openvpn_execve_check(&argv, ctx->es, S_FATAL, "Linux ip addr add failed");
 
-free(addr_str);
-free(brd_str);
-
 argv_reset(&argv);
 
 return 0;
@@ -103,7 +113,7 @@ net_addr_v6_add(openvpn_net_ctx_t *ctx, const char *iface,
 const struct in6_addr *addr, int prefixlen)
 {
 struct argv argv = argv_new();
-char *addr_str = (char *)print_in6_addr(*addr, 0, NULL);
+char *addr_str = (char *)print_in6_addr(*addr, 0, &ctx->gc);
 
 argv_printf(&argv, "%s -6 addr add %s/%d dev %s", iproute_path, addr_str,
 prefixlen, iface);
@@ -111,8 +121,6 @@ net_addr_v6_add(openvpn_net_ctx_t *ctx, const char *iface,
 openvpn_execve_check(&argv, ctx->es, S_FATAL,
  "Linux ip -6 addr add failed");
 
-free(addr_str);
-
 argv_reset(&argv);
 
 return 0;
@@ -123,7 +131,7 @@ net_addr_v4_del(openvpn_net_ctx_t *ctx, const char *iface,
 const in_addr_t *addr, int prefixlen)
 {
 struct argv argv = argv_new();
-char *addr_str = (char *)print_in_addr_t(*addr, 0, NULL);
+char *addr_str = (char *)print_in_addr_t(*addr, 0, &ctx->gc);
 
 argv_printf(&argv, "%s addr del dev %s %s/%d", iproute_path, iface,
 addr_str, prefixlen);
@@ -131,8 +139,6 @@ net_addr_v4_del(openvpn_net_ctx_t *ctx, const char *iface,
 argv_msg(M_INFO, &argv);
 openvpn_execve_check(&argv, ctx->es, 0, "Linux ip addr del failed");
 
-free(addr_str);
-
 argv_reset(&argv);
 
 return 0;
@@ -143,15 +149,13 @@ net_addr_v6_del(openvpn_net_ctx_t *ctx, co