added --loop option for automatic reconnects #190
Conversation
src/main.c
Outdated
| "\"%s\"\n", optarg); | ||
| break; | ||
| } | ||
| cfg.loop = loop; |
There was a problem hiding this comment.
A long is cast into an int. I'm not programming much in C these days, but couldn't this be an issue if loop is insanely large?
There was a problem hiding this comment.
very large values don't make much sense here. The value is used as argument for sleep(unsigned int), so even in this call, negative values are (probably) cast to large positive numbers. The worst that can happen is that openfortivpn waits a very long time before trying to reconnect, which gives a user experience nearly as if loop was left with the default value of 0.
I don't feel that a code change is needed here, but I'm open for suggestions. Maybe some comments of this discussion are helpful
There was a problem hiding this comment.
OK with me, that's not an important issue.
Let's reconsider in case it raises a Coverity issue.
src/main.c
Outdated
| sleep(cfg.loop); | ||
| if (run_tunnel(&cfg) != 0) | ||
| ret = EXIT_FAILURE; | ||
| } |
There was a problem hiding this comment.
I would prefer something like this (run_tunnel() is called from a single place). What do you think?
while (get_sig_received() == 0) {
if (run_tunnel(&cfg) != 0) {
ret = EXIT_FAILURE;
break;
} else
ret = EXIT_SUCCESS;
if (cfg.loop == 0)
break;
sleep(cfg.loop);
}
There was a problem hiding this comment.
yes, I was looking for something like this, but first didn't get the signal handling right and after I had introduced get_sig_received() I forgot to come back to this loop condition. Something with do { ... } while ( ) might be another solution that doesn't need a break;
There was a problem hiding this comment.
Right, do { ... } while ( ) might be better. But don't we need a break anyway to avoid the last sleep() call which should occur after the run_tunnel() call?
There was a problem hiding this comment.
I have just updated the branch with d489dd7 which addresses this (not sure if my ifs are nicer than your breaks though...)
|
A general remark: Supporting automatic reconnect was an idea which came up in one of the issues about unstable connections. I think they can mostly be ascribed MTU problems or other problems in the lower layers. Connections that are dropped after hours most probably hit the idle timeout configured on the vpn gateway. Does it still make sense to implement automatic reconnects from the client side? |
|
I'm not so fond of automatically reconnecting either. Sure, it might make sense when you hit the gateway timeout - which I believe is ~8 hours by default whether the connection is idle or not. On the other hand many FortiGate devices are configured to accept only tokens / one-time passwords. Reconnecting doesn't make sense in such cases. Whether the feature is useful or not, I think I'd rather keep it outside openfortivpn and write a simple shell wrapper that restarts openfortivpn, or use a crontab, or use the
|
|
A wiki that provides some extra hints and helper scripts is a good idea. I have seen the request for init scripts or ipup scripts for pppd several times already. But I'm using two factor authentication with Tokens as well, so I can't run it purely script based either... |
mrbaseman
force-pushed
the
reauthentication
branch
8 times, most recently
from
41cf497 to
db11494
Compare
mrbaseman
force-pushed
the
reauthentication
branch
2 times, most recently
from
85fa836 to
7ff6de2
Compare
mrbaseman
force-pushed
the
reauthentication
branch
2 times, most recently
from
5fef8a4 to
863e892
Compare
mrbaseman
force-pushed
the
reauthentication
branch
from
863e892 to
b41e24a
Compare
mrbaseman
force-pushed
the
reauthentication
branch
from
b41e24a to
da99aa2
Compare
|
the reauthentication feature was requested and successfully tested in #92. @DimitriPapadopoulos I have just seen that I hadn't yet properly addressed the cast of a long to an unsigned int. Now I have done so and also a bit of cleanup. Are you ok with merging this small patch into master (after rebasing and squashing it into a single commit)? |
|
Looks good to me. To tell the truth I would prefer to keep the reconnection mechanism outside of openfortivpn (a shell wrapper ?) but if there is a widespread need it's OK with me - I am of course biased by my own experience and needs. |
|
I think we have a couple of requests for this feature. Only, I'm not so happy with calling the command line option "--loop". Maybe there are better proposals? If not, I would rebase and merge this in the near future. (I was ill and on vacation and had a considerable backlog in my mailbox, now I'm ill again, so my activities here are a bit slowed down... ) |
|
Like Dimitri I'd rather use a wrapper to relaunch openfortivpn, but since there are multiple demands for this, let's do it!
Maybe |
|
Perhaps Most probably we should reuse the vocabulary used in systemd and other init systems. See for example How To Configure a Linux Service to Start Automatically After a Crash or Reboot, Autostart using systemd or Ensure systemd services restart on failure:
|
Sounds good too 👍 |
|
Nice job! Should I release v1.7.0? Or do you guys plan to merge other features soon? |
No description provided.