On Fri, 20 Sep 2024 22:01:18 +0200
"Sebastian J. Bronner" <waschtl_AT_sbronner.com> wrote:
Dear Sebastian,
thanks for your hard work, interesting!
> I am offering this code back to suckless to incorporate as desired and
> to get feedback. If the general idea is something that is of interest
> to the project, I am quite willing to adjust the code based on
> feedback to fit better.
Overall I find the approach to abstract out different authentication
backends cool. Let me give you some feedback:
> To share the code with you all, I ran `make dist` and uploaded the
> resultant tarball to
>
> https://sbronner.com/~waschtl/slock-1.5-waschtl-modular.tar.gz
>
> I look forward to all of your reactions.
Regarding the code structure, I would put the backends into a subfolder
backends/, and call the source files "pam.c", "passwd.c", etc. I don't
think it's a great approach to implement a function interface
(auth_*()) as a link-time name resolution. Your approach with common
functions screams for the use of function pointers, e.g. defining a
struct backend containing the backends you listed, and then defining an
array of backends in a backend.c or backend.h.
This would allow you to set some function pointers to NULL where it
doesn't make sense, e.g. 'backspace' for the PAM backend. Also think
about providing a better abstraction, e.g. a possibility for the
backends to have a void 'state' pointer that they can set up during
initialisation. In this way, the passwd backend could store the entered
password in this state (which might as well also be a full custom
struct), and you save yourself from abstracting the input of characters
through the auth-API.
Going even further, maybe the best approach would be to have a
well-defined event interface. Anyway, just keep it as simple as
possible!
Regarding the makefile (including config.mk), you use a few GNUisms
(e.g. +=), which makes it non-POSIX compliant (strictly speaking nested
macros are also not POSIX compliant). It's also not simple, with quite a
few shell commands, and I don't see why config.mk stuff is put into the
Makefile, and install(1) is a GNU tool, not a POSIX tool. I think a
main goal should be that it's still compilable even if libpam is not
installed.
The specification of the authentication backend during compile time via
AUTH is an interesting approach, but also fragile.
Please let me know what you think.
With best regards
Laslo
PS: I send it also directly in CC as my mail sent to the ML usually
gets put right into spam.
Received on Sat Sep 21 2024 - 10:05:40 CEST