Re: [Openvpn-devel] [Openvpn-users] [PATCH] Add support for specifying the syslog facility, as requested in trac #188.
Hi, On Fri, May 02, 2014 at 05:59:26PM +0200, Gert Doering wrote: > > - Why not re-use facilitynames[] from sys/syslog.h, instead of adding > > something which is basically a duplicate of exactly that lookup table? > Oh, wow. I didn't know that this exists - learned something new today. JFTR, this does *not* exist on Solaris (which we support) or AIX (which we do not support yet, but I'm toying with it since it recently grew TAP support...). It does exist on Linux, FreeBSD, NetBSD, MacOS X, OpenBSD. MinGW/Windows does not seem to have a syslog.h at all gert -- USENET is *not* the non-clickable part of WWW! //www.muc.de/~gert/ Gert Doering - Munich, Germany g...@greenie.muc.de fax: +49-89-35655025g...@net.informatik.tu-muenchen.de pgpXnZgPckq59.pgp Description: PGP signature
Re: [Openvpn-devel] [Openvpn-users] [PATCH] Add support for specifying the syslog facility, as requested in trac #188.
On Fri, May 2, 2014 at 11:20 AM, David Sommerseth < openvpn.l...@topphemmelig.net> wrote: > The core principle in OpenVPN's option > parsing is that the last argument wins. So if you have f.ex. --ping-exit > 3 > times in a command line and two times in a config file, it's the last one > which really sets the option. --syslog-facility should be no different. > > IMO, distro init.d scripts should add the config file as the last argument > when they kick off openvpn. If they add stuff which overrides the config > file, then it's a bug in the init.d script. > That might be good for distros, but Tunnelblick purposefully starts OpenVPN with "--management" *after* "--config"; other GUIs may do this, too. Tunnelblick does this to make sure that the management interface is used only by Tunnelblick. (Tunnelblick separately warns the user if the configuration file contains "--management" or other problematic options.) For some options, it is the *first* argument that wins: "--log", "--log-append", and I think "--daemon" (and possibly some other options like "--syslog"). Tunnelblick puts --log and --daemon first to make sure it controls logging. (Tunnelblick sends the log to a file, which it monitors, instead of logging through the management interface, so Tunnelblick (the GUI) does not need to be running when the OpenVPN instance is running -- for example, when nobody is logged in.)
Re: [Openvpn-devel] [Openvpn-users] [PATCH] Add support for specifying the syslog facility, as requested in trac #188.
Hi, On Fri, May 02, 2014 at 05:20:02PM +0200, David Sommerseth wrote: > - Why not re-use facilitynames[] from sys/syslog.h, instead of adding > something which is basically a duplicate of exactly that lookup table? Oh, wow. I didn't know that this exists - learned something new today. We might have to figure out how this is portably used - I see it exists on Linux and FreeBSD, at least, and both have it inside #ifdef SYSLOG_NAMES, so our code might have to do #define SYSLOG_NAMES #include or such. Plus possibly an autoconf test to see whether it's there, and if not, fall back to something in compat/ - that would make the change to options.c much smaller. > - Further, why the #ifdef's on LOG_FTP, LOG_SYSLOG, LOG_UUCP? "Because not all systems have them", I'd say :-) - syslog facilities are far from universal, alas. [..] > Using --syslog-facility is a bare minimum acceptable but not preferred. > Rather consider --daemon [progname] [facility] and --syslog [progname] > [facility]. If facility is not set, default to whatever is the default now > (IIRC, it is syslog). I think it would be good to loosen the restrictions that --syslog has to be called before --daemon, and move that to "latest option wins" (as usual). Haven't checked the code to see how complicated that is. gert -- USENET is *not* the non-clickable part of WWW! //www.muc.de/~gert/ Gert Doering - Munich, Germany g...@greenie.muc.de fax: +49-89-35655025g...@net.informatik.tu-muenchen.de pgpPk68MvBMgg.pgp Description: PGP signature
Re: [Openvpn-devel] [Openvpn-users] [PATCH] Add support for specifying the syslog facility, as requested in trac #188.
On 02/05/14 09:23, Gert Doering wrote: Hi, (why is this being discussed on -users? Taking it over to -devel) On Thu, May 01, 2014 at 09:52:45PM -0400, Timothe Litt wrote: 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) Uh. Feature-NAK - especially the last one really makes me want to throw up. Using [] as part of an option string is a bad idea, because it is commonly used for optional arguments (as you saw in David's question), and intermixing option argument "A" and "B" in the same string is not going to help anyone. Agreed! If we take this at all - and I have reservations, because the amount of code it brings in is quite significant - it should not touch the --daemon command line. It brings it's own config statement (--syslog-facility), so there really is no reason to mess up the syntax for --daemon. Also, --syslog-facility should be more clean, like --sysloc-facility facility [progname] with "progname" actually being *optional*, not "marked by using rectangular brackets". Agreed as well. This is primarily what I'd expect this to do too. [..] 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(). Init scripts can be adjusted. Obscure code inside OpenVPN stays forever (guess why the developers have so little time? right, maintaining stuff that someone else considered "a clever hack" 5 years ago). On distros which don't have package management, this is probably a fine thing. But on most Linux distros with package management, init.d scripts will be replaced on package updates. As we don't provide init.d script for all distros, it will not really be possible to change this. Further, it's not that uncommon that software adds a facility option, so I find this core feature a good enhancement. But! I do not like the [] syntax. The core principle in OpenVPN's option parsing is that the last argument wins. So if you have f.ex. --ping-exit 3 times in a command line and two times in a config file, it's the last one which really sets the option. --syslog-facility should be no different. IMO, distro init.d scripts should add the config file as the last argument when they kick off openvpn. If they add stuff which overrides the config file, then it's a bug in the init.d script. Anyway NAK to the v2 patch. Because it really didn't give answers to many of my questions. - We do not need SYSLOG_CAPABILITY, is there another #ifdef which restricts SYSLOG somehow? If yes, use that, if not, don't do this. - Why not re-use facilitynames[] from sys/syslog.h, instead of adding something which is basically a duplicate of exactly that lookup table? - Further, why the #ifdef's on LOG_FTP, LOG_SYSLOG, LOG_UUCP? - Don't fiddle around with malloc(). Use gc_arena and the string buffer functions defined in buffer.c/h Please, make the next round a far more simpler version, reuse what is already implemented and well tested other places rather than re-inventing the wheel. Using --syslog-facility is a bare minimum acceptable but not preferred. Rather consider --daemon [progname] [facility] and --syslog [progname] [facility]. If facility is not set, default to whatever is the default now (IIRC, it is syslog). -- kind regards, David Sommerseth
Re: [Openvpn-devel] [Openvpn-users] [PATCH] Add support for specifying the syslog facility, as requested in trac #188.
On 02/05/14 09:23, Gert Doering wrote: Hi, (why is this being discussed on -users? Taking it over to -devel) That's my fault. Well, it was the "Correct Identity" add-on in Thunderbird which suddenly began to pick the wrong identity (-users ML) which has a different reply-to setting. Sorry about that! -- kind regards, David Sommerseth
Re: [Openvpn-devel] [Openvpn-users] [PATCH] Add support for specifying the syslog facility, as requested in trac #188.
Hi, On Fri, May 02, 2014 at 08:15:01AM -0400, Timothe Litt wrote: > >(why is this being discussed on -users? Taking it over to -devel) > Er, I did post it to -devel, and I don't see any discussion of it on > -users...I just double-checked the archives, and it's all in -devel... The mail I replied to went to -users, and only there... > > --syslog-facility should be more clean, like > > > > --sysloc-facility facility [progname] > > --daemon and --syslog already control syslog's program name. Since we > would have to support the > existing usages, your suggestion to add that to --syslog-facility would > add complexity, not simplify things. > You really don't want that. Then let's not do that. Fine with me :-) > > intermixing > >option argument "A" and "B" in the same string is not going to help anyone. > Sorry that you don't like my accommodation for init scripts. It helps > OpenVPN and its USERS. "Helping init scripts" is actually something that only helps distribution builders, and they are few, and usually listen quite well if we tell them what needs to be adjusted. "Having unmaintainable code inside OpenVPN" is something that does not help anyone - people will forget why it is the way it is, break it when changing something else, and that will cause endless pain. We've been through that right now when rewriting socket.c to properly support dual-stack systems, breaking *some* bits of SOCKS proxy support in the process, breaking --inetd in the process, and so on - so we know all about non-intuitive options and hard-to-maintain code. [..] > >the last one really makes me want to throw up. > Yes, it's ugly. And my stomach rumbles too. So do not write such code, and do not expect us to merge it. > If OpenVPN were engineered from scratch, --daemon would just daemonize, > --syslog would control the syslog program name and facility, and the two > could be used together. And there wouldn't be silly ordering > constraints among the options. But that's not the way the existing code > -- and customers' config and distros' init files -- work. Then let's fix *that* problem (the ordering constraints), instead of adding more convolutions. That would indeed help "OpenVPN and the users". > This is the real world, and sometimes we have to live with ugly. Even if > it makes us a little queasy. No :-) - it's open source world, and since everyone can see what we do, we're not going to add more ugliness if we can avoid it. Closed source works differently. [..] > So it has to be something that can be embedded anywhere in the string, > And that means delimiters on both ends. "Anywhere in the string" is a hard "no". [..] > I didn't ignore it for 2 years, and I didn't let aesthetics get in the > way of the best solution that I could come up with. Yeah, I understand that the amount of free time we can give into OpenVPN is annoying you. It's annoying me as well that I have spend time in paid-for projects not related to OpenVPN to help feed my family, or that David had to tune down his involvement into OpenVPN due to health reasons. The amount of cleanup we've done in the last two years is quite enormous :-) - even if it's not really visible outside of the git log. [..] > >Feature-NAK > If you don't want the feature, why did you write in Trac #188: I'm fine with the addition of a way to set the syslog-facility, but not the way it is presented. Not intermixing stuff in other stuff in arguments. gert -- USENET is *not* the non-clickable part of WWW! //www.muc.de/~gert/ Gert Doering - Munich, Germany g...@greenie.muc.de fax: +49-89-35655025g...@net.informatik.tu-muenchen.de pgpAj00UqMIFK.pgp Description: PGP signature
Re: [Openvpn-devel] [Openvpn-users] [PATCH] Add support for specifying the syslog facility, as requested in trac #188.
(why is this being discussed on -users? Taking it over to -devel) Er, I did post it to -devel, and I don't see any discussion of it on -users...I just double-checked the archives, and it's all in -devel... --syslog-facility should be more clean, like --sysloc-facility facility [progname] --daemon and --syslog already control syslog's program name. Since we would have to support the existing usages, your suggestion to add that to --syslog-facility would add complexity, not simplify things. You really don't want that. intermixing option argument "A" and "B" in the same string is not going to help anyone. Sorry that you don't like my accommodation for init scripts. It helps OpenVPN and its USERS. Init scripts can be adjusted. The fact is that I'm not going to get all the distributions to change their init scripts, and neither are you. It's a major effort to do that and get it coordinated across all the distributions. No one -- not me, not you, and not the distro maintainers -- has time for that. Asking our users to do it piecemeal - and maintain it or lobby their distributions is not good engineering. It's a problem that we can solve on this end. Even if one could get distro cycles to change their init scripts - why would we waste them on something like this? We'll want them when we want something that can't be handled without their help... the last one really makes me want to throw up. Yes, it's ugly. And my stomach rumbles too. If OpenVPN were engineered from scratch, --daemon would just daemonize, --syslog would control the syslog program name and facility, and the two could be used together. And there wouldn't be silly ordering constraints among the options. But that's not the way the existing code -- and customers' config and distros' init files -- work. This is the real world, and sometimes we have to live with ugly. Even if it makes us a little queasy. Using [] as part of an option string is a bad idea, because it is commonly used for optional arguments (as you saw in David's question), and As for the choice of square brackets - the delimiter had to be something easy to parse that is uncommon in program and file names. The init file usage does not make it easy. Facility can not be a prefix, because the Debian usage I cite generates the --daemon argument from the config file name -- adding a prefix. It can't be a suffix because of the way systemd-based distros auto-generate multiple instances. So it has to be something that can be embedded anywhere in the string, And that means delimiters on both ends. I was aware that [ ] is used for 'optional' in syntax descriptions when I picked them. But I provided examples to clarify the point. And the other choices were worse, as they have other meanings to shells and filesystems. At least [ ] is visually distinctive and easy to remember. It could be something else. Do you have a better idea that doesn't add complexity? It brings it's own config statement (--syslog-facility) Actually, the first pass didn't - the facility was ONLY in the progname argument. See the patch in Trac, which was the minimal change required to solve the problems. However, the additional code is minimal, and for cases where it is possible to avoid the ugly overloading of progname, it is a cleaner way to specify facility. I thought it worth the effort so as not to force ugliness on users when it isn't required. someone else considered "a clever hack" None of this was the result of random choices or "cleverness". I've been in the software business long enough to have dealt with more of that than most. Try adding a major feature to a 30-year old OS that's written in assembler...or to microcode. I thought carefully about the issues, the requirement and how to meet it in a way that minimizes the internal and external impacts. I'm busy too - but I took the time to understand the problem and do a proper job. I checked multiple distributions, consulted standards, localized the changes and did careful testing in the environments that I have. And tried to give back to the community. I could have just compiled with -DLOG_OPENVPN=LOG_LOCAL5 and gone on with my life... Or thrown an 'elegant' solution over the wall and left customers to deal with the init file issues. I didn't ignore it for 2 years, and I didn't let aesthetics get in the way of the best solution that I could come up with. If we take this at all - and I have reservations, because the amount of code it brings in is quite significant The scope is hardly large. If you look at the actual code - the size of the patch is dominated by documentation and the lookup table. There are a couple of lines to parse the option, a subroutine to lookup the names, and local block of code in open to handle the syntax. Although the init-script compatibility is ugly and adds a few lines of code, it meets the requirements with the least
Re: [Openvpn-devel] [Openvpn-users] [PATCH] Add support for specifying the syslog facility, as requested in trac #188.
Hi, (why is this being discussed on -users? Taking it over to -devel) On Thu, May 01, 2014 at 09:52:45PM -0400, Timothe Litt wrote: > 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) Uh. Feature-NAK - especially the last one really makes me want to throw up. Using [] as part of an option string is a bad idea, because it is commonly used for optional arguments (as you saw in David's question), and intermixing option argument "A" and "B" in the same string is not going to help anyone. If we take this at all - and I have reservations, because the amount of code it brings in is quite significant - it should not touch the --daemon command line. It brings it's own config statement (--syslog-facility), so there really is no reason to mess up the syntax for --daemon. Also, --syslog-facility should be more clean, like --sysloc-facility facility [progname] with "progname" actually being *optional*, not "marked by using rectangular brackets". [..] > 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(). Init scripts can be adjusted. Obscure code inside OpenVPN stays forever (guess why the developers have so little time? right, maintaining stuff that someone else considered "a clever hack" 5 years ago). gert -- USENET is *not* the non-clickable part of WWW! //www.muc.de/~gert/ Gert Doering - Munich, Germany g...@greenie.muc.de fax: +49-89-35655025g...@net.informatik.tu-muenchen.de pgpR2GjqX4Ja9.pgp Description: PGP signature
