Thank you for the review. Responses below. Timothe Litt ACM Distinguished Engineer -------------------------- This communication may not represent the ACM or my employer's views, if any, on the matters discussed.
On 01-May-14 19:08, David Sommerseth wrote:
If you say --daemon/--syslog, you specify an optional string that has always been called called progname.Just done a very quick glaring-at-code-review ... generally looks reasonable, and I like to get support for facilities in the syslog API ... but see my comments below.On 28/04/14 23:44, Timothe Litt wrote:Users want this because it allows them to have syslog put all OpenVPN messagesin specific file(s) by using syslog.conf.Adds --syslog-facility name option, which must precede --daemon and --syslog.Adds ability to specify facility as [name] in --daemon and --syslog's prognameargument,which makes it possible to specify the syslog facility name without modifyingcertain system init scripts. --syslog-facility takes precedence. For example, Debian automatically generates a --daemon vpn-{configname}directive. If you name the config foo[local1].conf, it's as though you wereable tospecify --syslog-facility local1 --daemon vpn-foo --config foo[local1].conf.Absentthis feature, This isn't possible without modifying the distribution-providedinit script.--syslog-facility list (or any other undefined name) will list the facilitiesthat the platform supports.The old method of -DLOG_OPENVPN still provides the facility used by default.Thus, the priority is: --syslog-facility --daemon [facility] or --syslog [facility]Isn't it [progname] which is appropriate on --daemon/--syslog ?
The [ ] notation does not mean "optionally" here.The [facility] notation means literally, a facility name in square brackets, which can be anywhere in the progname string.
e.g. the following are valid:
--daemon
Daemonize, use the default progname (from PACKAGE)
for syslog
--daemon my-vpn-daemon
Daemonize, use "my-vpn-daemon" for the syslog progname.
--daemon [local1]
Daemonize, use the default progname, use the local1
syslog facility (unless --syslog-facility already specified one)
--daemon my[local1]-vpn-daemon --daemon [local1]my-vpn-daemon --daemon my-vpn-daemon[local1] Daemonize, use "my-vpn-daemon" for the syslog progname. Use the local1 syslog facility (unless --syslog-facility already specified one)
--syslog takes the same argument formats, with the same results (obviously, not daemonizing).
As I explain above, this is to make it possible to specify a facility name in cases where the system init script uses --daemon before the user can put a --syslog-facility. --syslog-facility must precede --daemon or --syslog because the latter two immediately switch logging to syslog; they are what consume the facility code when they call openlog().
Priority should be intuitive to users, but I guess I didn't use enough words. Since the facility can be specified three ways, one has to say how conflicts are resolved. The three ways are:
1 --syslog-facility
2 embedded in the progname argument of either --daemon or --syslog
3 From the #define LOG_OPENVPN (which can be specified as a -D in
CCFLAGS)
This is resolved by priority:- if --syslog-facility is used, that's where the facility comes from. The progname string is not parsed for a facility name.
- if --syslog-facility is NOT used-- the progname string is parsed for a facility name in [ ], which if present is used
- If neither --syslog-facility nor [facility] in programe (with --syslog or --daemon) is used, the hardcoded facility is used as before.
I can't say what that is in the man page, since it's value of #define LOG_OPENVPN that some people use today, (but the # define defaults to 'daemon')
\/ -- the last item in the list
Remember that the comments in the commit text don't go in the documentation, this is the only place that the end-user will see that there is an option to specify the facility code within the progname string. I think it's worth providing the explanation. I didn't want to write a book.-DLOG_OPENVPNThis is forward and backward compatible with existing scripts & initfiles.Signed-off-by: Timothe Litt <l...@acm.org> --- doc/openvpn.8 | 15 ++++++src/openvpn/error.c | 129 +++++++++++++++++++++++++++++++++++++++++++++++--src/openvpn/error.h | 1 + src/openvpn/options.c | 19 ++++++++ 4 files changed, 161 insertions(+), 3 deletions(-) diff --git a/doc/openvpn.8 b/doc/openvpn.8 index 34894e5..2c63795 100644 --- a/doc/openvpn.8 +++ b/doc/openvpn.8 @@ -2169,6 +2169,12 @@ When unspecified, .B progname defaults to "openvpn". +If --syslog-facility has not been specified and the name+includes [facility], the specified syslog facility name will be used with+the system logger. (This method of specifying the facility name may be +convenient when initialization scripts generate --daemon progname from +the config file name. This avoids the need to edit the script.)I'd skip the comment in the last parentheses. But I'm also wondering if this should just be changed to "see --syslog-facility for more info".
As noted above, this depends on the value of a #define that users modify today.When OpenVPN is run with the .B \-\-daemonoption, it will try to delay daemonization until the majority of initialization@@ -2188,6 +2194,15 @@ directive above for description of .B progname parameter. .TP +.B \-\-syslog-faciity name+When logging to syslog, use name (e.g. local1 or daemon) for the facility.+If name is absent or not supported, a platform-dependent list of valid +facility names is provided.I'm missing a comment here what the default facility is.
If you specify --syslog-facility with no argument, the list of valid names is preceded by an error message that says what the default is for this platform/compile. (Unless the user specified a numeric value that doesn't have a name, in which case the word 'default' is used.)
I will add: If this option is not specified, the default facility is usually 'daemon'.
It is always possible, but not convenient, to edit the system init scripts -- some of which forcibly insert --daemon on the comand line.+Must be specified before --daemon and --syslog.+If this is not convenient, the facility name may be specified as [name] in+the name parameter of either directive.The "if this is not convenient..." comment is confusing me.
Absent the [name] scheme, when the init script forces --daemon, one has to modify the init script with --syslog-facility foo before the --daemon - and maintain the change.
This issue is also noted in a previously-existing error message that appears when you specify --daemon twice (typically, once on
the command line and once in the config file.) I'll change 'convenient' to 'useful'.
+.TP .B \-\-errors-to-stderrOutput errors to stderr instead of stdout unless log output is redirected byone of the .B \-\-log diff --git a/src/openvpn/error.c b/src/openvpn/error.c index fd9f19d..fb0abbd 100644 --- a/src/openvpn/error.c +++ b/src/openvpn/error.c @@ -89,9 +89,10 @@ static bool machine_readable_output; /* GLOBAL */ /* Should timestamps be included on messages to stdout/stderr? */ static bool suppress_timestamps; /* GLOBAL */ -/* The program name passed to syslog */ +/* The program name and facility passed to syslog */ #if SYSLOG_CAPABILITYI don't see SYSLOG_CAPABILITY being configured in configure.ac. And I'm not convinced we should #ifdef this feature. If anything, nothing more than treating it like syslog in general.
I *AM* treating it *exactly* like syslog in general.SYSLOG_CAPABILITY already exists. I did not invent it. I did not add it here, just changed the comment.
SYSLOG_CAPABILITY gets set in ./src/openvpn/syshead.h,based on HAVE_OPENLOG && HAVE_SYSLOG. The HAVE_xx constans are defined in config.h.
I did not change any of that.The current syslog code (in addition to the variable definitions that follow this comment change) is already #ifdef SYSLOG_CAPABILITY; I just made the new new code (including the embedded help text) follow suit.
There are platforms without syslog (including native windows and some embedded systems).
Because it is in many <syslog.h> files (including debian and fedora), but is not in the the standard.static char *pgmname_syslog; /* GLOBAL */+static int facility_syslog = -1; /* Unspec (RFC5424 - can not be negative) */#endif/* If non-null, messages should be written here (used for debugging only) */@@ -439,6 +440,93 @@ out_of_memory (void) exit (1); } +#if SYSLOG_CAPABILITY +static int syslog_fac_code (const char *name, int default_code) +{ + static struct { + int code; + const char *const name; + } facnames[] = { + { LOG_AUTH, "auth" }, +#ifdef LOG_AUTHPRIV /* Prefered (private), but non-POSIX */ + { LOG_AUTHPRIV, "authpriv" }, +#else + { LOG_AUTH, "authpriv" }, /* Map to non-secure code */ +#endifThese are fine.+ { LOG_CRON, "cron" }, + { LOG_DAEMON, "daemon" }, +#ifdef LOG_FTP + { LOG_FTP, "ftp" }, +#endifWhy?
I noted this in the Trac ticket. For portability, the ones not in the standard are #ifdefed.There are a limited number of facility codes, so people often re-purpose them. E.g., FTP may be unused by a site, but the FTP facility reused for something else - say OpenVPN.
Note that we would get these if we used the <sys/syslog.h> tables as you suggest further down.
+ { LOG_LPR, "lpr" }, + { LOG_MAIL, "mail" }, + { LOG_NEWS, "news" }, +#ifdef LOG_SYSLOG + { LOG_SYSLOG, "syslog" }, +#endifWhy?
Same answer
+ { LOG_USER, "user" }, +#ifdef LOG_UUCP + { LOG_UUCP, "uucp" }, +#endifWhy?
Same answer
Because syslog.h is standardized [IEEE Std 1003.1-2001 -- goes back to Issue 4, version 2], and the facility names in syslog.h are a non-standard extension.+ { LOG_LOCAL0, "local0" }, + { LOG_LOCAL1, "local1" }, + { LOG_LOCAL2, "local2" }, + { LOG_LOCAL3, "local3" }, + { LOG_LOCAL4, "local4" }, + { LOG_LOCAL5, "local5" }, + { LOG_LOCAL6, "local6" }, + { LOG_LOCAL7, "local7" }, + + { 0, NULL }, + }, *fac;Just another thought ... why not reuse facilitynames[] from sys/syslog.h ? They are all declared there iirc. But needs to be checked up against Solaris and BSD. I don't think this is compiled in Windows, but worth double checking how this code behaves via Windows compilers.
You need to #define an enabling conditional (usually SYSLOG_NAMES) because you normally don't want storage allocated for the strings. That gets ignored in some implementations unless you #define something else (for glibc, __USE_MISC), because SYSLOG_NAMES is considered namespace pollution. So one would have to autoconfigure for that. See http://www.sourceware.org/ml/glibc-bugs/2013-12/msg00172.html for a discussion.
And then, for those platforms that don't have SYSLOG_NAMES (or call it something else), you need to autoconfigure and have this table anyway.
Finally, even when SYSLOG_NAMES *is* present, it includes names for thing that OpenVPN should never use (like INTERNAL_MARK and LOG_KERN). So we'd need a table of names to suppress when listing what's present on the platform.
It's simpler and more standard-conformant to simply add the table. Size is a wash.
The only downside is that a couple of other non-standard names may turn up on odd platforms; it's trivial to add them if someone notices.
As written, any environment that has a syslog that conforms to 1003.1 should be happy. That includes cygwin, any modern Unix, and many non-Unix systems.
Since the existing code already depends on that (the syslog API functions, LOG_PID and LOG_DAEMON), this patch should not break on any currently supported platform. Platforms that don't have the listed constants should not have SYSLOG_CAPABILITY set.
This memory only exists in the scope of this if statement; I didn't think it worth the overhead; there is no way for it to be lost as there is no way to exit the block without freeing it. The control flow is purely linear.+ + if (use_syslog) + { + msg (M_ERR, "syslog facility can not be changed after logging has started"); + return; + } + + /* Lookup facility code by name */ + + for (fac = facnames; fac->name; fac++) + { + if (!strcmp (fac->name, name)) + { + return fac->code; + } + } + + /* Not found, look for name of facility that will be used */ + + if (default_code == -1) + default_code = LOG_OPENVPN; + + for (fac = facnames; fac->name; fac++) + { + if (fac->code == default_code) + { + break; + } + } + + /* Warn, and list valid names (they are platform-dependent) */ ++ msg (M_WARN, "syslog: %s is not a valid facility name, using %s", name,+ (fac->name? fac->name : "default")); + for (fac = facnames; fac->name; fac++) + {+ msg (M_INFO, "syslog: %s facility is valid on this platform", fac->name);+ } + return default_code; +} + +void set_syslog_facility (const char *name) +{ + facility_syslog = syslog_fac_code (name, -1); +} + +#endif + void open_syslog (const char *pgmname, bool stdio_to_null) {@@ -447,8 +535,43 @@ open_syslog (const char *pgmname, bool stdio_to_null){ if (!use_syslog) {- pgmname_syslog = string_alloc (pgmname ? pgmname : PACKAGE, NULL);- openlog (pgmname_syslog, LOG_PID, LOG_OPENVPN); + char *pname = pgmname_syslog = string_alloc (pgmname ?+ pgmname : PACKAGE, NULL);+ int facility = facility_syslog; + if (facility == -1) + { + /* Unspecified, default */ + + facility = LOG_OPENVPN; ++ /* Attempt to extract from program name (simplifies init files) */+ char *facb, *face; + + /* Decode optional [facilityname] prefix */ + + if ((facb = strchr (pgmname_syslog, '[')) != NULL + && (face = strchr (facb+1, ']')) != NULL) + {+ char *facname = malloc (face - facb); /* [name] => name\0 */++ /* Extract facility name and remove from option string */+ + *face++ = '\0'; + strcpy (facname, facb+1); + memmove (facb, face, strlen(face)+1);Maybe we should use the gc_arena stuff? it's declared in buffer.h/.c, iirc.
If it had a longer lifetime, I'd agree.
+ /* Default program name */ + + if (!*pname) + pname = PACKAGE_NAME;Is this one really needed?
Yes. Consider --daemon [local1]In that case (facility, no program name) the program name has to be defaulted to PACKAGE.
It's consistent with the existing OpenVPN routines (open_syslog, close_syslog). The syslog API is actually openlog() and closelog(). 'syslog' is only used as the name of the routine that writes to the log.+ /* Convert extracted facility name to code */ + + facility = syslog_fac_code (facname, facility);Maybe choose a slightly better name than syslog_fac_code(). get_syslog_facility_code() is cleared for me. And it avoids people thinking syslog_fac_code() is a syslog API.
If we're going for extreme clarity, 'get' isn't specific enough.I originally used lookup_syslog_facility_code() - but it seemed rather long, especially for a static function.
I'll revert to that.
That's a question of style. In this case, the difference was trivial and I thought it less messy to show it than to ask a reviewer to look at the whole thing again.Also, merge in your other patch into a single patch. A patch fixing a patch easily gets messy when reviewing. It's actually better to send a v2 patch when this happens, just add a patch revision comment, similar to what you see in commit b77bffe8186647c.
I'm happy to comply. So: I have merged the two patches into one. I have changed the function name. I have tweaked the man page. I have clarified the comit description. I will mail out the revised patch momentarily. Thanks again.
smime.p7s
Description: S/MIME Cryptographic Signature
------------------------------------------------------------------------------ "Accelerate Dev Cycles with Automated Cross-Browser Testing - For FREE Instantly run your Selenium tests across 300+ browser/OS combos. Get unparalleled scalability from the best Selenium testing platform available. Simple to use. Nothing to install. Get started now for free." http://p.sf.net/sfu/SauceLabs
_______________________________________________ Openvpn-users mailing list Openvpn-users@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-users
