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