On Fri, 20 Sep 2024 22:01:18 +0200 "Sebastian J. Bronner" <wasc...@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.
