Mailing List Archive: vpnc: devel

persist option (patch)

Index | Next | Previous | View Threaded

Martin.vGagern at gmx

Jan 14, 2008, 1:16 AM

Post #1 of 7 (475 views)
Permalink

persist option (patch)

Martin von Gagern wrote:
> In the meantime, I'd be interested in a feature like the persist option
> of pppd. There should be a way to configure a permanent VPN connection,
> and to have vpnc try reestablish that whenever it gets lost.

I hacked a bit of code, and what I have now seems to work well enough.
At least it keeps running here despite a forced DSL disconnect every
24h, and according to lsof there seem to be no stale open file descriptors.

The patch causes large parts of the main body to run in a loop. It
reestablishes connection when it receives a SIGUSR1. There is also a new
option, Persist aka. --persist, which causes connection reestablishment
for the -1 and -2 values of do_kill, where -2 is the dpd that kept
bugging me.

The SIGUSR1 feature is yet undocumented. The loop body in main is poorly
indented, as I didn't want to distract from the main parts of the
modification by changing that much whitespace. Should be indented when
it gets applied.

Comments appreciated. Feel free to apply if you agree this is ready enough.

Greetings,
Martin von Gagern

Attachments:

persist1.patch (3.17 KB)

signature.asc (0.25 KB)

peter.speybrouck at gmail

Jan 14, 2008, 3:22 AM

Post #2 of 7 (451 views)
Permalink

Re: persist option (patch) [In reply to]

I tried the patch but not yet with debug enabled and it seems to work.

In my syslog I saw:
Jan 14 12:08:24 Peter vpnc[9987]: connection terminated by peer

but I was still connected after that. Let's see how long it lasts.

Good work!

Peter

2008/1/14 Martin von Gagern <Martin.vGagern[at]gmx.net>:
> Martin von Gagern wrote:
> > In the meantime, I'd be interested in a feature like the persist option
> > of pppd. There should be a way to configure a permanent VPN connection,
> > and to have vpnc try reestablish that whenever it gets lost.
>
> I hacked a bit of code, and what I have now seems to work well enough.
> At least it keeps running here despite a forced DSL disconnect every
> 24h, and according to lsof there seem to be no stale open file descriptors.
>
> The patch causes large parts of the main body to run in a loop. It
> reestablishes connection when it receives a SIGUSR1. There is also a new
> option, Persist aka. --persist, which causes connection reestablishment
> for the -1 and -2 values of do_kill, where -2 is the dpd that kept
> bugging me.
>
> The SIGUSR1 feature is yet undocumented. The loop body in main is poorly
> indented, as I didn't want to distract from the main parts of the
> modification by changing that much whitespace. Should be indented when
> it gets applied.
>
> Comments appreciated. Feel free to apply if you agree this is ready enough.
>
> Greetings,
> Martin von Gagern
>
> _______________________________________________
> vpnc-devel mailing list
> vpnc-devel[at]unix-ag.uni-kl.de
> https://lists.unix-ag.uni-kl.de/mailman/listinfo/vpnc-devel
> http://www.unix-ag.uni-kl.de/~massar/vpnc/
>
>
_______________________________________________
vpnc-devel mailing list
vpnc-devel[at]unix-ag.uni-kl.de
https://lists.unix-ag.uni-kl.de/mailman/listinfo/vpnc-devel
http://www.unix-ag.uni-kl.de/~massar/vpnc/

jmvpnc at loplof

Jan 21, 2008, 6:02 PM

Post #3 of 7 (438 views)
Permalink

Re: persist option (patch) [In reply to]

On Mon, Jan 14, 2008 at 10:16:27AM +0100, Martin von Gagern wrote:
> Martin von Gagern wrote:
> >In the meantime, I'd be interested in a feature like the persist option
> >of pppd. There should be a way to configure a permanent VPN connection,
> >and to have vpnc try reestablish that whenever it gets lost.
>
> I hacked a bit of code, and what I have now seems to work well enough.
> At least it keeps running here despite a forced DSL disconnect every
> 24h, and according to lsof there seem to be no stale open file descriptors.

Good. I'm just afraid that it will add additional memory leaks - yes,
there are some :-/ but we do not need to add new ones. Can you please test
that (e.g. run with valgrind).

> The patch causes large parts of the main body to run in a loop. It
> reestablishes connection when it receives a SIGUSR1. There is also a new
> option, Persist aka. --persist, which causes connection reestablishment
> for the -1 and -2 values of do_kill, where -2 is the dpd that kept
> bugging me.

Very nice.

> The SIGUSR1 feature is yet undocumented.

Please provide a patch. Also, please document the return codes.

