[Toybox] [CLEANUP] login.c

[Rob Landley]

I've been needing to do a proper cleanup pass on login forever, and
finally got around to it.

 toys/other/login.c |  246 ++++++++++++++++---------------------------
 1 file changed, 94 insertions(+), 152 deletions(-)

My notes, if you're curious:

principle: fewer lines means you can see more code at once.
  - keep more code in your head.
  adjacent code gives more opportunities to:
    combine redundant code
    remove transitional plumbing (data marshaling!)
    remove syntactic sugar (int, int, int; function call {})
      - trivial but they add up
    identify constant context. (Checks for zero that can't happen.)
  Use your framework properly.
    All code runs in a framework. Posix libc is a framework.
      Don't write your own strcmp() or basename() without good reason.
    Your selected set of shared libraries adds to your framework.

inline forbid[] environment variable array
  - more a code style thing, globals in union but zeroed, can't init.
    so init from main. Globals _outside_ that union generally wasted space,
    eliminate them where possible.

looking at login_main() variable declarations:
  f_flag becomes ff, h_flag becomes hh, more all three ints on same line.

toys.optc == 1 becomes !toys.optc because up top we >1 so it's zero or not.
  Testing zero/nonzero is cheapest test

"not root" test: TOYFLAG_NEEDROOT, infrastructure does it for us.

Style thing:
  Null terminator vs sizing array
  Not: subtle thing! repurposed count so made loop decrement to end with
  count == 0. removed count=0 initialization at declaration, later code
  depends on this.

inline set_environment()
  one caller, inline it

inline spawn_shell()
  one caller, inline it

inline handle_motd()
  problem: readall can return -1, don't write zero to array[-1].
    fix: just memset(toybuf). Not enough runtime to care, saves us a variable
    declaration, probably a wash size-wise. (save return value, dereference
    variable name, assign constant.) and it avoids testing for -1 (which
    should never happend and is just "print nothing" anyway.)

  problem: up top we continued if pid 1 or pid 2 were the tty, but here
  syslog is hardwired ttyname(0)? Change first check to loop and save which
  fd had the tty.

goto query_pass unnecessary, stuff above it can be an if () {}

in syslog(LOG_WARNING): move space to after "from ", cosmetic but otherwise
two spaces in !tty case.

Question: why do we reload getpwnam() each time through the loop? Is it
going to change? (Related: why do we clear f_flag = 0 after the
verify_password() attempt which we can only reach if we couldn't load
it or if it was locked? Will we load _different_ data out of the password
file on the second pass?

Ah, I see. It does prompt you for username each time through the loop
if you didn't specify one with -f, and -f doesn't take a normal colon-style
option argument, so not just "-f username", you could presumably do
"-f -p username" although WHY...? And would that work? Try ubuntu's...
"login -f -p landley" isn't an error, but it ignores the -f.
"login -p -f landley" works as you'd expect. So... right. Let's make it "f:"
in optargs and untangle this craziness. That means we don't actually need
to check f_flag at all, we just need to check the TT.username isn't null,
and we can assign 0 to it if we need to prompt for username next time around.

Inline verify_password()
  Checking the same conditions twice. (pass[0] == '!' and '*', both before
  and _in_ verify_password. Becomes obvious when inlined.)

Why don't we move the alarm() call _into_ the loop? Otherwise all
_three_ login attempts have a combined 60 seconds...

Do we ever write to utmp? The man page says we should. Also this
ENV=VAL command line stuff might be good to implement. And it says
that "*" means chroot into the home directory, which sounds containerish.
(All three are TODO items.)

Getting back to handle_motd(), we have a readfile() that does the open/close
of the file for is, and error checking. It can either take an existing buffer
and length, or check the file length and malloc an appropriate buffer.
So A) we should be using the more appropriate library function, B) should
we arbitrarily limit motd length? (Advantage: fewer mallocs on nommu system.
Disadvantage: undocumented length limit. Don't like length limit, malloc won.)

(Note to self: readfile() is wrapper around readfileat(), make readfile()
always malloc and use readfileat() directly if you want to supply buf.
Not doing it now because touches a lot of other callers. Another TODO.)

HOSTNAME_SIZE and USERNAME_MAX_SIZE don't have any benefits for being
factored out, it just separates the (only) user from the definition, so
hardwire the constant into the array and sizeof() the array if we need to.
And while we're at it, why are we copying strings into a fixed sized buffer
when we can just keep a pointer to the currently active string?

Query: what does -h _do_ exactly? The man page seems to have some sort of
mixup with -r (for rlogin protocol)...? We're both printing the -h value
into the log _and_ querying the local gethostname() to display to the user?
Wha...?

When prompting for username, there's a strange getchar() loop that
checks for EOF (except if you drop connection _after_ the first character
fgets() needs to detect this so that's gotta be redundant), and
we eat leading spaces. Why do we do that?

Hmmm, if you just hit enter at the login prompt it reprompts endlessly,
count doesn't apply to that and probably should. Adjust the outer loop to
be for (count = 0; count < 3; count++) and _that_ means the tricksy
do blah[--count]; while (count); loop no longer has to look like that to
end with count 0, so make it a more conventional for loop (not more efficient
code, but more conventional and thus easier to read code).

I'm not actually sure why login exits anyway. Presumably long ago on
nommu pdp-11 systems there was overhead to having multiple login processes
always running so they wanted them to only run while they had something to do?
Dunno. Ctrl-D (force EOF) can kill it anyway...

Having spwd (the shadow password stuff) live at the top level, get zeroed,
and get zeroed again at the end of the loop is silly. It only needs to
live for like 2 lines then get the one field we care about harvested.

 1438557222.0