date:20170907

Re: [Openvpn-devel] [PATCH] systemd: Enable systemd's auto-restart feature for server profiles

2017-09-07 Thread fragmentux




On 08/09/17 02:45, fragmentux wrote:

Hi,

On 08/09/17 02:30, Steven Haigh wrote:

On 2017-09-08 10:41, fragmentux wrote:

WRT systemd and 3rd party 'servers'

I am curious to know of any other "Server service" who decide ..
to *rely* on ..
a *forced* 'systemd (almost) unconditional restart by default'


As a basic list:

/usr/lib/systemd/system $ grep -r Restart *.service


This is the wrong place to grep ..



Oops .. maybe not "the wrong place" ..
but I stand by my objection.

regards,

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [PATCH v2] route: cleanup codestyle and make code more readable

2017-09-07 Thread Antonio Quartulli

This patch does not introduce any functional change.

The code in route.c seems to have been written in different
periods by different people, without sticking to a clear
codestyle. For this reason the code in this file in not
consistent at all.

Clean it up by:
- removing spaces from function invocations
- cutting line longer than 80 chars (where possible)
- moving function arguments on the same line when there is enough space
- adding empty line between var declarations and code
- adding empty line between code and final return
- adding empty line to make the code less sticky and easier to parse

Signed-off-by: Antonio Quartulli 
---

v2:
- rebased on top of master;
- checked with uncrustify and fixes applied;
- there are some little discrepancies with what uncrustify wants me to do
  but I think it is wrong.



 src/openvpn/route.c | 1583 +--
 1 file changed, 786 insertions(+), 797 deletions(-)

diff --git a/src/openvpn/route.c b/src/openvpn/route.c
index 8c71e6ec..310e510b 100644
--- a/src/openvpn/route.c
+++ b/src/openvpn/route.c
@@ -56,15 +56,21 @@ static bool add_route_service(const struct route_ipv4 *, 
const struct tuntap *);
 
 static bool del_route_service(const struct route_ipv4 *, const struct tuntap 
*);
 
-static bool add_route_ipv6_service(const struct route_ipv6 *, const struct 
tuntap *);
+static bool add_route_ipv6_service(const struct route_ipv6 *,
+   const struct tuntap *);
 
-static bool del_route_ipv6_service(const struct route_ipv6 *, const struct 
tuntap *);
+static bool del_route_ipv6_service(const struct route_ipv6 *,
+   const struct tuntap *);
 
 #endif
 
-static void delete_route(struct route_ipv4 *r, const struct tuntap *tt, 
unsigned int flags, const struct route_gateway_info *rgi, const struct env_set 
*es);
+static void delete_route(struct route_ipv4 *r, const struct tuntap *tt,
+ unsigned int flags,
+ const struct route_gateway_info *rgi,
+ const struct env_set *es);
 
-static void get_bypass_addresses(struct route_bypass *rb, const unsigned int 
flags);
+static void get_bypass_addresses(struct route_bypass *rb,
+ const unsigned int flags);
 
 #ifdef ENABLE_DEBUG
 
@@ -110,8 +116,10 @@ struct route_option_list *
 new_route_option_list(struct gc_arena *a)
 {
 struct route_option_list *ret;
+
 ALLOC_OBJ_CLEAR_GC(ret, struct route_option_list, a);
 ret->gc = a;
+
 return ret;
 }
 
@@ -119,8 +127,10 @@ struct route_ipv6_option_list *
 new_route_ipv6_option_list(struct gc_arena *a)
 {
 struct route_ipv6_option_list *ret;
+
 ALLOC_OBJ_CLEAR_GC(ret, struct route_ipv6_option_list, a);
 ret->gc = a;
+
 return ret;
 }
 
@@ -135,22 +145,28 @@ struct route_option_list *
 clone_route_option_list(const struct route_option_list *src, struct gc_arena 
*a)
 {
 struct route_option_list *ret;
+
 ALLOC_OBJ_GC(ret, struct route_option_list, a);
 *ret = *src;
+
 return ret;
 }
 
 struct route_ipv6_option_list *
-clone_route_ipv6_option_list(const struct route_ipv6_option_list *src, struct 
gc_arena *a)
+clone_route_ipv6_option_list(const struct route_ipv6_option_list *src,
+ struct gc_arena *a)
 {
 struct route_ipv6_option_list *ret;
+
 ALLOC_OBJ_GC(ret, struct route_ipv6_option_list, a);
 *ret = *src;
+
 return ret;
 }
 
 void
-copy_route_option_list(struct route_option_list *dest, const struct 
route_option_list *src, struct gc_arena *a)
+copy_route_option_list(struct route_option_list *dest,
+   const struct route_option_list *src, struct gc_arena *a)
 {
 *dest = *src;
 dest->gc = a;
@@ -169,15 +185,16 @@ static const char *
 route_string(const struct route_ipv4 *r, struct gc_arena *gc)
 {
 struct buffer out = alloc_buf_gc(256, gc);
+
 buf_printf(, "ROUTE network %s netmask %s gateway %s",
print_in_addr_t(r->network, 0, gc),
print_in_addr_t(r->netmask, 0, gc),
-   print_in_addr_t(r->gateway, 0, gc)
-   );
+   print_in_addr_t(r->gateway, 0, gc));
 if (r->flags & RT_METRIC_DEFINED)
 {
 buf_printf(, " metric %d", r->metric);
 }
+
 return BSTR();
 }
 
@@ -188,18 +205,22 @@ is_route_parm_defined(const char *parm)
 {
 return false;
 }
+
 if (!strcmp(parm, "default"))
 {
 return false;
 }
+
 return true;
 }
 
 static void