> The loop body in main is poorly
> indented, as I didn't want to distract from the main parts of the
> modification by changing that much whitespace. Should be indented when
> it gets applied.

I'm glad you didn't indent yet, it makes the patch much easier to read.
Please don't indent later patches, I'll apply in two phases so that the
core of the patch will remain easy to read.

> Comments appreciated. Feel free to apply if you agree this is ready enough.

Comments on code inline.

> --- config.c (revision 278)
> +++ config.c (working copy)
> + CONFIG_PERSIST, 0, 1,
> + "--persist",
> + "Persist",
> + NULL,
> + "Reconnect when connection is lost",
> + NULL
> + }, {

> + opt_persist = (config[CONFIG_PERSIST]) ? 1 : 0;

IMO persist should be assigend a value like true/false or on/off.
The default should be true/on.

> Index: vpnc.c
> ===================================================================
> --- vpnc.c (revision 278)
> +++ vpnc.c (working copy)
> @@ -37,6 +37,7 @@
> #include <poll.h>
> #include <sys/ioctl.h>
> #include <sys/utsname.h>
> +#include <signal.h>
>
> #include <gcrypt.h>
>
> @@ -199,6 +200,13 @@
> fcntl(sock, F_SETFD, FD_CLOEXEC);
> #endif
>
> +#ifdef SO_REUSEADDR

> + if (opt_persist) {

Why not set this unconditionally, no matter whether opt_persist is set
or not?

Thanks!
Joerg

--
Joerg Mayer <jmayer[at]loplof.de>
We are stuck with technology when what we really want is just stuff that
works. Some say that should read Microsoft instead of technology.
_______________________________________________
vpnc-devel mailing list
vpnc-devel[at]unix-ag.uni-kl.de
https://lists.unix-ag.uni-kl.de/mailman/listinfo/vpnc-devel
http://www.unix-ag.uni-kl.de/~massar/vpnc/

Martin.vGagern at gmx

Jan 22, 2008, 6:34 AM

Post #4 of 7 (437 views)
Permalink

Re: persist option (patch) [In reply to]

Hi Joerg,

Thanks for your feedback.

Joerg Mayer wrote:
> Good. I'm just afraid that it will add additional memory leaks - yes,
> there are some :-/ but we do not need to add new ones. Can you please test
> that (e.g. run with valgrind).

I've never before worked with valgrind, but that looks easy enough.
Indeed found some leaks. Some fixed in the attached patch.

There is one in do_phase1 which I didn't know how to fix. I marked it
with a comment in this patch. The first packet contains a chain of
payloads, beginning first with sa and second with ISAKMP_PAYLOAD_KE.
Later on this chain is broken in parts, so the free_isakmp_packet
doesn't catch it all. There are pointers to elements of these chains,
but as far as I can tell not necessarily the heads. I can imagine
several possible solutions:

1. Remember heads of chain fragments in extra variables in order to
free them later
2. Restructure chains so that the referenced element becomes the head
3. Free chain elements one after the other by clearing the next
pointer first, retaining the referenced elements

Which approach would you prefer? I also haven't looked at code and specs
closely enough to know what these payloads are all about, and which
parts of a chain referenced by head will still be required later.

Trying to simulate a connection loss by unplugging the ethernet cable, I
found out that the reconnect died with "no response from target". I
suppressed that exit for persistent connections, and limited the
exponential backoff to 2^6. Seems to work.

This makes me think what other errors should become recoverable in
persistent mode, how to recover from them, and what error codes to use
for the non-recoverable ones. Also some syslog messages for non-fatal
errors would seem a good idea. This whole issue will take some more
investigation, postponed for at least a month until I find the time,
except someone else wants to help.

> Please provide a patch. Also, please document the return codes.

In that case we'd probably want some more return codes described. I
started paragraphs in the man page. I think we should introduce at least
one error code for configuration file issues, and maybe one for network
errors. Those modifications would affect a lot of source lines, so maybe
you'd want them in a separate patch, not together with the persistance
stuff. Is there a preferred header where to define appropriate exit
values for the different conditions?

> IMO persist should be assigend a value like true/false or on/off.
> The default should be true/on.

Makes sense. Implemented that way, with a reusable function atobool.

>> +#ifdef SO_REUSEADDR
>> + if (opt_persist) {
>
> Why not set this unconditionally, no matter whether opt_persist is set
> or not?

Wanted to reduce the possible impact of my modifications, to make
acceptance more likely. Also I'm not sure about the effect of two
different processes reusing that address in short sequence, although for
USP sockets at least that shouldn't be much of a problem. Set
unconditionally in this patch.

The attached patch shows current state of affairs, but is probably not
fit for inclusion, as the described memory leak should be fixed first.
After that, a commit of the persistance stuff would be great, in order
to make the exit codes into a separate patch without conflicts.

Greetings,
Martin von Gagern

Attachments:

persist2.patch (6.23 KB)

signature.asc (0.25 KB)

opfer at gentoo

Jan 22, 2008, 7:56 AM

Post #5 of 7 (435 views)
Permalink

Re: persist option (patch) [In reply to]

Hi,

Martin von Gagern <Martin.vGagern[at]gmx.net>:
[...]
+.B 1
+There is an error in the configuration file.
+.TP
+.B 1
+An error occurred using some network resource or system call.
+.TP
[...]

1 occurs twice. Is this intended?

V-Li

--
Christian Faulhammer, Gentoo Lisp project
<URL:http://www.gentoo.org/proj/en/lisp/>, #gentoo-lisp on FreeNode

<URL:http://www.faulhammer.org/>

Attachments:

signature.asc (0.18 KB)

Martin.vGagern at gmx

Jan 22, 2008, 10:13 AM

Post #6 of 7 (436 views)
Permalink

Re: persist option (patch) [In reply to]

Christian Faulhammer wrote:
> Hi,
>
> Martin von Gagern <Martin.vGagern[at]gmx.net>:
> [...]
> +.B 1
> +There is an error in the configuration file.
> +.TP
> +.B 1
> +An error occurred using some network resource or system call.
> +.TP
> [...]
>
> 1 occurs twice. Is this intended?

It is intended to represent the current state of affairs. There are two
paragraphs, because in the long run i'd rather have two different exit
codes. Right now, however, all those situations will exit with 1, so
instead of documentation inconsistent with behaviour, I'd rather have
this strange man page. I don't really expect this to be committed until
the exit codes have been diversified, though, and that will include
adjustments to this part of the man page as well.

Greetings,
Martin von Gagern

Attachments:

signature.asc (0.25 KB)

Martin.vGagern at gmx

May 6, 2008, 4:59 PM

Post #7 of 7 (256 views)
Permalink

Re: persist option (patch) [In reply to]

Hi!

I'm back to continue working on a persist option. I hope your mail
client and the archives place this mail here along with the original
thread. In case they don't, you can find its history here:
http://thread.gmane.org/gmane.network.vpnc.devel/2008/focus=2086

Martin von Gagern wrote:
> Joerg Mayer wrote:
>> Good. I'm just afraid that it will add additional memory leaks - yes,
>> there are some :-/ but we do not need to add new ones. Can you please
>> test that (e.g. run with valgrind).

OK, now I have the memory leaks down to two, at least for short
connections here in my setup. These two do repeat for persistent
reconnects, so they should get fixed in order to make persistence work.
I've marked them with comments containing LEAK.
Anyone willing to help me track these down?

> There is one in do_phase1 which I didn't know how to fix. I marked it
> with a comment in this patch. The first packet contains a chain of
> payloads, beginning first with sa and second with ISAKMP_PAYLOAD_KE.
> Later on this chain is broken in parts, so the free_isakmp_packet
> doesn't catch it all. There are pointers to elements of these chains,
> but as far as I can tell not necessarily the heads. I can imagine
> several possible solutions:
>
> 1. Remember heads of chain fragments in extra variables in order to
> free them later
> 2. Restructure chains so that the referenced element becomes the head
> 3. Free chain elements one after the other by clearing the next
> pointer first, retaining the referenced elements

I thought of a fourth solution: temporarily set next to NULL, flatten
the payload, and reestablish the next pointer. Introduced a new function
flatten_isakmp_payload1 to do this.

> This makes me think what other errors should become recoverable in
> persistent mode, how to recover from them, and what error codes to use
> for the non-recoverable ones. Also some syslog messages for non-fatal
> errors would seem a good idea. This whole issue will take some more
> investigation, postponed for at least a month until I find the time,
> except someone else wants to help.

I had a closer look at this, and I get the feeling this will become a
major rewrite of code, as I'd have to catch send errors in sendrecv in
case the underlying interface is temporarily down. I'd have to check
that condition for every location where sendrecv gets called, directly
or indirectly, and make sure to clean up all cruft while returning to
the main loop.

I won't do this kind of rewriting here in my working copy, and I fear
I'm likely to have unstable code while in transit. Is there any chance
of getting a branch in the project svn, and write permissions to that at
least?

Greetings,
Martin von Gagern

Attachments:

persist3.patch (16.5 KB)

signature.asc (0.25 KB)

Index | Next | Previous | View Threaded

Interested in having your list archived? Contact lists@gossamer-threads.com