Add management command to reopen log file.

PATCH NAK Gabriel Corona 3 participants 3 messages Jan 11, 2014 – Jan 12, 2014

Share & tools

Gabriel Corona

author

2014-01-11 15:53 UTC #0 lore raw

PATCH 1/1 Add management command to reopen log file.

Useful for better logrotate integration without going the syslog path. Triggered by the "reopen-log" management command.

---
 configure.ac          |  8 ++++++++
 src/openvpn/error.c   | 14 ++++++++++++++
 src/openvpn/init.c    |  7 +++++++
 src/openvpn/manage.c  | 35 +++++++++++++++++++++++++++++++++++
 src/openvpn/manage.h  |  9 +++++++++
 src/openvpn/options.c |  8 ++++++++
 src/openvpn/options.h |  4 ++++
 7 files changed, 85 insertions(+)

configure.ac +8 -0

index 7b35e50..a2079c4 100644
@@ -110,6 +110,13 @@ AC_ARG_ENABLE(
 )
 
 AC_ARG_ENABLE(
+	[reopen-log],
+	[AS_HELP_STRING([--disable-reopen-log], [disable reopening log files @<:@default=yes@:>@])],
+	,
+	[enable_reopen_log="yes"]
+)
+
+AC_ARG_ENABLE(
 	[pkcs11],
 	[AS_HELP_STRING([--enable-pkcs11], [enable pkcs11 support @<:@default=no@:>@])],
 	,

@@ -924,6 +931,7 @@ test "${ac_cv_header_sys_uio_h}" = "yes" && AC_DEFINE([HAVE_IOVEC], [1], [struct
 test "${enable_multi}" = "yes" && AC_DEFINE([ENABLE_CLIENT_SERVER], [1], [Enable client/server capability])
 test "${enable_server}" = "no" && AC_DEFINE([ENABLE_CLIENT_ONLY], [1], [Enable client capability only])
 test "${enable_management}" = "yes" && AC_DEFINE([ENABLE_MANAGEMENT], [1], [Enable management server capability])
+test "${enable_reopen_log}" = "yes" && AC_DEFINE([ENABLE_REOPEN_LOG], [1], [Enable reopening log file])
 test "${enable_socks}" = "yes" && AC_DEFINE([ENABLE_SOCKS], [1], [Enable Socks proxy support])
 test "${enable_http_proxy}" = "yes" && AC_DEFINE([ENABLE_HTTP_PROXY], [1], [Enable HTTP proxy support])
 test "${enable_multihome}" = "yes" && AC_DEFINE([ENABLE_MULTIHOME], [1], [Enable multi-homed UDP server capability])

src/openvpn/error.c +14 -0

index 6848425..c0c75e8 100644
@@ -467,6 +467,20 @@ get_orig_stderr (void)
 
 #endif
 
+#ifdef ENABLE_REOPEN_LOG
+void
+reopen_log (const char* file, bool append)
+{
+  if(file==NULL)
+    return;
+  flockfile(default_out);
+  fflush(default_out);
+  std_redir = false;
+  redirect_stdout_stderr(file, append);
+  funlockfile(stdout);
+}
+#endif
+
 void
 redirect_stdout_stderr (const char *file, bool append)
 {

src/openvpn/init.c +7 -0

index 2420216..2666236 100644
@@ -3164,6 +3164,10 @@ open_management (struct context *c)
 	  unsigned int flags = c->options.management_flags;
 	  if (c->options.mode == MODE_SERVER)
 	    flags |= MF_SERVER;
+#ifdef ENABLE_REOPEN_LOG
+	  if (c->options.logappend)
+	    flags |= MF_LOG_APPEND;
+#endif
 	  if (management_open (management,
 			       c->options.management_addr,
 			       c->options.management_port,

@@ -3175,6 +3179,9 @@ open_management (struct context *c)
 			       c->options.management_state_buffer_size,
 			       c->options.management_write_peer_info_file,
 			       c->options.remap_sigusr1,
+#ifdef ENABLE_REOPEN_LOG
+			       c->options.logfile,
+#endif
 			       flags))
 	    {
 	      management_set_state (management,

src/openvpn/manage.c +35 -0

index 0a4542a..d5e3cbf 100644
@@ -85,6 +85,9 @@ man_help ()
   msg (M_CLIENT, "load-stats             : Show global server load stats.");
   msg (M_CLIENT, "log [on|off] [N|all]   : Turn on/off realtime log display");
   msg (M_CLIENT, "                         + show last N lines or 'all' for entire history.");
+#ifdef ENABLE_REOPEN_LOG
+  msg (M_CLIENT, "reopen-log             : Reopen log file for log file rotation");
+#endif
   msg (M_CLIENT, "mute [n]               : Set log mute level to n, or show level if n is absent.");
   msg (M_CLIENT, "needok type action     : Enter confirmation for NEED-OK request of 'type',");
   msg (M_CLIENT, "                         where action = 'ok' or 'cancel'.");

@@ -587,6 +590,14 @@ man_log (struct management *man, const char *parm)
 	       LOG_PRINT_INT_DATE|LOG_PRINT_MSG_FLAGS);
 }
 
+#ifdef ENABLE_REOPEN_LOG
+static void
+man_reopen_log (struct management *man)
+{
+  reopen_log (man->settings.logfile, man->settings.flags & MF_LOG_APPEND);
+}
+#endif
+
 static void
 man_echo (struct management *man, const char *parm)
 {

@@ -1230,6 +1241,12 @@ man_dispatch_command (struct management *man, struct status_output *so, const ch
 	    man_log (man, p[2]);
 	}
     }
+#ifdef ENABLE_REOPEN_LOG
+  else if (streq (p[0], "reopen-log"))
+    {
+      man_reopen_log (man);
+    }
+#endif
   else if (streq (p[0], "echo"))
     {
       if (man_need (man, p, 1, MN_AT_LEAST))

@@ -1966,6 +1983,9 @@ man_settings_init (struct man_settings *ms,
 		   const int state_buffer_size,
 		   const char *write_peer_info_file,
 		   const int remap_sigusr1,
+#ifdef ENABLE_REOPEN_LOG
+		   const char *logfile,
+#endif
 		   const unsigned int flags)
 {
   if (!ms->defined)

@@ -2031,6 +2051,12 @@ man_settings_init (struct man_settings *ms,
 		(GETADDR_RESOLVE|GETADDR_WARN_ON_SIGNAL|GETADDR_FATAL, addr, 0, NULL, NULL);
 	    }
 	}
+#ifdef ENABLE_REOPEN_LOG
+      if (logfile)
+	{
+	  ms->logfile = strdup(logfile);
+	}
+#endif
       
       /*
        * Log history and echo buffer may need to be resized

@@ -2055,6 +2081,9 @@ static void
 man_settings_close (struct man_settings *ms)
 {
   free (ms->write_peer_info_file);
+#ifdef ENABLE_REOPEN_LOG
+  free (ms->logfile);
+#endif
   CLEAR (*ms);
 }

@@ -2156,6 +2185,9 @@ management_open (struct management *man,
 		 const int state_buffer_size,
 		 const char *write_peer_info_file,
 		 const int remap_sigusr1,
+#ifdef ENABLE_REOPEN_LOG
+		 const char *logfile,
+#endif
 		 const unsigned int flags)
 {
   bool ret = false;

@@ -2175,6 +2207,9 @@ management_open (struct management *man,
 		     state_buffer_size,
 		     write_peer_info_file,
 		     remap_sigusr1,
+#ifdef ENABLE_REOPEN_LOG
+		     logfile,
+#endif
 		     flags);
 
   /*

src/openvpn/manage.h +9 -0

index 28da69f..360022b 100644
@@ -217,6 +217,9 @@ struct man_settings {
   struct sockaddr_un local_unix;
 #endif
   bool management_over_tunnel;
+#ifdef ENABLE_REOPEN_LOG
+  char* logfile;
+#endif
   struct user_pass up;
   int log_history_cache;
   int echo_buffer_size;

@@ -334,6 +337,9 @@ struct management *management_init (void);
 #define MF_UP_DOWN          (1<<10)
 #define MF_QUERY_REMOTE     (1<<11)
 #define MF_QUERY_PROXY      (1<<12)
+#ifdef ENABLE_REOPEN_LOG
+#define MF_LOG_APPEND       (1<<13)
+#endif
 
 bool management_open (struct management *man,
 		      const char *addr,

@@ -346,6 +352,9 @@ bool management_open (struct management *man,
 		      const int state_buffer_size,
 		      const char *write_peer_info_file,
 		      const int remap_sigusr1,
+#ifdef ENABLE_REOPEN_LOG
+		      const char* logfile,
+#endif
 		      const unsigned int flags);
 
 void management_close (struct management *man);

src/openvpn/options.c +8 -0

index 23af272..5cdcf09 100644
@@ -4634,6 +4634,10 @@ add_option (struct options *options,
     {
       VERIFY_PERMISSION (OPT_P_GENERAL);
       options->log = true;
+#ifdef ENABLE_REOPEN_LOG
+      options->logfile = p[1];
+      options->logappend = false;
+#endif
       redirect_stdout_stderr (p[1], false);
     }
   else if (streq (p[0], "suppress-timestamps"))

@@ -4646,6 +4650,10 @@ add_option (struct options *options,
     {
       VERIFY_PERMISSION (OPT_P_GENERAL);
       options->log = true;
+#ifdef ENABLE_REOPEN_LOG
+      options->logfile = p[1];
+      options->logappend = true;
+#endif
       redirect_stdout_stderr (p[1], true);
     }
 #ifdef ENABLE_MEMSTATS

src/openvpn/options.h +4 -0

index f80532c..839d938 100644
@@ -297,6 +297,10 @@ struct options
   int inetd;
 
   bool log;
+#ifdef ENABLE_REOPEN_LOG
+  char* logfile;
+  bool logappend;
+#endif
   bool suppress_timestamps;
   int nice;
   int verbosity;

Signature · 1 line

1.8.5.2

Gert Doering

NAK

2014-01-11 19:59 UTC #1 lore raw

Hi,

Quoted 2 lines · Gabriel Corona

Useful for better logrotate integration without going the syslog path. Triggered by the "reopen-log" management command.

NAK, the way it is written. It is introducing *18* additional #ifdefs, and we have already way too many of them, causing testing troubles to no end.

Dubious on the feature itself - why not use syslog if you need log file rotation (= "neither an ACK nor a NAK")?

gert

Signature · 4 lines

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

David Sommerseth

2014-01-12 01:12 UTC #2 lore raw

Quoted 6 lines · Gert Doering

Hi,
Quoted 2 lines · Gabriel Corona
Useful for better logrotate integration without going the syslog path. Triggered by the "reopen-log" management command.

Quoted 6 lines

NAK, the way it is written. It is introducing *18* additional #ifdefs, and we have already way too many of them, causing testing troubles to no end.
Dubious on the feature itself - why not use syslog if you need log file rotation (= "neither an ACK nor a NAK")?

I agree with Gert here.

In regards to the feature, I can't really say I see any good use case. Well, I do see the usefulness of such a feature, but I would rather say that's a bad setup than a lacking feature.

So I'm just thinking aloud here now. For servers where this matters, you normally already have syslog available. And syslog and logrotate (or similar approaches) already covers this. The only place I see this being useful is where you don't have syslog available. But in those places, you normally don't these requirements for log rotation (like embedded devices, with little memory and/or storage). Many of these places you even don't have a persistent storage for log files. And considering many Linux based embedded devices now also seems to move over to systemd as well, you already have the journal included, which can cover many of the syslog+logrotate requirements. (In fact systemd can even grab program output sent to stdout and stderr and put that into the journal).

I'm not saying NAK to the feature, but an ACK won't come from me before I see some convincing use cases.

Signature · 3 lines

kind regards,

David Sommerseth