Suggestion: login.c->record_login()

[David Rankin]

The problem with a "OS-based" ifdef system is that it makes porting
to a new OS problematic. Also, what happens if FooNix or BarBSD decide
to add/subtract/change the way they use struct utmp/utmpx/et al.? Under
an OS-based ifdef, configure "figures out" the messy details for
you (hopefully the right way), while this setup would require manual
intervention to go to either older supported OSes or newer versions.

For the record, I think this "configure soup" stinks. But I also think
that this would make things less portable, not more.

Thanks,
David

On Mon, Dec 27, 1999 at 05:51:03PM +0000, 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.

> 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
-- 
David W. Rankin, Jr.     Husband, Father, and UNIX Sysadmin. 
   Email: drankin at bohemians.lexington.ky.us   Address/Phone Number: Ask me.
"It is no great thing to be humble when you are brought low; but to be humble
when you are praised is a great and rare accomplishment." St. Bernard