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

Support elogind as logind alternative #787

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Polynomial-C
Copy link

Can be enabled with --enable-logind=elogind

logind from systemd is still the default

Can be enabled with --enable-logind=elogind

logind from systemd is still the default
@Polynomial-C
Copy link
Author

You can find the elogind project here: https://github.com/elogind/elogind

It's a drop-in replacement for systemd-logind as it's based on the systemd sources.

@thkukuk
Copy link
Contributor

thkukuk commented Apr 11, 2024

Basically the patch is ok, but I expect that Linux-PAM will use more systemd functions in the future, even if I don't have anything concrete in mind at the moment.

That's why I would prefer SYSTEMD_LIBS and EGLOGIND_LIBS to be two independent variables from the beginning, so that we don't have to separate them later again.

@ldv-alt what do you think?

Comment on lines +563 to +564
AS_IF([test "$WITH_LOGIND" != "no"], [
AS_CASE(["$enableval"], [elogind], [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be better to use AS_CASE(["$WITH_LOGIND"], ...) instead of AS_IF(...) and AS_CASE(["$enableval"], ...)?

AS_CASE(["$enableval"], [elogind], [
PKG_CHECK_MODULES([LOGIND], [libelogind >= 255], [LOGIND_CFLAGS="-DUSE_LOGIND=1 $LOGIND_CFLAGS"])
], [
PKG_CHECK_MODULES([LOGIND], [libsystemd >= 254], [LOGIND_CFLAGS="-DUSE_LOGIND=1 $LOGIND_CFLAGS"], [AS_IF([test "$WITH_LOGIND" != "check"], [AC_MSG_ERROR([Cannot find libsystemd])])])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When using AS_CASE, it would be more readable to handle each case separately rather than merging yes and check cases.

@ldv-alt
Copy link
Member

ldv-alt commented Apr 11, 2024

Basically the patch is ok, but I expect that Linux-PAM will use more systemd functions in the future, even if I don't have anything concrete in mind at the moment.

That's why I would prefer SYSTEMD_LIBS and EGLOGIND_LIBS to be two independent variables from the beginning, so that we don't have to separate them later again.

@ldv-alt what do you think?

We could have SYSTEMD_* and ELOGIND_* variables initialized in configure.ac, and LOGIND_* variables based on them that are AC_SUBST'ed and would currently be used in Makefile.am files. This way we could make SYSTEMD_* variables AC_SUBST'ed when needed.

@Yamakuzure
Copy link

Basically the patch is ok, but I expect that Linux-PAM will use more systemd functions in the future, even if I don't have anything concrete in mind at the moment.

libelogind.so is a drop-in replacement for libsystemd.so

If you need any public function that is currently stubbed, we might add it.

However, why would PAM use (any) systemd (functions), as systemd-logind uses PAM, and has several PAM modules. One of them, pam_systemd, is distributed as pam_elogind by us.

@thkukuk
Copy link
Contributor

thkukuk commented Apr 17, 2024

However, why would PAM use (any) systemd (functions), as systemd-logind uses PAM, and has several PAM modules. One of them, pam_systemd, is distributed as pam_elogind by us.

If PAM would not use systemd functions, there would be no need for you to add elogind support.
One year ago PAM didn't used any libsystemd function and nobody could imagine that we ever would. Now we are using libsystemd functions to replace utmp with systemd-logind.
Nobody knows what will come tomorrow, and which functions more we need/want to use.

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

Successfully merging this pull request may close these issues.

None yet

4 participants