Suggestion: login.c->record_login()

[Andre Lucas]

How on Earth does breaking out stuff OpenSSH already does into smaller,
more manageable chunks inhibit portability? I would suggest that the
opposite applies.

You have OS specific stuff because, well, this stuff is OS specific.
Then you have autoconf make a best guess if you don't know the OS.
That's the same as it is now, BTW, except that because it's trying to be
general it doesn't always get it right, even on the platforms it
directly supports.

Having autoconf guess at OSs that you really care about supporting well,
and can therefore program special cases for, is pointless IMO. I know I
care a great deal about having logins registered properly on a telnet,
rlogin and rcp replacement program, more than I care about philosophical
arguments regarding autoconf that mean it doesn't work correctly.

Look at configure.in for any large package, I guarantee it will be full
of special cases. That's why it's there. Likewise, any package that gets
'down to the metal' has chunks of highly OS specific code. tcpdump
(actually libpcap) springs to mind here, where it interfaces with
different OS' Berkeley Packet Filter equivalents.

Even with the platform hacks, tcpdump is a great program that people use
even if their OS has an equivalent utility, e.g. snoop. That's the kind
of pragmatic approach that I think OpenSSH has to take to win hearts and
minds over for-profit SSH in The Real World(tm), and if that means
putting in specific code for Linux, Solaris, AIX, HPUX, Digital, NetBSD
and whatever else, well, so what?

In any case, I'm not talking about removing autoconf's wonderful 'best
guess' functionality. I'm talking about a code cleanup that will enhance
it.

Regards,
-Andre

David Rankin wrote:
> 
> 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