Reworked array of pppd args #295
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hi, why did you open a second pull request for that? By the way, could you explain in the commit message the motivation for that change? Thanks in advance. |
|
Hi. I managed to mess the previous pull request, thus I closed it and opened another one. The motivation for the change is that adding/removing possible options requires coherent and error-prone changes to the number of NULLs in the args[] array, and the a-posteriori check at Line 194 in 68d24bd |
|
@adrienverge Hi. Are you considering this PR for merging in the trunk, or should I close it and perhaps open another one at a later time? |
|
Hi @gbon121, sorry, I won't be able to review it soon myself. The reason for this change seems legitimate, I hope it doesn't make the code less clear. |
|
Note that the code base is currently changed for BSD support - especially in |
|
Don't worry about this. I can take care of the needed code changes in one of the pull requests, whichever gets merged first. For the BSD support there is still a bit of work to be done in #300 (adding a sample configuration for ppp, testing, review of the man page, maybe hiding options that are not supported on BSD). |
|
@mrbaseman Hi, Martin. |
Use a dynamically (re)allocated array instead of a fixed-length args[]. This eliminates the need to carefully match the number of NULL placeholders with the number of possible options, and also the need for a-posteriori check of bounds.
|
@gbon121 I have squashed and rebased this for you. I remember you had a reference for the |
|
I have found what I had in mind #270 (comment) but you just wrote that it might silence coverty warnings |
|
@mrbaseman Right. Perhaps Coverity complained because the original code overwrites a fixed-length array via a cursor that is checked for validity only after (possibly) stepping over the end of the array. My function reallocates the array on demand and does a minimum of housekeeping. Not sure if Coverity is happy now, but we can try. Anyway, the current code should be less error-prone. |
|
Ah, Martin, would you please paste here the sequence of git commands that you used to rebase, squash and push the commits in this PR? Perhaps I can still learn how to do such edits myself... |
|
@gbon121 I'll have a look tomorrow. I can recommend this tutorial - What's described in the section "Squash your changes", especially the variant with the hash, is what I usually do |
|
Hi @gbon121, |
mrbaseman
added a commit
to mrbaseman/openfortivpn
that referenced
this pull request
Apr 26, 2018
This reverts commit de87e36. The reason to revert it is that a huge patch series is going to be landed which has conflicts with these changes here. An appropriately adapted change will be committed again at a later stage.
mrbaseman
added a commit
to mrbaseman/openfortivpn
that referenced
this pull request
Apr 26, 2018
This is a re-worked variant for commit de87e36 which had to be reverted for the patch series to apply. Original description: Use a dynamically (re)allocated array instead of a fixed-length args[]. This eliminates the need to carefully match the number of NULL placeholders with the number of possible options, and also the need for a-posteriori check of bounds.
mrbaseman
added a commit
to mrbaseman/openfortivpn
that referenced
this pull request
Oct 24, 2018
this is a squashed commit consisting of many smaller changes. Here the original commit messages: Revert "Reworked array of pppd args (adrienverge#295)" This reverts commit de87e36. The reason to revert it is that a huge patch series is going to be landed which has conflicts with these changes here. An appropriately adapted change will be committed again at a later stage. Use autoconf macros in #ifdef Note that 'struct termios' is defined in <termios.h> (POSIX). Some obscure systems might use <sys/termios.h> but we do not support that for now. avoid usage of '$^' in Makefile.am it is not supported in the make shipped with Freebsd in configure.ac use AM_SILENT_RULES only if it is available header inclusion of sys/mutex.h and net/if_var.h using autoconf variables include mach/mach.h based on autoconf variable for inclusion of termios.h and libutil.h use autoconf variables supppress compiler warnings in clang specifically for clang determine existence of /proc/net/route via configure script determine existence of netstat for osx and bsd via configure script use autoconf variable about existence of /proc/net/route once more use __clang__ for clang specific pragma from configure script check whether rtentry is available and has rt_dst a few missing bits for FreeBSD support, e.g. ppp instead of pppd restore traditional behavior with pppd (for travis build without run time dependency) make termios.h a required header during configure time detect ppp or pppd at run-time fail at configure time when required header is not found improve autoconf header tests: on FreeBSD some headers need others during the test apppropriate output for ppp in pppd_terminate go back from run-time detection to configure time detection of pppd introduced preprocessor variable ppp_path for the path to either pppd or ppp added the configure options to specify the location of ppp and pppd, respectively, use ipparam to select a specific "system" from the ppp configuration added debugging output of ppp_path which is not hard-coded anymore allow override of netstat location as configure option allow cross-compiling for a system with /proc/net/route on another one where it is missing, and add a missing backslash in Makefile.am as a fix to the previous commit update logic of the cross-compiling options to work in both directions hide options concerning ppp or pppd if not configured with this particular application force re-compilation after re-configuration Reworked array of pppd args (adrienverge#295) This is a re-worked variant for commit de87e36 which had to be reverted for the patch series to apply. Original description: Use a dynamically (re)allocated array instead of a fixed-length args[]. This eliminates the need to carefully match the number of NULL placeholders with the number of possible options, and also the need for a-posteriori check of bounds. ppp: add sample config, fix typo in command line option, print out error number in addition to strerror, more debugging output and reference added to ppp.conf.example once more fix configure script logic and properly define PPP_PATH a couple of minor updates * add speed and mru to the ppp.conf example, but I still do not recieve packets * replace the last #ifdef __APPLE__ with #if HAVE_MACH_MACH_H statements * use pppd_name in run_tunnel as well update ppp example config (thanks to @nferch) correctly use ppp vs. pppd in debug output Revert "correctly use ppp vs. pppd in debug output" This reverts commit 88a0fa6. change "pppd" to "ppp" in debugging output in general, indepedent of the daemon used for establishing the ppp link Add hacks for FreeBSD support Look for a different packet to determine whether configuration has completed. This should probably mirror the FSM in IPCP, but works for now. Fix backwards #if Skip "Refs" and "Use" columns as they aren't present in FreeBSD Use different interface name make it compile on linux again: - detection of pppd and subsequent definition of HAVE_USR_SBIN_PPPD did not work - some code parts were not properly merged back from master (regions where there were changes in both branches) make it compile on FreeBSD, my first successful connection with this commit! make it work on linux again main point: the ppp interface name is ppp on linux some more code cleanup apply coding style again
mrbaseman
added a commit
that referenced
this pull request
Oct 31, 2018
* FreeBSD support this is a squashed commit consisting of many smaller changes. Here the original commit messages: Revert "Reworked array of pppd args (#295)" This reverts commit de87e36. The reason to revert it is that a huge patch series is going to be landed which has conflicts with these changes here. An appropriately adapted change will be committed again at a later stage. Use autoconf macros in #ifdef Note that 'struct termios' is defined in <termios.h> (POSIX). Some obscure systems might use <sys/termios.h> but we do not support that for now. avoid usage of '$^' in Makefile.am it is not supported in the make shipped with Freebsd in configure.ac use AM_SILENT_RULES only if it is available header inclusion of sys/mutex.h and net/if_var.h using autoconf variables include mach/mach.h based on autoconf variable for inclusion of termios.h and libutil.h use autoconf variables supppress compiler warnings in clang specifically for clang determine existence of /proc/net/route via configure script determine existence of netstat for osx and bsd via configure script use autoconf variable about existence of /proc/net/route once more use __clang__ for clang specific pragma from configure script check whether rtentry is available and has rt_dst a few missing bits for FreeBSD support, e.g. ppp instead of pppd restore traditional behavior with pppd (for travis build without run time dependency) make termios.h a required header during configure time detect ppp or pppd at run-time fail at configure time when required header is not found improve autoconf header tests: on FreeBSD some headers need others during the test apppropriate output for ppp in pppd_terminate go back from run-time detection to configure time detection of pppd introduced preprocessor variable ppp_path for the path to either pppd or ppp added the configure options to specify the location of ppp and pppd, respectively, use ipparam to select a specific "system" from the ppp configuration added debugging output of ppp_path which is not hard-coded anymore allow override of netstat location as configure option allow cross-compiling for a system with /proc/net/route on another one where it is missing, and add a missing backslash in Makefile.am as a fix to the previous commit update logic of the cross-compiling options to work in both directions hide options concerning ppp or pppd if not configured with this particular application force re-compilation after re-configuration Reworked array of pppd args (#295) This is a re-worked variant for commit de87e36 which had to be reverted for the patch series to apply. Original description: Use a dynamically (re)allocated array instead of a fixed-length args[]. This eliminates the need to carefully match the number of NULL placeholders with the number of possible options, and also the need for a-posteriori check of bounds. ppp: add sample config, fix typo in command line option, print out error number in addition to strerror, more debugging output and reference added to ppp.conf.example once more fix configure script logic and properly define PPP_PATH a couple of minor updates * add speed and mru to the ppp.conf example, but I still do not recieve packets * replace the last #ifdef __APPLE__ with #if HAVE_MACH_MACH_H statements * use pppd_name in run_tunnel as well update ppp example config (thanks to @nferch) correctly use ppp vs. pppd in debug output Revert "correctly use ppp vs. pppd in debug output" This reverts commit 88a0fa6. change "pppd" to "ppp" in debugging output in general, indepedent of the daemon used for establishing the ppp link Add hacks for FreeBSD support Look for a different packet to determine whether configuration has completed. This should probably mirror the FSM in IPCP, but works for now. Fix backwards #if Skip "Refs" and "Use" columns as they aren't present in FreeBSD Use different interface name make it compile on linux again: - detection of pppd and subsequent definition of HAVE_USR_SBIN_PPPD did not work - some code parts were not properly merged back from master (regions where there were changes in both branches) make it compile on FreeBSD, my first successful connection with this commit! make it work on linux again main point: the ppp interface name is ppp on linux some more code cleanup apply coding style again * be more lax in detecting IPCP packet * clean up remove/normalize whitespace fix logging message * normalize output of ppp vs pppd * fix regression when no systemd is present on linux on Scientific Linux release 6.10 (Carbon), a RHEL clone maintained at CERN I have noticed that configure fails with the following error: checking for LIBSYSTEMD... ./configure: line 4877: syntax error near unexpected token `else' ./configure: line 4877: `else' if libsystemd is not found. The empty square bracket in configure.ac: PKG_CHECK_MODULES(LIBSYSTEMD, [libsystemd], [AC_DEFINE(HAVE_SYSTEMD)], [ ]) suppresses a human readable configure error but produces invalid shell code. Printing out a message that we haven't found libsystemd solves the problem. * rework header and function tests in configure.ac * fix overrides in configure.ac command line arguments (--with-... and --enable-...) are processed first, so it is clearer to move their definition more to the top of configure.ac. existence of files is processed later, so we have to put additional if- statements around so that the already processed overrides are not touched. we may not assign an empty value to the with_... and enable_... shell variables in configure.ac, because that would also override the values from command line * fix for the previous commit the travis workaround must come later than the check for file existence also the with_... values are empty now, instead of containing "no" * add runtime checks to verify that the configured executables really exist * avoid the last platform specific preprocessor directive
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.