Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Small improvements #124

Closed
wants to merge 4 commits into from
Closed

Conversation

digmore
Copy link

@digmore digmore commented May 18, 2017

Hi,

Great tool... thanks for sharing it.

This PR offers some small improvements I made in response to issues I had getting up and running.

Firstly, I was also confused by the OTP handling as in #104. It turns out the documentation mentions both "-o" and "--otp" switches, but only the latter was implemented in the code. I have added handling for the short option here. This may possibly close issue #104 as one option literally didn't exist and there is a spelling mistake in the second option in his example.

Secondly, I was having DNS issues due to the search option of resolv.conf not being supported. This information doesn't come back from my VPN end-point when the tunnel is established so it seems right that the client doesn't set it. As a workaround I added support to the configuration file for defining custom scripts to call on tunnel up and interface down events. For my case I manipulate this search parameter from a shell script called by openfortivpn when these events occur. I'm sure there would be other use cases; It seems to be a feature present in other VPN clients.

Please consider these for inclusion. C is not my most comfortable language so hopefully it is acceptable. It has tested okay over a few days on my machine.

Thanks

digmore and others added 4 commits May 11, 2017 16:48
Add optional configuration items "up-script" and "down-script" to
define the paths to custom scripts/programs, which should be
executed on tunnel up and interface down events respectively.
@mrbaseman
Copy link
Collaborator

thanks for the hint about the -o option. Together with a few other minor fixes I have just landed this on master.
About the main code change here: The up- and down-scripts: There is already an approach for resolvconf.d which makes use of the ip-up and ip-down scripts of pppd in /etc/ppp/ (see #101).
Yesterday I have implemented the support for ipparam of pppd which could be used for the search parameter in resolv.conf, too.
I understand that your solution is more transparent to first time users, but I would like to keep openfortivpn a relatively thin client and not put too much features into it, especially when the same thing can be achieved with other means.
Maybe we were just by coincidence working on the same problem and arrived at two different solutions. But if your approach has clear advantages, please explain and I would be happy to accept your pull request.
One more thing: @adrienverge has a strong interest in keeping the commits to the repository clean. The commits should be atomic and revertable if needed. This pull request is about two separate problems (-o option and the scripts), which makes it harder to squash all the commits (including the rebase and coding style fixes) into one single transaction. Now that I have already put the fix for the -o option into master, rebasing the branch should solve that problem, but I just wanted to mention this here.

@digmore
Copy link
Author

digmore commented May 21, 2017

@mrbaseman, I appreciate for the tip about improving PRs.

I hadn't considered the interplay with pppd. I should have checked the other issues first. I managed to get this working using the ip-up.d/ip-down.d script approach with your ipparam branch. It's fine for me needs.

I'm going to close this PR as it's unnecessary. Thanks for the feedback..

@digmore digmore closed this May 21, 2017
@digmore digmore deleted the small-improvements branch May 21, 2017 23:30
@mrbaseman
Copy link
Collaborator

thanks for confirming that the ipparam is working. So I have just merged my branch into master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants