Mailing List Archive: Suggestion: login.c->record

Suggestion: login.c->record_login()

Dec 27, 1999, 9:51 AM

Post #1 of 5 (1529 views)

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

Re: Suggestion: login.c->record_login() [ In reply to ]

djm at mindrot

Dec 27, 1999, 3:57 PM

Post #2 of 5 (1472 views)

Permalink

-----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@mindrot.org (home) -or- djm@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-----

Re: Suggestion: login.c->record_login() [ In reply to ]

andre.lucas at dial

Dec 27, 1999, 4:16 PM

Post #3 of 5 (1471 views)

Permalink

Damien Miller wrote:
>
> -----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.

Ok, it was stating the obvious somewhat :-)

> <snip>

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

I thought that too. It's real hassleware, but some poor soul has to do
it!

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

Ah, if only people at my work thought that way...

Ta,
-Andre

>
> 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@mindrot.org (home) -or- djm@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-----

Re: Suggestion: login.c->record_login() [ In reply to ]

drankin at bohemians

Dec 27, 1999, 9:39 PM

Post #4 of 5 (1471 views)

Permalink

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

Re: Suggestion: login.c->record_login() [ In reply to ]

andre.lucas at dial

Dec 28, 1999, 3:57 AM

Post #5 of 5 (1465 views)

Permalink

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