Suggestion: login.c->record_login()

[Damien Miller]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Mon, 27 Dec 1999, Andre Lucas wrote:

> 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.

No kidding :) The login portions of OpenSSH have, so far, proved to
be the most platform dependant.

Apart from the PAM code, most other changes have been minor or 
drop-in replacements for missing functions.

> 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:

[pseudocode snipped]

> 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.)

I think this is an excellent idea. If it works well, then it could
also serve as the basis for solving this problem for other projects.

The specific recording modules:

> set_utmp_entry  (struct logindata l);
> set_utmpx_entry (struct logindata l);
> set_wtmp_entry  (struct logindata l);

would probably still be fairly heavily preprocessed. Most of
the #ifdef mess in login.c is to work around differences in
struct utmp.

> 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.

Don't underestimate the first either - I find C code much easier to
understand than complex autoconf/makefile interactions.

> 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 would be happier sacrificing easy 'diffability' on one source file
in return for better code.

login.c is hardly ever touched by the OpenBSD people, the last real
change to it was in August. Keeping track of this pace of change
will not be a problem :)

My goal is to have a 1.2.1.0 release before, or shortly after the 
new year. I think that major surgery to the login code should wait 
until after then.

Regards,
Damien

- --
| "Bombay is 250ms from New York in the new world order" - Alan Cox
| Damien Miller - http://www.mindrot.org/
| Email: djm at mindrot.org (home) -or- djm at ibs.com.au (work)


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.0.0 (GNU/Linux)
Comment: For info see http://www.gnupg.org

iD8DBQE4Z/z5ormJ9RG1dI8RAkdKAKCO2XzYtXq2yUuj9ob9p5Msvz+WZwCeL7g+
BeXnqHdlDlFtd1GOzPRYyhA=
=dXL1
-----END PGP SIGNATURE-----