Hi Laslo,
thank you for your feedback. I applied some of it straight away and
packed the updated code into another tarball under
https://sbronner.com/~waschtl/slock-1.5-waschtl-modular2.tar.gz
I split your feedback into 10 points which I will respond to one at a
time:
> Regarding the code structure, I would put the backends into a subfolder
> backends/, and call the source files "pam.c", "passwd.c", etc.
Done. I'm very much in favor of keeping things orderly. While I was
making these changes, I discovered that using '/' in target names in the
Makefile, while apparently compliant with POSIX.1-2024, seems not to be
compliant with POSIX.1-2017. However, the three make implementations [1]
I tested do not mind, except for pdpmake when specifically told to
strictly comply with POSIX.1-2017. I used the Makefile of slstatus for
inspiration.
[1] pdpmake 2.0.1 (2024-09-18)
bmake 20240909
GNU make 4.4.1
> 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.
I thought about this a lot, and I keep coming back to link-time
selection of functions being better than run-time selection of
functions. I can think of several reasons for this: (1) It potentially
keeps the compiled binary smaller. (2) It keeps backend-specific code
contained to 3 files: The .c file in backends, config.def.h, and
config.mk. Otherwise an additional file would be necessary just to
define the function mappings. (3) It avoids a global variable.
The idea here is not to have all backends available in the binary, but
just one of them selected at link time. To me, at least, this implies a
static API.
> 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!
This is a point I will gladly revisit. I'll get back to you on this.
> Regarding the makefile (including config.mk), you use a few GNUisms
> (e.g. +=), which makes it non-POSIX compliant
Fixed. Oops. I never realized how influenced by GNU my Makefile was
until now. I read up on the POSIX Makefile specs and fixed this. I quite
identified with statements at
https://nullprogram.com/blog/2017/08/20/ .
> (strictly speaking nested
> macros are also not POSIX compliant).
These I couldn't figure out how to avoid. There are a few points
pointing at them being potentially OK to leave in, though:
1. They are allowed in POSIX.1-2024.
2. All the tested make implementations accepted them (see above).
3. Using targets with the '/' is similarly non-POSIX.1-2017, but
compliant with POSIX.1-2024, and are used in another suckless project
(slstatus).
> It's also not simple, with quite a
> few shell commands,
Fixed. These were GNU conditionals. They are no more.
> and I don't see why config.mk stuff is put into the
> Makefile,
Fixed. I had never heard of config.mk before this. A also was not able
to find any documentation mentioning it anywhere. But I put it back the
way it seemed to be intended. Can you point me to some documentation
about config.mk files?
> and install(1) is a GNU tool, not a POSIX tool.
Fixed.
> I think a
> main goal should be that it's still compilable even if libpam is not
> installed.
I completely agree. Compiling with the passwd backend selected does not
require any of the pam-specific dependencies to be installed (libpam,
libXft). Or did you mean something else?
> The specification of the authentication backend during compile time via
> AUTH is an interesting approach, but also fragile.
The idea is that someone packaging slock for distribution with an
operating system will set it up using the variables BACKEND and possibly
CFLAGS_passwd. Making it fragile at that point makes it robust after
installation. This way it will always attempt to function as intended,
regardless of a misconfiguration of the system later. This is also the
most straightforward way of only requiring the libraries needed for the
selected backend that I could come up with.
Kind regards,
Sebastian
Received on Fri Oct 04 2024 - 07:36:58 CEST