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

Make lock dir configurable #435

Merged
merged 1 commit into from
Oct 10, 2023
Merged

Conversation

martinetd
Copy link
Contributor

@martinetd martinetd commented Aug 3, 2023

lock dir changed on linux from /var/lock to /run/pppd/lock with pppd-2.5.0, which makes pppd fail to start if the distribution does not pre-create the directory.

This reverts it back to /var/lock.

In the line of configurability, also make the lock dir configurable through ./configure --with-lock-dir for distributions that prefer /run/pppd/lock, but change the default for linux back to /var/lock

This commit is bigger than it should be, because the current configure mechanism for PPPD_RUNTIME_DIR and others is inconditional: pathnames.h has #else cases for when the variable is not defined but that is not possible.
This does not work for lock dir, because on non-linux platform the path is different, and reproducing this logic at configure time is not trivial.

Instead of the current method, only set the the PPPD_X_DIR variables in pppdconf.h if it was explicitly set in configure, and otherwise leave it to the else case.

Note:

  • the else cases were incorrect for runtime dir and log dirs, and have been fixed
  • PPPD_PLUGIN_DIR has been kept as is because it is used for rpath in makefiles and needs to be set inconditionnaly at configure time

Fixes: 66a8c74 ("Let ./configure control the paths for pppd")
Fixes: #419

