Re: [Openvpn-devel] [Openvpn-users] [PATCH] Add support for specifying the syslog facility, as requested in trac #188.

2014-05-02 Thread Gert Doering
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.

2014-05-02 Thread Jonathan K. Bullard
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.

2014-05-02 Thread Gert Doering
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.

2014-05-02 Thread David Sommerseth

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.

2014-05-02 Thread David Sommerseth

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.

2014-05-02 Thread Gert Doering
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.

2014-05-02 Thread Timothe Litt

(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.

2014-05-02 Thread Gert Doering
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