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. _______________________________________________ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net