martinetd added a commit to martinetd/ppp that referenced this pull request Aug 3, 2023
PR ppp-project#435 makes lockdir default go back to /var/lock, but runtime dir
still changed from /run to /run/pppd in commit 66a8c74 ("Let
./configure control the paths for pppd") and is likely to not exist on
some distros, in which case the pppdb will not be created.

This is not a big problem but might as well just try to create the
directory if it is missing.

Return code of mkdir does not need to be checked as the following open
will fail anyway if mkdir failed.

See: ppp-project#419
Signed-off-by: Dominique Martinet <[email protected]>
@martinetd
Copy link
Contributor Author

(ah, note the default for localstatedir is /usr/local/var so that'll make the default lock dir /usr/local/var/lock and not /var/lock, but distros will configure --localstatedir=/var and it'll work out in practice... Happy to override this to not use localstatedir if you prefer)

@martinetd
Copy link
Contributor Author

fixed make check (had to add LOCALSTATEDIR define to tests cflags as well now as they use PPP_PATH_LOCKDIR which now requires LOCALSTATEDIR for its default value)

@paulusmack
Copy link
Collaborator

It looks fine to me, but I would like to hear what @enaess thinks of it (and of #436).

@Neustradamus
Copy link
Member

@paulusmack: @enaess has done some comments in the other PR recently.
Not in this PR, have you seen?

@rfc1036
Copy link
Contributor

rfc1036 commented Sep 24, 2023

I do not think that the locks directory should be configurable: /var/lock/ is part of the Linux ABI and there is no reason to change it. People with non-standard file system schemes can use local non-standard patches.

@martinetd
Copy link
Contributor Author

Happy to hard-code it back to /var/lock then, I can update this PR ~end of next week if that's preferred; that should simplify this patch quite a bit.

@martinetd
Copy link
Contributor Author

Sorry for the delay I had forgotten to update this to hardcode back the lock path.

I know the standard path is /var/lock, but since we've always been using /var/run/lock I've preferred to revert back to the old path instead of using /var/lock: happy to change it to /var/lock, there's just no standard define for it in paths.h

Also, my original commit had a drive-by fix: "the else cases were incorrect for runtime dir and log dirs, and have been fixed"
This has also been dropped, but can be found in https://github.com/martinetd/ppp/commits/lock_dir_configurable if anyone cares.

@paulusmack
Copy link
Collaborator

Actually, in ppp-2.4.9 and prior, the lock directory is /var/lock on Linux or /var/spool/locks on Solaris. The lock directory changed in commit 66a8c74 ("Let ./configure control the paths for pppd", 2022-07-30) to PPP_PATH_LOCKDIR set in pathnames.h to /var/run/lock.

On the whole I would rather we went back to using /var/lock.

lock dir changed on linux from /var/lock to /run/pppd/lock with
pppd-2.5.0, which makes pppd fail to start if the distribution does not
pre-create the directory.

This reverts it back to /var/lock.

The paths for other OS should be identical as LOCALSTATEDIR should be
/var, but also revert them back as well just in case.
Since the variable is no longer used remove it from makefiles.

Fixes: 66a8c74 ("Let ./configure control the paths for pppd")
Fixes: ppp-project#419
Signed-off-by: Dominique Martinet <[email protected]>
@martinetd
Copy link
Contributor Author

Oh, sorry I didn't go far enough.
We had #define LOCK_DIR "/var/lock" indeed; I've reverted back to this.

That also made me notice the non-standard LOCALSTATEDIR define we use again which should indeed be /var, but since it looks like the general consensus is to not make it configurable I've reverted back to explicit /var for all OSes and removed the now-unused variable. (Happy to use LOCALSTATEDIR instead if that's better, just say)

@paulusmack paulusmack merged commit 99cbf5e into ppp-project:master Oct 10, 2023
25 checks passed
@martinetd
Copy link
Contributor Author

Thanks!

@martinetd
Copy link
Contributor Author

martinetd commented Oct 10, 2023

Hmm, sorry for the double-notification to everyone following this, but just a head's up github did something a bit weird here: it apparently rewrote the from of the merged commit?

martinetd@f838224 (original commit that I submitted/was reviewed) had From with my work e-mail (@atmark-techno.com) and doesn't have a co-authored-by;
the commit that was merged 99cbf5e has a from with my personal email (@codewreck.org) and got a co-authored-by added with the work mail... This is a bit weird given there's only one person involved.

(I'm not expecting this to be fixed at this point, but that also means the commit doesn't have a sign off from its "author")

I've never seen this before so not sure what repo setting is involved here, but someone might want to have a look...
(assuming github did this because the commiter of the commit is marked GitHub <[email protected]>; please ignore this if it was intended)

(EDIT: I've now added work email to this github account so my other PR should work fine, but this is still annoying...)

algitbot pushed a commit to alpinelinux/aports that referenced this pull request Oct 10, 2023
Backport upstream fix for pppd failing to start when lock dir
does not exist (by reverting the lock dir to somewhere that
should always exist)

This fixes using pppd from something else than the main pppd service
(e.g. networkmanager of ifupdown-ng), as these would not go through the
checkpath hook, and pppd can now run without /run/ppp

Link: ppp-project/ppp#435
Fixes: #15145
algitbot pushed a commit to alpinelinux/aports that referenced this pull request Oct 10, 2023
Backport upstream fix for pppd failing to start when lock dir
does not exist (by reverting the lock dir to somewhere that
should always exist)

This fixes using pppd from something else than the main pppd service
(e.g. networkmanager of ifupdown-ng), as these would not go through the
checkpath hook, and pppd can now run without /run/ppp

(cherry picked from commit 4a61d71)

Link: ppp-project/ppp#435
Fixes: #15145
bell-sw pushed a commit to bell-sw/alpaquita-aports that referenced this pull request Oct 17, 2023
[ commit 4a61d7196b8c5808cad5fbed61aba06934bd256f ]

Backport upstream fix for pppd failing to start when lock dir
does not exist (by reverting the lock dir to somewhere that
should always exist)

This fixes using pppd from something else than the main pppd service
(e.g. networkmanager of ifupdown-ng), as these would not go through the
checkpath hook, and pppd can now run without /run/ppp

Link: ppp-project/ppp#435
Fixes: #15145
@Neustradamus Neustradamus removed the request for review from paulusmack December 21, 2023 00:04
@Bonnietwin Bonnietwin mentioned this pull request Mar 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

lock directory moved in ppp-2.5.0
4 participants