[PATCH] sulogin size reduction and new logging

Denis Vlasenko vda.linux at googlemail.com
Sat Sep 9 13:59:15 UTC 2006


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.


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

 
+       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?

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.
If you want to make applet's ability to print to syslog configurable,
then you need separate CONFIG_xxx. See CONFIG_FEATURE_UDHCP_SYSLOG.

+       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?

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


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

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


Please see current svn, I did some minor changes to sulogin
after your patch was applied.
--
vda



More information about the busybox mailing list