[PATCH] sulogin size reduction and new logging

[Tito]

On Saturday 9 September 2006 15:59, you wrote:
> On Friday 08 September 2006 00:40, Tito wrote:
> > Hi,
> > this patch reduces the size of sulogin
> > and moves it to the new syslog mode.
> > 
> > Review is as always welcome.
> 
> -#define SULOGIN_PROMPT "\nGive root password for system maintenance\n" \
> +#define SULOGIN_PROMPT "Give root password for system maintenance\n" \
>         "(or type Control-D for normal startup):"
> 
> If it is used just once, #define seems silly.

For me it make s the code more readable as you don't need to store 
2 lines of text in it.

> 
> +       if (bb_getopt_ulflags (argc, argv, "t:", &timeout_arg)) {
> +               if (safe_strtoi(timeout_arg, &timeout)) {
> +                       timeout = 0;
>                 }
>         }
> 
> Where is support for -f and -h? We say that we have it...
> 
> # busybox sulogin --help
> BusyBox v1.2.1 (2006.08.29-06:37+0000) multi-call binary
> Usage: sulogin [OPTION]... [tty-device]
> Single user login
> Options:
>         -f      Do not authenticate (user already authenticated)
>         -h      Name of the remote host for this login
>         -p      Preserve environment
> 
> Although it wan't there in the first place... not your fault.

I didn't notice it because I don't use verbose usage.
I think it is best is to rip -h (collision with -h like help) and -p out of the help text,
unless you ant to implement them.
In this case i would suggest to change -h to -H.
 
>  
> +       if (ENABLE_FEATURE_SYSLOG) {
> +               logmode = LOGMODE_BOTH;
> +               openlog(bb_applet_name, LOG_CONS | LOG_NOWAIT, LOG_AUTH);
> +       }
> 
> LOG_CONS? Do you want double lines to appear on console
> if syslogd isn't running?

Feel free to fix the flags.

> FEATURE_SYSLOG is not what you think it is. It is merely
> a build-time tool to avoid using syslog() in bb_verror_msg()
> if none of selected applets uses it (think about bbox with
> only 'cp' applet compiled in...)
> 
> You use it like this: if applet uses syslog, then add
> "select CONFIG_FEATURE_SYSLOG" into relevant Config.in.
> Example:
> 
> config CONFIG_SU
>         bool "su"
>         default n
>         select CONFIG_FEATURE_SUID
>         select CONFIG_FEATURE_SYSLOG
>         help
>           su is used to become another user during a login session...
> 
> Do not use xx_FEATURE_SYSLOG in *.c file.

#define CONFIG_FEATURE_SYSLOG 1
#define ENABLE_FEATURE_SYSLOG 1
#define USE_FEATURE_SYSLOG(...)  __VA_ARGS__
#define SKIP_FEATURE_SYSLOG(...)

If it is like this is a somewhat twisted logic vs what we do for all other flags
and will create trouble because people like me will try to use it 
the same way of all other flags.
 
> If you want to make applet's ability to print to syslog configurable,
> then you need separate CONFIG_xxx. See CONFIG_FEATURE_UDHCP_SYSLOG.

Don't think Rob will like this one flag per applet policy......
I would move this to the general setup and allow the enable/disable
all logging and relative code at once

> 
> +       if (argv[optind]) {
> +               close(0);
> +               close(1);
> +               close(2);
> +               dup(xopen(argv[optind], O_RDWR));
> +               dup(0);
>         }
> +
>         if (!isatty(0) || !isatty(1) || !isatty(2)) {
> -               exit(EXIT_FAILURE);
> +               bb_error_msg_and_die("Not a tty");
>         }
> 
> Just think where writes to fd 2 will go. Maybe to file with
> filename argv[optind]... do we really want that?

You are right.

> I will fix it now in svn but a better solution is needed.
> fd = xopen(); if(!isatty(fd)) error_and_die; close close close dup dup dup?
> Otherwise user may be left quite puzzled ("why sulogin dies silently?").
> 
> bb_[p]error prints "<appletname>: <message>\n". Starting <message>
> with capital letter is not correct wrt English language.
> I am nitpicking here...

I see messages starting with capital letters in syslog and terminals 
all the time....

Sep  9 16:13:44 localhost pppd[5254]: Using interface ppp0
Sep  9 16:13:44 localhost pppd[5254]: Connect: ppp0 <--> /dev/ttyS0

> 
> +       if (!(pwd = getpwuid(0))) {
> 
> This one is small, but try to avoid if(... a=expr .... ) if expr is complex.
> Example from crond.c:
> if ((logfd = open(LogFile, O_WRONLY | O_CREAT | O_APPEND, 0600)) >= 0) ....
> Awwww my eyes....

Maybe this code is smaller if the compiler is not so smart to optimise it to 
the same code..........and it creates less lines so more code fits in your screen at the same time

logfd = open(LogFile, O_WRONLY | O_CREAT | O_APPEND, 0600);
if (logfd >= 0)

>  
> +                       bb_info_msg("Normal startup");
> 
> As opposed to error_msg, this is right. info_msg does NOT print applet name,
> capital letter is ok.

;-)

> 
> +AUTH_ERROR:    
> +       bb_error_msg_and_die("No password entry for `root'");
>  }
> 
> I think labels are usually lowercase.

I have seen them usually UPPERCASE in most bb code and personally think
that so they are easier to spot, but that is a matters of taste. 
> 
> Please see current svn, I did some minor changes to sulogin
> after your patch was applied.
> --
> vda
> 

Will do ASAP.

Ciao,
Tito