-setenv_route_addr(struct env_set *es, const char *key, const in_addr_t addr, 
int i)
+setenv_route_addr(struct env_set *es, const char *key, const in_addr_t addr,
+  int i)
 {
 struct gc_arena gc = gc_new();
 struct buffer name = alloc_buf_gc(256, );
+
 if (i >= 0)
 {
 buf_printf(, "route_%s_%d", key, i);
@@ -208,6 +229,7 @@ setenv_route_addr(struct

Re: [Openvpn-devel] [PATCH] systemd: Enable systemd's auto-restart feature for server profiles

2017-09-07 Thread fragmentux


Hi,

On 08/09/17 02:30, Steven Haigh wrote:

On 2017-09-08 10:41, fragmentux wrote:

WRT systemd and 3rd party 'servers'

I am curious to know of any other "Server service" who decide ..
to *rely* on ..
a *forced* 'systemd (almost) unconditional restart by default'


As a basic list:

/usr/lib/systemd/system $ grep -r Restart *.service


This is the wrong place to grep ..



autovt@.service:Restart=always
autovt@.service:RestartSec=0
console-getty.service:Restart=always
console-getty.service:RestartSec=0
container-getty@.service:Restart=always
container-getty@.service:RestartSec=0
dbus-org.freedesktop.login1.service:Restart=always
dbus-org.freedesktop.login1.service:RestartSec=0
debug-shell.service:Restart=always
debug-shell.service:RestartSec=0
getty@.service:Restart=always
getty@.service:RestartSec=0
mariadb.service:# Restart crashed server only, on-failure would also 
restart, for example, when

mariadb.service:Restart=on-abort
mariadb.service:RestartSec=5s
NetworkManager.service:Restart=on-failure
rsyslog.service:Restart=on-failure
rsyslog.service:Restart=on-failure
serial-getty@.service:Restart=always
sshd.service:Restart=on-failure
sshd.service:RestartSec=42s
sssd-autofs.service:Restart=on-failure
sssd-nss.service:Restart=on-failure
sssd-pac.service:Restart=on-failure
sssd-pam.service:Restart=on-failure
sssd-ssh.service:Restart=on-failure
sssd-sudo.service:Restart=on-failure
systemd-journald.service:Restart=always
systemd-journald.service:RestartSec=0
systemd-logind.service:Restart=always
systemd-logind.service:RestartSec=0
systemd-nspawn@.service:RestartForceExitStatus=133
systemd-udevd.service:Restart=always
systemd-udevd.service:RestartSec=0


I said Server as in 'Server' not localhost.

(other than sshd  ..
but that is a whole `nother ball game, let us not get into that)

cheers :)

Regards,

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH v2] lz4: Move towards a newer LZ4 API

2017-09-07 Thread Antonio Quartulli

Hi,

On 08/09/17 01:20, David Sommerseth wrote:

[CUT]

> diff --git a/src/openvpn/comp-lz4.c b/src/openvpn/comp-lz4.c
> index e056caa8..bdb3247d 100644
> --- a/src/openvpn/comp-lz4.c
> +++ b/src/openvpn/comp-lz4.c
> @@ -43,6 +43,7 @@
>  
>  #include "memdbg.h"
>  
> +
>  static void
>  lz4_compress_init(struct compress_context *compctx)
>  {
> @@ -86,7 +87,7 @@ do_lz4_compress(struct buffer *buf,
>  return false;
>  }
>  
> -zlen = LZ4_compress_limitedOutput((const char *)BPTR(buf), (char 
> *)BPTR(work), BLEN(buf), zlen_max );
> +zlen = LZ4_compress_default((const char *)BPTR(buf), (char 
> *)BPTR(work), BLEN(buf), zlen_max );

while at it, could you remove the annoying space between the last
argument and the closing parenthesis? (you or the committer)

Cheers,

>  
>  if (zlen <= 0)
>  {
> 

-- 
Antonio Quartulli



signature.asc
Description: OpenPGP digital signature
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] systemd: Enable systemd's auto-restart feature for server profiles

2017-09-07 Thread Steven Haigh


On 2017-09-08 10:41, fragmentux wrote:

WRT systemd and 3rd party 'servers'

I am curious to know of any other "Server service" who decide ..
to *rely* on ..
a *forced* 'systemd (almost) unconditional restart by default'


As a basic list:

/usr/lib/systemd/system $ grep -r Restart *.service
autovt@.service:Restart=always
autovt@.service:RestartSec=0
console-getty.service:Restart=always
console-getty.service:RestartSec=0
container-getty@.service:Restart=always
container-getty@.service:RestartSec=0
dbus-org.freedesktop.login1.service:Restart=always
dbus-org.freedesktop.login1.service:RestartSec=0
debug-shell.service:Restart=always
debug-shell.service:RestartSec=0
getty@.service:Restart=always
getty@.service:RestartSec=0
mariadb.service:# Restart crashed server only, on-failure would also 
restart, for example, when

mariadb.service:Restart=on-abort
mariadb.service:RestartSec=5s
NetworkManager.service:Restart=on-failure
rsyslog.service:Restart=on-failure
rsyslog.service:Restart=on-failure
serial-getty@.service:Restart=always
sshd.service:Restart=on-failure
sshd.service:RestartSec=42s
sssd-autofs.service:Restart=on-failure
sssd-nss.service:Restart=on-failure
sssd-pac.service:Restart=on-failure
sssd-pam.service:Restart=on-failure
sssd-ssh.service:Restart=on-failure
sssd-sudo.service:Restart=on-failure
systemd-journald.service:Restart=always
systemd-journald.service:RestartSec=0
systemd-logind.service:Restart=always
systemd-logind.service:RestartSec=0
systemd-nspawn@.service:RestartForceExitStatus=133
systemd-udevd.service:Restart=always
systemd-udevd.service:RestartSec=0

--
Steven Haigh

? net...@crc.id.au ? http://www.crc.id.au
? +61 (3) 9001 6090? 0412 935 897

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] systemd: Enable systemd's auto-restart feature for server profiles

2017-09-07 Thread fragmentux



WRT systemd and 3rd party 'servers'

I am curious to know of any other "Server service" who decide ..
to *rely* on ..
a *forced* 'systemd (almost) unconditional restart by default'

Regards,

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] systemd: Enable systemd's auto-restart feature for server profiles

2017-09-07 Thread fragmentux

Hi,

On 07/09/17 23:55, David Sommerseth wrote:

On 07/09/17 23:02, fragmentux wrote:
i,

all your comment are totally valid from a sys-admin point of view but
from an openvpn POV, the only responsibility is to provide a secure VPN.

Use all of systemd's functions to maximize openvpn's process *security*
But *forcing* restart as an almost unconditional default is nonsense.

We are in the position to promote sane and good defaults.  This
behaviour is considered sane and good by many sys-admins.  So when these
two view-points intersects, I see no harm of us actually promoting this
change.

Do you want to discuss the "sanity" of said sys-admins ?

I do not subscribe to "popular POV" .: I disagree with your sys-admins.
I believe the restart decision should remain in the hands of the owner.

How would you do this for non-systemd systems ?

Isn't that obvious?  systemd unit files are for systemd.  Non-systemd
systems doesn't have systemd unit files, thus there is very little we
can do about them.

Consistent behaviour is clearly *not* your primary concern for OpenVPN.
^Take Note^

I disagree with making this change to the default
openvpn-server@.service unit file.

Your opposition have been noted.

Yeah right ..

If you really want to include them then how about:

Either:
   openvpn-server@.service (responsible for start/stop etc actions)
   openvpn-server-auto-restart@.service (speaks for itself)

NAK.  This is not how the design around systemd unit files is intended
to be used.  

"design around systemd" .. Not OpenVPN

So what does Mr. https://github.com/poettering recommend ?

I prefer my "broken" servers to Fail outright
regardless of what Pot. or you think.

> Plus: it already exists a Debian bug ticket where there are
> comments about us adding 2 more unit files.  If adding even more, I can
> already sense the heat increasing on that ticket.

Thus .. see Next

Or rather
   include extra .service files in ./contrib. as samples or such.

NAK.  I rather have a document 

Another badly (if at all) maintained OpenVPN document ..

and lets face facts here .. this is *systemd* not "$(I deleted what I 
wanted to say .. maybe you can guess)" or some such.

simply describing how to change the
defaults using 'systemctl edit'.  Which is exactly how systemd is
designed to be used.  But we should have a baseline of recommended
defaults, and sys-admins can choose to opt-out of these defaults through
standard mechanisms, not by adding complexity through more unit files to
scan through.

Auto Restart on Failure for OpenVPN is *not* a recommended default
other than by you and I stand against it.

Just image a system 

Must I   ?  To what `end`  ?

which actively uses both openvpn-server@ and
openvpn-server-autorestart@.  Unless we also split up
/etc/openvpn/server ... it will be even more confusing when
investigating a server in 2 years why something is misbehaving.  "Did
this config run through this or that unit file?".  openvpn-server@ is
clear and specific, it handles server configurations.  Period.

Thus it should *not* be the decision of an openvpn developer to decide
if "somebody else's" server should behave like yours ..
IE: Forced Restart .. By default.

*NAK*

If you want a specific configuration or all openvpn-server@ OpenVPN
configurations to behave differently from the recommended defaults, then
you do that through 'systemctl edit', where it is very visible if this
specific configuration have some additional tweaks not - through

I know enough about systemd thanks.

I reject the idea that *all* servers under the control of /systemd/
should _inherit_ an almost forced restart due to *your* preference.

'systemctl status'.  This way sys-admins won't have remember or research
which 'sub-unit file' of openvpn-server@ to achieve a specific behaviour.

"sys-admins won't have to remember" .. word fail me.

People have a job to do ..

My final note:

1. As soon as I read this patch I was against it ..
2. I understand that I am not the most educated of Linux users
3. This "patch" has nothing to to with the function of OpenVPN
   therefore it should remain that way.

regards,

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] systemd: Enable systemd's auto-restart feature for server profiles

2017-09-07 Thread David Sommerseth

On 07/09/17 23:02, fragmentux wrote:
i,
> 
> all your comment are totally valid from a sys-admin point of view but
> from an openvpn POV, the only responsibility is to provide a secure VPN.
> 
> Use all of systemd's functions to maximize openvpn's process *security*
> But *forcing* restart as an almost unconditional default is nonsense.

We are in the position to promote sane and good defaults.  This
behaviour is considered sane and good by many sys-admins.  So when these
two view-points intersects, I see no harm of us actually promoting this
change.

> How would you do this for non-systemd systems ?

Isn't that obvious?  systemd unit files are for systemd.  Non-systemd
systems doesn't have systemd unit files, thus there is very little we
can do about them.

> I disagree with making this change to the default
> openvpn-server@.service unit file.

Your opposition have been noted.

> If you really want to include them then how about:
> 
> Either:
>   openvpn-server@.service (responsible for start/stop etc actions)
>   openvpn-server-auto-restart@.service (speaks for itself)

NAK.  This is not how the design around systemd unit files is intended
to be used.  Plus: it already exists a Debian bug ticket where there are
comments about us adding 2 more unit files.  If adding even more, I can
already sense the heat increasing on that ticket.

> Or rather
>   include extra .service files in ./contrib. as samples or such.

NAK.  I rather have a document simply describing how to change the
defaults using 'systemctl edit'.  Which is exactly how systemd is
designed to be used.  But we should have a baseline of recommended
defaults, and sys-admins can choose to opt-out of these defaults through
standard mechanisms, not by adding complexity through more unit files to
scan through.

Just image a system which actively uses both openvpn-server@ and
openvpn-server-autorestart@.  Unless we also split up
/etc/openvpn/server ... it will be even more confusing when
investigating a server in 2 years why something is misbehaving.  "Did
this config run through this or that unit file?".  openvpn-server@ is
clear and specific, it handles server configurations.  Period.

If you want a specific configuration or all openvpn-server@ OpenVPN
configurations to behave differently from the recommended defaults, then
you do that through 'systemctl edit', where it is very visible if this
specific configuration have some additional tweaks not - through
'systemctl status'.  This way sys-admins won't have remember or research
which 'sub-unit file' of openvpn-server@ to achieve a specific behaviour.

-- 
kind regards,

David Sommerseth
OpenVPN Technologies, Inc

signature.asc
Description: OpenPGP digital signature
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] systemd: Enable systemd's auto-restart feature for server profiles

2017-09-07 Thread fragmentux


Hi,

all your comment are totally valid from a sys-admin point of view but
from an openvpn POV, the only responsibility is to provide a secure VPN.

Use all of systemd's functions to maximize openvpn's process *security*
But *forcing* restart as an almost unconditional default is nonsense.
How would you do this for non-systemd systems ?

I disagree with making this change to the default
openvpn-server@.service unit file.

If you really want to include them then how about:

Either:
  openvpn-server@.service (responsible for start/stop etc actions)
  openvpn-server-auto-restart@.service (speaks for itself)

Or rather
  include extra .service files in ./contrib. as samples or such.

regards,

On 07/09/17 16:46, David Sommerseth wrote:

On 07/09/17 17:08, fragmentux wrote:

Hi,

On 07/09/17 00:52, David Sommerseth wrote:

Systemd supervises services it has started and can act upon unexpected
scenarios.  This change will restart OpenVPN after 5 seconds if the
OpenVPN
process exits unexpectedly.


Define "unexpectedly"
(my2c: something needs to be fixed)


Yes, that _could_ be, but is not for sure.  It is hard to define
"unexpectedly".  Or to flip it around, if we could define "unexpectedly"
properly, there is nothing unexpected happening.

But _some_ scenarios

  - The out-of-memory killer picks an openvpn process
Fix: Add more memory, reduce the number of memory hungry processes,
usually nothing OpenVPN can do anything with

  - The server process segfaults dies (segfault)
This happens most commonly due to poor/badly written plug-ins
Fix: Get an updated/fixed plug-in, not something OpenVPN can
fully control.

  - A fatal error happened
Which can have many reasons (grep M_FATAL in the source), most
of them it is safe to restart the OpenVPN process.  It could be
caused by the crypto layer not getting enough random data, it
could be a malicious attempt of an attack, triggering some of
the ASSERT() checks in the code.  M_FATAL in general is more
of the error type "It is not safe to continue running this process"
but it doesn't mean it isn't safe to restart the process.  Again,
situations outside OpenVPN influences its ability to run.

And there's a plethora of other scenarios too.

So to define "unexpectedly" is not far away from trying to define "life".



The on-failure mode is the recommended mode by upstream systemd.


IE:
This is the way "upstream systemd" recommend to use systemd
*if*  "some failed process"  is to be restarted.

(my2c: A decision better left to the sys-admin not openvpn developer)


Why is it bad to have this behaviour being the default?  The sys-admin
still have the ability to override this.

And even though I have the role of developer here in the OpenVPN
community, outside of my day-work role I do also have the role as a
sys-admin.  From my point of view, this isn't a "this sounds like a cool
feature".  It is actually something I have enabled on systems manually
for some time, so if my smaller scale environment finds this useful -
why wouldn't others find it useful?  And also having had sys-admin roles
in former work life, this is something we deployed on many services in
far larger environments.


This change have been tested on a test server for some month, and it
works indeed as intended when provoking the OpenVPN process to stop.


Define "provoking"
(my2c: not unexpectedly at all, nothing to fix here)


Like adding a plug-in which triggers a segfault on the first connection
attempt.  Or sending a SIGKILL to the process.  This was needed to test
and verify that the restart mechanism did work.  Otherwise I would not
have had confidence in believing this was done correctly.  And even
though that my triggering isn't unexpected, it is unexpected by the
OpenVPN process.

But sure, if you have a way to trigger OpenVPN to unexpectedly failing,
we'd be delighted to hear about it  as that is most likely something
we should fix in OpenVPN ;-)

In case you haven't noticed, OpenVPN is quite stable and seldom fails by
itself.  But when it does, it is for most end-users depending on it a
nice feature that things do try to recover on its own.

These events do also not happen in complete silence.  They are logged.
And a responsible sys-admin will check logs at regular intervals, some
even have automated alerts happening.  But most sys-admins will be
pleased that they don't have to go into panic rescue mode instantly
because a VPN server went down and they have 50 users on the phone
complaining they can't reach the intranet.


Either way,
it is only a few lines of a systemd unit.service file
to be deleted/customised by the relevant sys-admin.


systemctl edit openvpn-server@CONFIGNAME
then add Restart=no

Which have the advantage of disabling this feature on selected
configurations, not all configs by default.  If you want the default, I
believe you just do: systemctl edit openvpn-server@

Re: [Openvpn-devel] [PATCH v2] lz4: Move towards a newer LZ4 API

2017-09-07 Thread Simon Matter

Hi,

While we are at it, I found it useful to see the used LZ4 version at
runtime as it is done with LZO and other libraries.

I've patched my rpms with the patch attached.

Regards,
Simon

> We are using a deprecated function, LZ4_compress_limitedOutput(), which
> will be removed with time.  The correct function to use is
> LZ4_compress_default().
> Both function takes the same number of arguments and data types, so the
> change
> is minimal.
>
> This patch will also enforce the system LZ4 library to be at least v1.7.1.
>  If
> the system library is not found or it is older, it will be build using the
> bundled
> LZ4 library.  The version number requirement is based on the LZ4 version
> we ship.
>
> The changes in configure.ac for the version check is modelled around the
> same
> approach we use for OpenSSL.  Plus it does a few minor reformats and
> improvements
> to comply with more recommend autoconf coding style.
>
> This patch is a result of the discussions in this mail thread:
> https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg14135.html
>
> Signed-off-by: David Sommerseth 
>
> ---
> v2 - Don't use LZ4 version based #ifdef wrapper function
>  Do the LZ4 version check in ./configure
> ---
>  configure.ac   | 72
> +++---
>  src/openvpn/comp-lz4.c |  3 ++-
>  2 files changed, 53 insertions(+), 22 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index 6f1044e8..74443353 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -1088,37 +1088,67 @@ dnl
>  AC_ARG_VAR([LZ4_CFLAGS], [C compiler flags for lz4])
>  AC_ARG_VAR([LZ4_LIBS], [linker flags for lz4])
>  if test "$enable_lz4" = "yes" && test "$enable_comp_stub" = "no"; then
> -AC_CHECKING([for LZ4 Library and Header files])
> -havelz4lib=1
> -
> -# if LZ4_LIBS is set, we assume it will work, otherwise test
> -if test -z "${LZ4_LIBS}"; then
> - AC_CHECK_LIB(lz4, LZ4_compress,
> - [ LZ4_LIBS="-llz4" ],
> - [
> - AC_MSG_RESULT([LZ4 library not found.])
> - havelz4lib=0
> - ])
> +if test -z "${LZ4_CFLAGS}" -a -z "${LZ4_LIBS}"; then
> + # if the user did not explicitly specify flags, try to autodetect
> + PKG_CHECK_MODULES([LZ4],
> +   [liblz4 >= 1.7.1],
> +   [have_lz4="yes"],
> +   [] # If this fails, we will do another test next
> + )
>  fi
>
>  saved_CFLAGS="${CFLAGS}"
> +saved_LIBS="${LIBS}"
>  CFLAGS="${CFLAGS} ${LZ4_CFLAGS}"
> -AC_CHECK_HEADERS(lz4.h,
> -   ,
> -   [
> -AC_MSG_RESULT([LZ4 headers not found.])
> -havelz4lib=0
> -   ])
> -
> -if test $havelz4lib = 0 ; then
> - AC_MSG_RESULT([LZ4 library or header not found, using version in
> src/compat/compat-lz4.*])
> +LIBS="${LIBS} ${LZ4_LIBS}"
> +
> +# If pkgconfig check failed or LZ4_CFLAGS/LZ4_LIBS env vars
> +# are used, check the version directly in the LZ4 include file
> +if test "${have_lz4}" != "yes"; then
> + AC_CHECK_HEADERS([lz4.h],
> +  [have_lz4h="yes"],
> +  [])
> +
> + if test "${have_lz4h}" = "yes" ; then
> + AC_MSG_CHECKING([additionally if system LZ4 version >= 1.7.1])
> + AC_COMPILE_IFELSE(
> + [AC_LANG_PROGRAM([[
> +#include 
> +  ]],
> +  [[
> +/* Version encoding: MMNNPP (Major miNor Patch) - see lz4.h for details
> */
> +#if LZ4_VERSION_NUMBER < 10701L
> +#error LZ4 is too old
> +#endif
> +  ]]
> + )],
> + [
> + AC_MSG_RESULT([ok])
> + have_lz4="yes"
> + ],
> + [AC_MSG_RESULT([system LZ4 library is too old])]
> + )
> + fi
> +fi
> +
> +# if LZ4_LIBS is set, we assume it will work, otherwise test
> +if test -z "${LZ4_LIBS}"; then
> + AC_CHECK_LIB([lz4],
> +  [LZ4_compress],
> +  [LZ4_LIBS="-llz4"],
> +  [have_lz4="no"])
> +fi
> +
> +if test "${have_lz4}" != "yes" ; then
> + AC_MSG_RESULT([ usuable LZ4 library or header not found, using 
> version
> in src/compat/compat-lz4.*])
>   AC_DEFINE([NEED_COMPAT_LZ4], [1], [use copy of LZ4 source in compat/])
>   LZ4_LIBS=""
>  fi
>  OPTIONAL_LZ4_CFLAGS="${LZ4_CFLAGS}"
>  OPTIONAL_LZ4_LIBS="${LZ4_LIBS}"
> -AC_DEFINE(ENABLE_LZ4, 1, [Enable LZ4 compression library])
> +AC_DEFINE(ENABLE_LZ4, [1], [Enable LZ4 compression library])
>  CFLAGS="${saved_CFLAGS}"
> +LIBS="${saved_LIBS}"
>  fi
>
>
> diff --git a/src/openvpn/comp-lz4.c b/src/openvpn/comp-lz4.c
> index e056caa8..bdb3247d 100644
> --- a/src/openvpn/comp-lz4.c
> +++ b/src/openvpn/comp-lz4.c
> @@ -43,6 +43,7 @@
>
>  #include "memdbg.h"
>
> +
>  static void
>  lz4_compress_init(struct

[Openvpn-devel] [PATCH v2] lz4: Move towards a newer LZ4 API

2017-09-07 Thread David Sommerseth

We are using a deprecated function, LZ4_compress_limitedOutput(), which
will be removed with time.  The correct function to use is 
LZ4_compress_default().
Both function takes the same number of arguments and data types, so the change
is minimal.

This patch will also enforce the system LZ4 library to be at least v1.7.1.  If
the system library is not found or it is older, it will be build using the 
bundled
LZ4 library.  The version number requirement is based on the LZ4 version we 
ship.

The changes in configure.ac for the version check is modelled around the same
approach we use for OpenSSL.  Plus it does a few minor reformats and 
improvements
to comply with more recommend autoconf coding style.

This patch is a result of the discussions in this mail thread:
https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg14135.html

Signed-off-by: David Sommerseth 

---
v2 - Don't use LZ4 version based #ifdef wrapper function
 Do the LZ4 version check in ./configure
---
 configure.ac   | 72 +++---
 src/openvpn/comp-lz4.c |  3 ++-
 2 files changed, 53 insertions(+), 22 deletions(-)

diff --git a/configure.ac b/configure.ac
index 6f1044e8..74443353 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1088,37 +1088,67 @@ dnl
 AC_ARG_VAR([LZ4_CFLAGS], [C compiler flags for lz4])
 AC_ARG_VAR([LZ4_LIBS], [linker flags for lz4])
 if test "$enable_lz4" = "yes" && test "$enable_comp_stub" = "no"; then
-AC_CHECKING([for LZ4 Library and Header files])
-havelz4lib=1
-
-# if LZ4_LIBS is set, we assume it will work, otherwise test
-if test -z "${LZ4_LIBS}"; then
-   AC_CHECK_LIB(lz4, LZ4_compress,
-   [ LZ4_LIBS="-llz4" ],
-   [
-   AC_MSG_RESULT([LZ4 library not found.])
-   havelz4lib=0
-   ])
+if test -z "${LZ4_CFLAGS}" -a -z "${LZ4_LIBS}"; then
+   # if the user did not explicitly specify flags, try to autodetect
+   PKG_CHECK_MODULES([LZ4],
+ [liblz4 >= 1.7.1],
+ [have_lz4="yes"],
+ [] # If this fails, we will do another test next
+   )
 fi
 
 saved_CFLAGS="${CFLAGS}"
+saved_LIBS="${LIBS}"
 CFLAGS="${CFLAGS} ${LZ4_CFLAGS}"
-AC_CHECK_HEADERS(lz4.h,
-   ,
-   [
-  AC_MSG_RESULT([LZ4 headers not found.])
-  havelz4lib=0
-   ])
-
-if test $havelz4lib = 0 ; then
-   AC_MSG_RESULT([LZ4 library or header not found, using version in 
src/compat/compat-lz4.*])
+LIBS="${LIBS} ${LZ4_LIBS}"
+
+# If pkgconfig check failed or LZ4_CFLAGS/LZ4_LIBS env vars
+# are used, check the version directly in the LZ4 include file
+if test "${have_lz4}" != "yes"; then
+   AC_CHECK_HEADERS([lz4.h],
+[have_lz4h="yes"],
+[])
+
+   if test "${have_lz4h}" = "yes" ; then
+   AC_MSG_CHECKING([additionally if system LZ4 version >= 1.7.1])
+   AC_COMPILE_IFELSE(
+   [AC_LANG_PROGRAM([[
+#include 
+]],
+[[
+/* Version encoding: MMNNPP (Major miNor Patch) - see lz4.h for details */
+#if LZ4_VERSION_NUMBER < 10701L
+#error LZ4 is too old
+#endif
+]]
+   )],
+   [
+   AC_MSG_RESULT([ok])
+   have_lz4="yes"
+   ],
+   [AC_MSG_RESULT([system LZ4 library is too old])]
+   )
+   fi
+fi
+
+# if LZ4_LIBS is set, we assume it will work, otherwise test
+if test -z "${LZ4_LIBS}"; then
+   AC_CHECK_LIB([lz4],
+[LZ4_compress],
+[LZ4_LIBS="-llz4"],
+[have_lz4="no"])
+fi
+
+if test "${have_lz4}" != "yes" ; then
+   AC_MSG_RESULT([ usuable LZ4 library or header not found, using 
version in src/compat/compat-lz4.*])
AC_DEFINE([NEED_COMPAT_LZ4], [1], [use copy of LZ4 source in compat/])
LZ4_LIBS=""
 fi
 OPTIONAL_LZ4_CFLAGS="${LZ4_CFLAGS}"
 OPTIONAL_LZ4_LIBS="${LZ4_LIBS}"
-AC_DEFINE(ENABLE_LZ4, 1, [Enable LZ4 compression library])
+AC_DEFINE(ENABLE_LZ4, [1], [Enable LZ4 compression library])
 CFLAGS="${saved_CFLAGS}"
+LIBS="${saved_LIBS}"
 fi
 
 
diff --git a/src/openvpn/comp-lz4.c b/src/openvpn/comp-lz4.c
index e056caa8..bdb3247d 100644
--- a/src/openvpn/comp-lz4.c
+++ b/src/openvpn/comp-lz4.c
@@ -43,6 +43,7 @@
 
 #include "memdbg.h"
 
+
 static void
 lz4_compress_init(struct compress_context *compctx)
 {
@@ -86,7 +87,7 @@ do_lz4_compress(struct buffer *buf,
 return false;
 }
 
-zlen = LZ4_compress_limitedOutput((const char *)BPTR(buf), (char 
*)BPTR(work), BLEN(buf), zlen_max );
+zlen = LZ4_compress_default((const char *)BPTR(buf), (char 
*)BPTR(work), BLEN(buf), zlen_max );
 
 if (zlen <= 0)
 {

Re: [Openvpn-devel] [PATCH] systemd: Enable systemd's auto-restart feature for server profiles

2017-09-07 Thread fragmentux


Hi,

On 07/09/17 00:52, David Sommerseth wrote:

Systemd supervises services it has started and can act upon unexpected
scenarios.  This change will restart OpenVPN after 5 seconds if the OpenVPN
process exits unexpectedly.


Define "unexpectedly"
(my2c: something needs to be fixed)


The on-failure mode is the recommended mode by upstream systemd.


IE:
This is the way "upstream systemd" recommend to use systemd
*if*  "some failed process"  is to be restarted.

(my2c: A decision better left to the sys-admin not openvpn developer)



This change have been tested on a test server for some month, and it
works indeed as intended when provoking the OpenVPN process to stop.


Define "provoking"
(my2c: not unexpectedly at all, nothing to fix here)


Either way,
it is only a few lines of a systemd unit.service file
to be deleted/customised by the relevant sys-admin.

Regards,

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] lz4: Changing how LZ4 library handling is done

2017-09-07 Thread David Sommerseth

On 07/09/17 16:06, David Sommerseth wrote:
> On 07/09/17 08:12, Gert Doering wrote:
>> Hi,
>>
>> On Thu, Sep 07, 2017 at 03:22:25AM +0200, David Sommerseth wrote:
>>> This change will expect the system to have LZ4 libraries and headers
>>> installed by default.  We still carry a bundled LZ4 library, which
>>> must now be explicitly enabled through providing --enable-bundled-lz4
>>> to ./configure.  Otherwise, as before, --disable-lz4 will completely
>>> remove any LZ4 support.
>>
>> I'm totally missing the *reason* why you want to change this
> 
> Bundled libraries are considered to be bad, as it requires active
> maintenance.  Just look at which version we ship in OpenVPN (1.6.0) and

Just a correction.  We bundle lz4-1.7.1, not 1.6.0.  That was a
left-over from my testing of the API update patch.  But that doesn't
change my argument that much.

-- 
kind regards,

David Sommerseth
OpenVPN Technologies, Inc




signature.asc
Description: OpenPGP digital signature
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] systemd: Enable systemd's auto-restart feature for server profiles

2017-09-07 Thread David Sommerseth

On 07/09/17 08:16, Gert Doering wrote:
> 
> Restarting is good, but if there is something faulty that leads to
> "the process always dies right away", this can lead to very quickly
> filling disks with not-so-useful logging...
Oh, I overlooked this one.  Just one comment in regards to the "filling
disks".  That naturally depends on how logging is configured on the
system, and that varies a lot.  But my experience based on defaults in
the environments I use:

* systemd journal have some reasonably sane defaults to avoid this to
  happen; IIRC it defaults to rotate the journal when reaching 10-15% of
  available disk space *or* 4GB of log data.

* RHEL (and clones) usually have rsyslog installed too, which the
  journal forwards log data too.  And it most commonly it also have
  logrotate installed too (at least on the server variant) which runs
  on a regular basis.  But it also depends on how big the partition
   where /var/log resides is.

So the risks for such restarts to cause full disks should be fairly
minimal.  And for those who have enabled remote logging, that is often
to pay more attention to log events, so then such scenarios would
probably be detected even quicker.

-- 
kind regards,

David Sommerseth
OpenVPN Technologies, Inc

signature.asc
Description: OpenPGP digital signature
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] systemd: Enable systemd's auto-restart feature for server profiles

2017-09-07 Thread Samuli Seppänen

On 07/09/2017 16:00, David Sommerseth wrote:
> On 07/09/17 10:04, Samuli Seppänen wrote:
>> On 07/09/2017 10:16, Samuli Seppänen wrote:
>>> On 07/09/2017 09:16, Gert Doering wrote:
 Hi,

 On Thu, Sep 07, 2017 at 01:52:02AM +0200, David Sommerseth wrote:
> @@ -18,6 +18,8 @@ DeviceAllow=/dev/net/tun rw
>  ProtectSystem=true
>  ProtectHome=true
>  KillMode=process
> +RestartSec=5s
> +Restart=on-failure

 Is there a way to get exponential backoff on restart?

 Restarting is good, but if there is something faulty that leads to
 "the process always dies right away", this can lead to very quickly
 filling disks with not-so-useful logging...

 (Otherwise, yes, restarting is good :-) )

>>>
>>> Hi,
>>>
>>> From systemd.unit man-page[1]:
>>>
>>>   StartLimitIntervalSec=, StartLimitBurst=
>>>
>>> Configure unit start rate limiting. By default, units which are
>>> started more than 5 times within 10 seconds are not permitted to
>>> start any more times until the 10 second interval ends.
>>>
>>> I verified this behavior on CentOS 7 using another daemon (monit) by
>>> setting "Restart=on-failure" for it, breaking its config file and
>>> forcibly killing it. Note that RestartSec is the default, i.e. 100ms:
>>>
>>> ---
>>>
>>> Sep 07 09:55:37 centos-7 systemd[1]: monit.service: control process
>>> exited, code=exited status=1
>>>
>>> Sep 07 09:55:37 centos-7 systemd[1]: Unit monit.service entered failed
>>> state.
>>>
>>> Sep 07 09:55:37 centos-7 systemd[1]: monit.service holdoff time over,
>>> scheduling restart.
>>>
>>> Sep 07 09:55:37 centos-7 systemd[1]: Stopping Pro-active monitoring
>>> utility for unix systems...
>>>
>>> Sep 07 09:55:37 centos-7 systemd[1]: Starting Pro-active monitoring
>>> utility for unix systems...
>>>
>>> Sep 07 09:55:37 centos-7 systemd[1]: monit.service start request
>>> repeated too quickly, refusing to start.
>>>
>>> Sep 07 09:55:37 centos-7 systemd[1]: Failed to start Pro-active
>>> monitoring utility for unix systems.
>>>
>>> Sep 07 09:55:37 centos-7 systemd[1]: Unit monit.service entered failed
>>> state.
>>>
>>> ---
>>>
>>> As you can see, systemd quickly realizes that monit will not come back
>>> up and stops trying.
>>>
>>> However, when I added "RestartSec=5s" the StartLimit* thresholds were
>>> never triggered. This meant that systemd never ceased trying to restart
>>> the monit service.
>>>
>>> David: any particular reason why you added RestartSec? Why not just let
>>> it be the default (100ms)?
> 
> Partly to avoid respawning too fast.  We don't know what kind of
> additional plug-ins or management interface tools have been integrated
> and how they react if OpenVPN goes into a tight restart-loop.  And
> partly to _escape_ the stopping of restarts.
> 
> I'm not sure I'm easily buying into the "faulty configuration" argument.

There is no "faulty configuration argument". I broke monit's
configuration file intentionally for the sole purpose of testing
systemd's restart functionality.

>  Because these restart scenarios mostly happens when a sys-admin is
> _not_ around.  If you're playing with configurations, do a restart to
> test the new config, you are around to handle a failed situation.  The
> only area where this can fail is when we break options and OpenVPN get
> updated automatically.  But in both these scenarios, a restart delay of
> 5 seconds won't cause too much stress on the system.
> 
> I also opted for not much longer delay, as if a restart happens
> successfully, most users won't notice that too much.  If it is 30
> seconds or 1 minute, that is much more noticeable.  But I'm open for
> adjusting this too.

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] lz4: Changing how LZ4 library handling is done

2017-09-07 Thread David Sommerseth

On 07/09/17 08:12, Gert Doering wrote:
> Hi,
> 
> On Thu, Sep 07, 2017 at 03:22:25AM +0200, David Sommerseth wrote:
>> This change will expect the system to have LZ4 libraries and headers
>> installed by default.  We still carry a bundled LZ4 library, which
>> must now be explicitly enabled through providing --enable-bundled-lz4
>> to ./configure.  Otherwise, as before, --disable-lz4 will completely
>> remove any LZ4 support.
> 
> I'm totally missing the *reason* why you want to change this

Bundled libraries are considered to be bad, as it requires active
maintenance.  Just look at which version we ship in OpenVPN (1.6.0) and
we've had two rebase patches on the -devel ML which have been ignored,
first updating to v1.7.4.2 [0] and then v1.7.5 [1].  I have not bothered
yet to send a new rebase request to the latest v1.8.0 [2].  That is an
awfully bad track record.

[0] Dec 15, 2016

[1] Feb 21, 2017

[2] Aug 17, 2017 

If we even manage to miss that there are CVE fixes in the LZ4 library,
then we've in even bigger trouble.  Especially if we're so bad at
updating just the maintenance releases.

Yes, I can already hear you say that updates caused by CVEs will be
handled more promptly.  But from my point of view, that's just words.
We do not have any processes for handling bundled libraries nor response
times nor who and how updates in bundled libraries are tracked.

> We don't have --enable-bundled-compat flags for the rest of the things
> in compat/ either, don't we?

Somehow I expected that argument from you, Gert.  But that is truly
comparing apples and oranges.

LZ4 is a large library compared to the compat functions we do provide:

  1605  compat-basename.c
  2208  compat-inet_ntop.c
  2261  compat-inet_pton.c
  2353  compat-daemon.c
  3210  compat-gettimeofday.c
  4057  compat-dirname.c
 49760  compat-lz4.c

The code complexity of compat-dirname.c (which is the second largest
compat code) is negligible compared to compat-lz4.c  So the risk of a
security issue should be considerably lower in these other functions we add.

Secondly, we only use our compat-*.c functions if the underlying system
does not carry those features - most commonly, these are libc related
functions.  So if a systems libc does not carry a function we need, we
need to have a wrapper for a specific function.  And strictly speaking,
LZ4 is not a requirement for OpenVPN to function as an application.

> Also, I can't see consensus that "remove the bundled lz4" is the way to 
> go - this was on the plate for the hackathon to discuss.  You and Antonio
> are convinced that this is a good way forward, applying a very specific 
> Linux-distro-based mindset to it ("missing libraries are a problem of the
> package builder, why should we care?") - please listen to my arguments: there
> are people out there that build OpenVPN from source (tarball or git), and
> they are looking at library dependencies with a slightly different view.

Hence the --enable-bundled-lz4.  If you do not have or want to build LZ4
yourself first, then you can use our bundled LZ4 - with the risks that
implies.  But this is done explicitly, so a developer or package
maintainer is forced to take a decision here first.

But equally important, if anyone is going to build OpenVPN from source,
what is the chances that they will not be able to build LZ4 from source
before building OpenVPN?

And we already have several other external libraries we depend on which
we do not carry a compat-* version for ... LZO being the most obvious
one, as it is a 1:1 comparison to LZ4.  Then there is openssl/mbedtls.
But we do have feature specific dependencies  pkcs11-helper, p11kit,
which is available on all our supported platforms.  Then there is libpam
(for *nix).  And Linux can add libselinux and libsystemd into the mix as
well.

This patch actually aligns LZ4 to be treated more equally to LZO, with
the distinction that we do have --enable-bundled-lz4 - at least for some
time forward.

And some more arguments why bundled libraries are bad, here even from a
FreeBSD perspective:

A couple of other with more generic perspectives:

And the perspective from a few Linux distros:

-- 
kind regards,

David Sommerseth
OpenVPN Technologies, Inc

signature.asc
Description: OpenPGP digital signature
--
Check

Re: [Openvpn-devel] [PATCH] lz4: Move towards a newer LZ4 API

2017-09-07 Thread David Sommerseth

On 07/09/17 08:13, Gert Doering wrote:> HI,
> 
> On Thu, Sep 07, 2017 at 04:28:27AM +0200, David Sommerseth wrote:
>> We are using a deprecated function, LZ4_compress_limitedOutput(), which
>> will be removed with time.  The correct function to use is 
>> LZ4_compress_default().
>> Both function takes the same number of arguments and data types, so the 
>> change
>> is minimal.
> 
> I wonder why we should bother to have a wrapper function here.
> 
> We can ship a lz4 library that has the new function, and if a system only
> provides an older version, declare it unsuitable (configure check) and
> use the bundled one.

Yeah, I was thinking along those lines too when working on this patch.
I just remembered vaguely our IRC chat long time a go and looked back at
the initial mail discussion, and there was a preference for the wrapping
back then.

But I like much more that we have a defined LZ4 version which we
support, and ditch the #ifdef'ed wrapper.  I'll send a v2 soon.


-- 
kind regards,

David Sommerseth
OpenVPN Technologies, Inc




signature.asc
Description: OpenPGP digital signature
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] systemd: Enable systemd's auto-restart feature for server profiles

2017-09-07 Thread David Sommerseth

On 07/09/17 14:17, Samuli Seppänen wrote:
> On 07/09/2017 11:13, Gert Doering wrote:
>> Hi,
>>
>> On Thu, Sep 07, 2017 at 11:04:01AM +0300, Samuli Seppänen wrote:
>>> "Note that units which are configured for Restart= and which reach the
>>> start limit are not attempted to be restarted anymore; however, they may
>>> still be restarted manually at a later point, from which point on, the
>>> restart logic is again activated."
>>
>> Which is not what I hoped for... "turn it off and leave it so" is non
>> helpful (it might be a transient error preventing the startup).
>>
> 
> Good point. Systemd seems to be able to adjust its restart behavior
> depending on exit code of the main service process (i.e. OpenVPN) using
> "RestartForceExitStatus" and "RsstartPreventExitStatus"[1]. Perhaps
> these could be helpful in our case...

Okay, lets try to align what OpenVPN does in various scenarios and
compare it how we want restarts to happen.

First, have a look at the man page:

Then realise that OpenVPN (AFAIK) only operates with two exit codes:

#define OPENVPN_EXIT_STATUS_GOOD0
#define OPENVPN_EXIT_STATUS_ERROR   1
#define OPENVPN_EXIT_STATUS_USAGE   1
#define OPENVPN_EXIT_STATUS_CANNOT_OPEN_DEBUG_FILE  1
(from error.h)

So the RestartForceExitStatus/RestartPreventExitStatus is not going to
be helpful if all graceful errors results in 1, which is the most common
way OpenVPN stops - through the M_FATAL.  So that leaves us with
SIGSEGV, SIGABRT and similar unclean exit signals.

To avoid restarts on faulty configurations or if we can define scenarios
where we do not want OpenVPN to be restarted automatically, we need to
introduce more exit codes.  This way we can implicitly tell systemd if
it should restart OpenVPN or not.

-- 
kind regards,

David Sommerseth
OpenVPN Technologies, Inc

signature.asc
Description: OpenPGP digital signature
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] systemd: Enable systemd's auto-restart feature for server profiles

2017-09-07 Thread David Sommerseth

On 07/09/17 15:07, Gert Doering wrote:
> Hi,
> 
> On Thu, Sep 07, 2017 at 03:02:20PM +0200, David Sommerseth wrote:
>>> Which is not what I hoped for... "turn it off and leave it so" is non
>>> helpful (it might be a transient error preventing the startup).
>>
>> I'm confused.  What is it you want?
>>
>> * try restarting in an endless loop?
>> * try restarting X times and then stop trying?
> 
> "exponential decay"
> 
> Restart a few times quickly, and then slow down, like "re-try after 1s, 3s,
> 5s, 10s, 15s, 30s, 60s, ... and then stick to every-5-minutes" or so
> (like OpenVPN's own connection re-try logic :-) ).   

Yes, that is a nice idea.  But to my knowledge, that is not something
systemd supports today.  So we can't have that easily.  We could look
into a ExcePreStart=, which tracks how long time it has gone since the
last restart and adds the "exponential decay" sleep.  But that means
more code to maintain.

> That way you get quick restart if a temporary failure happens, and avoid 
> filling logs with useless crap if the error persists - like "bind to an
> IP address that is not present in the system, because the ppp0 interface
> where it would show up is down", or something like this.
> 
> But if that cannot be done, that's how it is - and in that case, "stop
> trying" is not what I want.
Which means the current patch is what is possible to achieve today.  :)


-- 
kind regards,

David Sommerseth
OpenVPN Technologies, Inc




signature.asc
Description: OpenPGP digital signature
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] systemd: Enable systemd's auto-restart feature for server profiles

2017-09-07 Thread Gert Doering

Hi,

On Thu, Sep 07, 2017 at 03:02:20PM +0200, David Sommerseth wrote:
> > Which is not what I hoped for... "turn it off and leave it so" is non
> > helpful (it might be a transient error preventing the startup).
> 
> I'm confused.  What is it you want?
> 
> * try restarting in an endless loop?
> * try restarting X times and then stop trying?

"exponential decay"

Restart a few times quickly, and then slow down, like "re-try after 1s, 3s,
5s, 10s, 15s, 30s, 60s, ... and then stick to every-5-minutes" or so
(like OpenVPN's own connection re-try logic :-) ).   

That way you get quick restart if a temporary failure happens, and avoid 
filling logs with useless crap if the error persists - like "bind to an
IP address that is not present in the system, because the ppp0 interface
where it would show up is down", or something like this.

But if that cannot be done, that's how it is - and in that case, "stop
trying" is not what I want.

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

signature.asc
Description: PGP signature
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] route: cleanup codestyle and make code more readable

2017-09-07 Thread Antonio Quartulli

Hi,

On 07/09/17 04:45, David Sommerseth wrote:
> On 23/08/17 07:30, Antonio Quartulli wrote:
>> This patch does not introduce any functional change.
>>
>> The code in route.c seems to have been written in different
>> periods by different people, without sticking to a clear
>> codestyle. For this reason the code in this file in not
>> consistent at all.
>>
>> Clean it up by:
>> - removing spaces from function invocations
>> - cutting line longer than 80 chars (where possible)
>> - moving function arguments on the same line when there is enough space
>> - adding empty line between var declarations and code
>> - adding empty line between code and final return
>> - adding empty line to make the code less sticky and easier to parse
>>
>> Signed-off-by: Antonio Quartulli 
>> ---
>>
>> Yes, this is a quite big patch. However, since we are planning  a big
>> restructuring of the route.c file, it is better to take care of the
>> style in a separate patch (this) so that later we don't need to mixup 
>> cleanups
>> and refactoring.
>>
>> Note that this patch is based on master plus the following patches:
>>
>> - ensure function declarations are compiled with their definitions
>> - fix a couple of typ0s in comments and strings
>> - route: avoid definition of unused variables in certain configurations
>> - convert *_inline attributes to bool
>> - reformatting: fix style in crypto*.{c, h}
>> - Allow learning iroutes with network made up of all 0s (only if netbits < 8)
>> - ifconfig-ipv6(-push): allow using hostnames
>>
>>
>> Applying this patch without the above, might lead to screams,
>> natural disasters and endless nightmares.
> 
> I got it applying quite nicely (working my way through more patches
> now).  And yes, I like that we clean up the coding style further in this
> file.  But unfortunately, I'll have to say NAK in this round.
> 

I am not sure that what you get by applying this patch without the rest
is 100% correct. But I can rebase this patch on top of master if you are
willing to work on this one before the others mentioned in the patch
message.

> - Many places you replace spaces with tabs.

I must have messed up my vim style file recently. will recheck. Thanks!

> - There are several scenarios where our uncrustify config actually
>   improves your patch further (see the attachment).

Oh ok. I never thought about re-running uncrustify. Thanks for the
suggestion.

> - And the contradictions like the ones below
> 
>> -static void delete_route(struct route_ipv4 *r, const struct tuntap *tt, 
>> unsigned int flags, const struct route_gateway_info *rgi, const struct 
>> env_set *es);
>> +static void delete_route(struct route_ipv4 *r, const struct tuntap *tt,
>> + unsigned int flags,
>> + const struct route_gateway_info *rgi,
>> + const struct env_set *es);
> 
> vs
> 
>>  static void
>> -delete_route(struct route_ipv4 *r,
>> - const struct tuntap *tt,
>> - unsigned int flags,
>> - const struct route_gateway_info *rgi,
>> - const struct env_set *es)
>> +delete_route(struct route_ipv4 *r, const struct tuntap *tt, unsigned int 
>> flags,
>> + const struct route_gateway_info *rgi, const struct env_set *es)
> 
> I think the change you do in the former one is also more readable than
> squeezing everything into as few lines as possible, especially when
> there's lots of arguments.

Honestly I only applied the formal rule "a line should not be longer
than 80 chars". This is what I grasped from our codestyle. Is this not
correct?

In the first case less arguments fit on the first line because of the
return type. In the second case there is more space available.

This is a contradiction given by the codingstyle itself.

Should we use another rule to decide how function declarations and
definitions should look like? I think in the rest of the code we tried
to apply the line length rule too.

Let me know how this should be arranged otherwise (and let's write it in
the codingstyle page) :)

> 
> Our uncrustify config doesn't touch these details of function
> declarations, as tun.c and route.c is fairly extreme in variations here.
>  So we let that pass on the reformatting patch before the v2.4 release,
> to take care of them manually, as we didn't spend much extra time
> looking at more tweaks for uncrustify to make the result readable.  But
> I'm not sure we documented our preferences on function declarations, I
> don't recall that now.

I couldn't find anything about it in [1]

> 
> Even though we are not united in the use of uncrustify after the
> reformatting patches we did in December, I think it makes sense to at
> least double check what uncrustify would change and consider those.  The
> lesser the gap is to that result, the easier it will be to have a
> consistent coding style over the complete code base.

Do you think it might be reasonable to document all the rules in the

Re: [Openvpn-devel] [PATCH] systemd: Enable systemd's auto-restart feature for server profiles

2017-09-07 Thread David Sommerseth

On 07/09/17 10:13, Gert Doering wrote:
> Hi,
> 
> On Thu, Sep 07, 2017 at 11:04:01AM +0300, Samuli Seppänen wrote:
>> "Note that units which are configured for Restart= and which reach the
>> start limit are not attempted to be restarted anymore; however, they may
>> still be restarted manually at a later point, from which point on, the
>> restart logic is again activated."
> 
> Which is not what I hoped for... "turn it off and leave it so" is non
> helpful (it might be a transient error preventing the startup).

I'm confused.  What is it you want?

* try restarting in an endless loop?
* try restarting X times and then stop trying?


-- 
kind regards,

David Sommerseth
OpenVPN Technologies, Inc




signature.asc
Description: OpenPGP digital signature
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] systemd: Enable systemd's auto-restart feature for server profiles

2017-09-07 Thread David Sommerseth

On 07/09/17 10:04, Samuli Seppänen wrote:
> On 07/09/2017 10:16, Samuli Seppänen wrote:
>> On 07/09/2017 09:16, Gert Doering wrote:
>>> Hi,
>>>
>>> On Thu, Sep 07, 2017 at 01:52:02AM +0200, David Sommerseth wrote:
 @@ -18,6 +18,8 @@ DeviceAllow=/dev/net/tun rw
  ProtectSystem=true
  ProtectHome=true
  KillMode=process
 +RestartSec=5s
 +Restart=on-failure
>>>
>>> Is there a way to get exponential backoff on restart?
>>>
>>> Restarting is good, but if there is something faulty that leads to
>>> "the process always dies right away", this can lead to very quickly
>>> filling disks with not-so-useful logging...
>>>
>>> (Otherwise, yes, restarting is good :-) )
>>>
>>
>> Hi,
>>
>> From systemd.unit man-page[1]:
>>
>>   StartLimitIntervalSec=, StartLimitBurst=
>>
>> Configure unit start rate limiting. By default, units which are
>> started more than 5 times within 10 seconds are not permitted to
>> start any more times until the 10 second interval ends.
>>
>> I verified this behavior on CentOS 7 using another daemon (monit) by
>> setting "Restart=on-failure" for it, breaking its config file and
>> forcibly killing it. Note that RestartSec is the default, i.e. 100ms:
>>
>> ---
>>
>> Sep 07 09:55:37 centos-7 systemd[1]: monit.service: control process
>> exited, code=exited status=1
>>
>> Sep 07 09:55:37 centos-7 systemd[1]: Unit monit.service entered failed
>> state.
>>
>> Sep 07 09:55:37 centos-7 systemd[1]: monit.service holdoff time over,
>> scheduling restart.
>>
>> Sep 07 09:55:37 centos-7 systemd[1]: Stopping Pro-active monitoring
>> utility for unix systems...
>>
>> Sep 07 09:55:37 centos-7 systemd[1]: Starting Pro-active monitoring
>> utility for unix systems...
>>
>> Sep 07 09:55:37 centos-7 systemd[1]: monit.service start request
>> repeated too quickly, refusing to start.
>>
>> Sep 07 09:55:37 centos-7 systemd[1]: Failed to start Pro-active
>> monitoring utility for unix systems.
>>
>> Sep 07 09:55:37 centos-7 systemd[1]: Unit monit.service entered failed
>> state.
>>
>> ---
>>
>> As you can see, systemd quickly realizes that monit will not come back
>> up and stops trying.
>>
>> However, when I added "RestartSec=5s" the StartLimit* thresholds were
>> never triggered. This meant that systemd never ceased trying to restart
>> the monit service.
>>
>> David: any particular reason why you added RestartSec? Why not just let
>> it be the default (100ms)?

Partly to avoid respawning too fast.  We don't know what kind of
additional plug-ins or management interface tools have been integrated
and how they react if OpenVPN goes into a tight restart-loop.  And
partly to _escape_ the stopping of restarts.

I'm not sure I'm easily buying into the "faulty configuration" argument.
 Because these restart scenarios mostly happens when a sys-admin is
_not_ around.  If you're playing with configurations, do a restart to
test the new config, you are around to handle a failed situation.  The
only area where this can fail is when we break options and OpenVPN get
updated automatically.  But in both these scenarios, a restart delay of
5 seconds won't cause too much stress on the system.

I also opted for not much longer delay, as if a restart happens
successfully, most users won't notice that too much.  If it is 30
seconds or 1 minute, that is much more noticeable.  But I'm open for
adjusting this too.


-- 
kind regards,

David Sommerseth
OpenVPN Technologies, Inc




signature.asc
Description: OpenPGP digital signature
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] systemd: Enable systemd's auto-restart feature for server profiles

2017-09-07 Thread Samuli Seppänen

On 07/09/2017 11:13, Gert Doering wrote:
> Hi,
> 
> On Thu, Sep 07, 2017 at 11:04:01AM +0300, Samuli Seppänen wrote:
>> "Note that units which are configured for Restart= and which reach the
>> start limit are not attempted to be restarted anymore; however, they may
>> still be restarted manually at a later point, from which point on, the
>> restart logic is again activated."
> 
> Which is not what I hoped for... "turn it off and leave it so" is non
> helpful (it might be a transient error preventing the startup).
> 

Good point. Systemd seems to be able to adjust its restart behavior
depending on exit code of the main service process (i.e. OpenVPN) using
"RestartForceExitStatus" and "RsstartPreventExitStatus"[1]. Perhaps
these could be helpful in our case...

-- 
Samuli Seppänen
Community Manager
OpenVPN Technologies, Inc

irc freenode net: mattock

[1] 

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] tcp-server: ensure AF family is propagated to child context

2017-09-07 Thread Arne Schwabe

Am 07.09.17 um 11:55 schrieb Antonio Quartulli:
> From: Antonio Quartulli 
> 
> Commit 23d61c56 introduced the AF_UNSPEC socket family
> to be used when we don't know the actual one until the local
> socket binding is performed.
> 
> In such case AF_UNSPEC is stored in the `ce.af` member of
> the `c->options` object, indicating that the family has to be
> determined at runtime.
> 
> However, the determined value is never propagated back to the
> `options` object, which remains AF_UNSPEC and that is
> later used to initialize the TCP children contexts (UDP
> children contexts are unaffected).
> 
> This unexpected setting can trigger weird behaviours, like
> the one reported in ticket #933.
> In this case the value AF_UNSPEC in combination with the
> changes implemented in 2bed089d are leading to a TCP
> server quitting with M_FATAL upon client connection.
> 
> Note that the misbehaviour described in #933 can only be
> triggered when running a TCP server with mtu-disc set
> in the config (no matter the value).
> 
> Fix this inconsistency by always propagating the AF
> family from the top to the child context when running
> in TCP server mode.
>

ACK. That is a really weird corner case that I must have missed :)

Arne

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [PATCH] tcp-server: ensure AF family is propagated to child context

2017-09-07 Thread Antonio Quartulli

From: Antonio Quartulli 

Commit 23d61c56 introduced the AF_UNSPEC socket family
to be used when we don't know the actual one until the local
socket binding is performed.

In such case AF_UNSPEC is stored in the `ce.af` member of
the `c->options` object, indicating that the family has to be
determined at runtime.

However, the determined value is never propagated back to the
`options` object, which remains AF_UNSPEC and that is
later used to initialize the TCP children contexts (UDP
children contexts are unaffected).

This unexpected setting can trigger weird behaviours, like
the one reported in ticket #933.
In this case the value AF_UNSPEC in combination with the
changes implemented in 2bed089d are leading to a TCP
server quitting with M_FATAL upon client connection.

Note that the misbehaviour described in #933 can only be
triggered when running a TCP server with mtu-disc set
in the config (no matter the value).

Fix this inconsistency by always propagating the AF
family from the top to the child context when running
in TCP server mode.

As a direct consequence, this patch fixes Trac #933.

Signed-off-by: Antonio Quartulli 
---
 src/openvpn/socket.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c
index 3d4f881e..0fc91f21 100644
--- a/src/openvpn/socket.c
+++ b/src/openvpn/socket.c
@@ -1786,6 +1786,8 @@ link_socket_init_phase1(struct link_socket *sock,
 ASSERT(sock->info.proto == PROTO_TCP_SERVER);
 ASSERT(!sock->inetd);
 sock->sd = accept_from->sd;
+/* inherit (possibly guessed) info AF from parent context */
+sock->info.af = accept_from->info.af;
 }
 
 /* are we running in HTTP proxy mode? */
-- 
2.14.1


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] systemd: Enable systemd's auto-restart feature for server profiles

2017-09-07 Thread Gert Doering

Hi,

On Thu, Sep 07, 2017 at 11:04:01AM +0300, Samuli Seppänen wrote:
> "Note that units which are configured for Restart= and which reach the
> start limit are not attempted to be restarted anymore; however, they may
> still be restarted manually at a later point, from which point on, the
> restart logic is again activated."

Which is not what I hoped for... "turn it off and leave it so" is non
helpful (it might be a transient error preventing the startup).

Meh.

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


signature.asc
Description: PGP signature
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] systemd: Enable systemd's auto-restart feature for server profiles

2017-09-07 Thread Samuli Seppänen

On 07/09/2017 10:16, Samuli Seppänen wrote:
> On 07/09/2017 09:16, Gert Doering wrote:
>> Hi,
>>
>> On Thu, Sep 07, 2017 at 01:52:02AM +0200, David Sommerseth wrote:
>>> @@ -18,6 +18,8 @@ DeviceAllow=/dev/net/tun rw
>>>  ProtectSystem=true
>>>  ProtectHome=true
>>>  KillMode=process
>>> +RestartSec=5s
>>> +Restart=on-failure
>>
>> Is there a way to get exponential backoff on restart?
>>
>> Restarting is good, but if there is something faulty that leads to
>> "the process always dies right away", this can lead to very quickly
>> filling disks with not-so-useful logging...
>>
>> (Otherwise, yes, restarting is good :-) )
>>
> 
> Hi,
> 
> From systemd.unit man-page[1]:
> 
>   StartLimitIntervalSec=, StartLimitBurst=
> 
> Configure unit start rate limiting. By default, units which are
> started more than 5 times within 10 seconds are not permitted to
> start any more times until the 10 second interval ends.
> 
> I verified this behavior on CentOS 7 using another daemon (monit) by
> setting "Restart=on-failure" for it, breaking its config file and
> forcibly killing it. Note that RestartSec is the default, i.e. 100ms:
> 
> ---
> 
> Sep 07 09:55:37 centos-7 systemd[1]: monit.service: control process
> exited, code=exited status=1
> 
> Sep 07 09:55:37 centos-7 systemd[1]: Unit monit.service entered failed
> state.
> 
> Sep 07 09:55:37 centos-7 systemd[1]: monit.service holdoff time over,
> scheduling restart.
> 
> Sep 07 09:55:37 centos-7 systemd[1]: Stopping Pro-active monitoring
> utility for unix systems...
> 
> Sep 07 09:55:37 centos-7 systemd[1]: Starting Pro-active monitoring
> utility for unix systems...
> 
> Sep 07 09:55:37 centos-7 systemd[1]: monit.service start request
> repeated too quickly, refusing to start.
> 
> Sep 07 09:55:37 centos-7 systemd[1]: Failed to start Pro-active
> monitoring utility for unix systems.
> 
> Sep 07 09:55:37 centos-7 systemd[1]: Unit monit.service entered failed
> state.
> 
> ---
> 
> As you can see, systemd quickly realizes that monit will not come back
> up and stops trying.
> 
> However, when I added "RestartSec=5s" the StartLimit* thresholds were
> never triggered. This meant that systemd never ceased trying to restart
> the monit service.
> 
> David: any particular reason why you added RestartSec? Why not just let
> it be the default (100ms)?
> 

Actually the man-page quote above is a bit ambiguous. But later on the
man-page explain the logic pretty clearly:

"Note that units which are configured for Restart= and which reach the
start limit are not attempted to be restarted anymore; however, they may
still be restarted manually at a later point, from which point on, the
restart logic is again activated."

-- 
Samuli Seppänen
Community Manager
OpenVPN Technologies, Inc

irc freenode net: mattock

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] systemd: Enable systemd's auto-restart feature for server profiles

2017-09-07 Thread Samuli Seppänen

On 07/09/2017 09:16, Gert Doering wrote:
> Hi,
> 
> On Thu, Sep 07, 2017 at 01:52:02AM +0200, David Sommerseth wrote:
>> @@ -18,6 +18,8 @@ DeviceAllow=/dev/net/tun rw
>>  ProtectSystem=true
>>  ProtectHome=true
>>  KillMode=process
>> +RestartSec=5s
>> +Restart=on-failure
> 
> Is there a way to get exponential backoff on restart?
> 
> Restarting is good, but if there is something faulty that leads to
> "the process always dies right away", this can lead to very quickly
> filling disks with not-so-useful logging...
> 
> (Otherwise, yes, restarting is good :-) )
> 

Hi,

>From systemd.unit man-page[1]:

  StartLimitIntervalSec=, StartLimitBurst=

Configure unit start rate limiting. By default, units which are
started more than 5 times within 10 seconds are not permitted to
start any more times until the 10 second interval ends.

I verified this behavior on CentOS 7 using another daemon (monit) by
setting "Restart=on-failure" for it, breaking its config file and
forcibly killing it. Note that RestartSec is the default, i.e. 100ms:

---

Sep 07 09:55:37 centos-7 systemd[1]: monit.service: control process
exited, code=exited status=1

Sep 07 09:55:37 centos-7 systemd[1]: Unit monit.service entered failed
state.

Sep 07 09:55:37 centos-7 systemd[1]: monit.service holdoff time over,
scheduling restart.

Sep 07 09:55:37 centos-7 systemd[1]: Stopping Pro-active monitoring
utility for unix systems...

Sep 07 09:55:37 centos-7 systemd[1]: Starting Pro-active monitoring
utility for unix systems...

Sep 07 09:55:37 centos-7 systemd[1]: monit.service start request
repeated too quickly, refusing to start.

Sep 07 09:55:37 centos-7 systemd[1]: Failed to start Pro-active
monitoring utility for unix systems.

Sep 07 09:55:37 centos-7 systemd[1]: Unit monit.service entered failed
state.

---

As you can see, systemd quickly realizes that monit will not come back
up and stops trying.

However, when I added "RestartSec=5s" the StartLimit* thresholds were
never triggered. This meant that systemd never ceased trying to restart
the monit service.

David: any particular reason why you added RestartSec? Why not just let
it be the default (100ms)?

-- 
Samuli Seppänen
Community Manager
OpenVPN Technologies, Inc

irc freenode net: mattock

[1] 

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] systemd: Enable systemd's auto-restart feature for server profiles

2017-09-07 Thread Gert Doering

Hi,

On Thu, Sep 07, 2017 at 01:52:02AM +0200, David Sommerseth wrote:
> @@ -18,6 +18,8 @@ DeviceAllow=/dev/net/tun rw
>  ProtectSystem=true
>  ProtectHome=true
>  KillMode=process
> +RestartSec=5s
> +Restart=on-failure

Is there a way to get exponential backoff on restart?

Restarting is good, but if there is something faulty that leads to
"the process always dies right away", this can lead to very quickly
filling disks with not-so-useful logging...

(Otherwise, yes, restarting is good :-) )

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


signature.asc
Description: PGP signature
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] lz4: Move towards a newer LZ4 API

2017-09-07 Thread Gert Doering

HI,

On Thu, Sep 07, 2017 at 04:28:27AM +0200, David Sommerseth wrote:
> We are using a deprecated function, LZ4_compress_limitedOutput(), which
> will be removed with time.  The correct function to use is 
> LZ4_compress_default().
> Both function takes the same number of arguments and data types, so the change
> is minimal.

I wonder why we should bother to have a wrapper function here.

We can ship a lz4 library that has the new function, and if a system only
provides an older version, declare it unsuitable (configure check) and
use the bundled one.

Less #ifdef

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


signature.asc
Description: PGP signature
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] systemd: Enable systemd's auto-restart feature for server profiles

[Openvpn-devel] [PATCH v2] route: cleanup codestyle and make code more readable

Re: [Openvpn-devel] [PATCH] systemd: Enable systemd's auto-restart feature for server profiles

Re: [Openvpn-devel] [PATCH v2] lz4: Move towards a newer LZ4 API

Re: [Openvpn-devel] [PATCH] systemd: Enable systemd's auto-restart feature for server profiles

Re: [Openvpn-devel] [PATCH] systemd: Enable systemd's auto-restart feature for server profiles

Re: [Openvpn-devel] [PATCH] systemd: Enable systemd's auto-restart feature for server profiles

Re: [Openvpn-devel] [PATCH] systemd: Enable systemd's auto-restart feature for server profiles

Re: [Openvpn-devel] [PATCH] systemd: Enable systemd's auto-restart feature for server profiles

Re: [Openvpn-devel] [PATCH v2] lz4: Move towards a newer LZ4 API

[Openvpn-devel] [PATCH v2] lz4: Move towards a newer LZ4 API

Re: [Openvpn-devel] [PATCH] systemd: Enable systemd's auto-restart feature for server profiles

Re: [Openvpn-devel] [PATCH] lz4: Changing how LZ4 library handling is done

Re: [Openvpn-devel] [PATCH] systemd: Enable systemd's auto-restart feature for server profiles

Re: [Openvpn-devel] [PATCH] systemd: Enable systemd's auto-restart feature for server profiles

Re: [Openvpn-devel] [PATCH] lz4: Changing how LZ4 library handling is done

Re: [Openvpn-devel] [PATCH] lz4: Move towards a newer LZ4 API

Re: [Openvpn-devel] [PATCH] systemd: Enable systemd's auto-restart feature for server profiles

Re: [Openvpn-devel] [PATCH] systemd: Enable systemd's auto-restart feature for server profiles

Re: [Openvpn-devel] [PATCH] systemd: Enable systemd's auto-restart feature for server profiles

Re: [Openvpn-devel] [PATCH] route: cleanup codestyle and make code more readable

Re: [Openvpn-devel] [PATCH] systemd: Enable systemd's auto-restart feature for server profiles

Re: [Openvpn-devel] [PATCH] systemd: Enable systemd's auto-restart feature for server profiles

Re: [Openvpn-devel] [PATCH] systemd: Enable systemd's auto-restart feature for server profiles

Re: [Openvpn-devel] [PATCH] tcp-server: ensure AF family is propagated to child context

[Openvpn-devel] [PATCH] tcp-server: ensure AF family is propagated to child context

Re: [Openvpn-devel] [PATCH] systemd: Enable systemd's auto-restart feature for server profiles

Re: [Openvpn-devel] [PATCH] systemd: Enable systemd's auto-restart feature for server profiles

Re: [Openvpn-devel] [PATCH] systemd: Enable systemd's auto-restart feature for server profiles

Re: [Openvpn-devel] [PATCH] systemd: Enable systemd's auto-restart feature for server profiles

Re: [Openvpn-devel] [PATCH] lz4: Move towards a newer LZ4 API

31 matches

Site Navigation

Mail list logo

Footer information