[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