Suggestion: login.c->record_login()

[Andre Lucas]

Hi,

A lot of the problems with openssh portability so far appear to be with
the login record functionality, i.e. lastlog support, and variations on
handling utmp vs. utmpx etc. Looking at for-profit SSH 1.2.27, login.c
is rather embarassing spaghetti code, so laden with '#ifdef's it's
almost impossible to read.

OpenSSH's code isn't anything like that, but then it doesn't support as
many platforms yet. Even with the best of intentions, there's every
prospect that the code will mutate into the same kind of thing as in
SSH. It's a tricky problem to solve, because the code varies so much and
is so important.

I wonder if this is a suitable moment to suggest that record_login()
gets a major rewrite.  We could abstract it more, so we pass a
superstructure containing all the information we have to functions that
handle exactly one of utmp, utmpx, wtmp, wtmpx, lastlog, or whatever
else comes along.

*Then* we could use #ifdef to call the right code. One way might be:

#ifdef LINUX
/* Linux: Do the business with PAM <cough> */
set_pam_entry   (struct logindata l);
/* ... whatever else Linux needs ... */
#endif /* LINUX */

#ifdef HPUX
/* HPUX: Do things the hard way
 * no lastlog, utmp *and* utmpx, wtmp */
set_utmp_entry  (struct logindata l);
set_utmpx_entry (struct logindata l);
set_wtmp_entry  (struct logindata l);
#endif /* HPUX */
etc...

It's still not exactly pretty, but it's a lot more readily understood
IMO. (I've left error propogation out for clarity at this point.)

There would still be #ifdef blocks in the individual routines, to cover
for platform differences in the individual structures, but even that's
clearer; it's far simpler to see why a change is being made for a
particular platform if you don't have to wonder what kind of structure
will eventually be set for which platform!

I have two reasons for suggesting this approach over another potential
method, that of having a platform-specific file or block for each
supported OS. First, that complicates the Makefile (IMO) unnecessarily
with different files (or makes one huge conditionally compiled file with
one file), and secondly it doesn't stand a chance of working on a
platform without specific support. The second is the killer for me.

I'm aware that there are issues for Damien here in keeping track of
OpenBSD changes to the OpenSSH codebase. I still think that this would
be tidier and easier in the long run.

I look forward to your comments.

Ta,
-Andre Lucas
Instinet Global Services