[PATCH] correct_passwd and sulogin size reduction

[Rob Landley]

On Wednesday 01 February 2006 02:32, Bernhard Fischer wrote:
> On Sat, Jan 21, 2006 at 10:21:21PM +0100, Tito wrote:
> >Hi,
> >this patch set contains a new GPL'ed  version of correct_password.c
> >with reduced size and more flexibility so that it can be used in more
> > apps.
>
> I'd prefer to wait for Rob to tag 1.1.1 and only afterwards check this
> in, unless someone else ack's this for 1.1.1.

The lack of salt is something we really need to fix for 1.1.1.  Now that I 
understand the layout of the thing, it seems ok.  I just haven't had time to 
finish reviewing it.  (I miss my Austin coffee shop.)

> >+ char *clear;
> >+ const char *crypt_pw;
>
> -> #ifdef CONFIG_FEATURE_SHADOWPASSWDS
>
> #if ENABLE_FEATURE_SHADOWPASSWDS

if (ENABLE_FEATURE_SHADOWPASSWDS) {

> >- if (( strcmp ( pw-> pw_passwd, "x" ) == 0 ) || ( strcmp ( pw->
> > pw_passwd, "*" ) == 0 )) { -  struct spwd *sp = getspnam ( pw-> pw_name
> > );

> >  if (access(bb_path_passwd_file, 0) == -1) {
>
> Personally i prefer to test against !(access()) in these situations,
> fwiw.

Yeah, if something returns zero for success and nonzero for failure, I try not 
to assume I know what the nonzero will be.  (In this case, the man page is 
specific, but still...)

> >-  bb_do_delay(FAIL_DELAY);
> >-  puts("Login incorrect");
> >+  printf("%s", SULOGIN_PROMPT);
> >   fflush(stdout);
>
> perhaps bb_xfflush_stdout()

You know, I still think the whole FILE * buffering is way more trouble than 
it's worth, except in the few cases where we actually know we'd be likely to 
do byte-at-a-time out otherwise.  (A FILE * structure is bigger than an fd, 
it's an instance of dynamic allocation we don't need, keep track of when it 
has and hasn't been flushed is a pain...)

Oh well.

> is the switch smaller than a temporary var and if else here?
> (perhaps reuse "timeout" ...)

I've found that if/else staircase is generally smaller when I try it (although 
that could be a compiler version artifact), and it's also a _lot_ more 
friendly to if(ENABLE).  Just a random comment...

Rob
-- 
Steve Ballmer: Innovation!  Inigo Montoya: You keep using that word.
I do not think it means what you think it means.