On Thu, Apr 28, 2011 at 04:56:29PM +0200, Werner Fink wrote:
> More code cleanup, that is use bit mask for eight bit option, use
I can imagine a separate patch for the eight-bit-option change :-)
> modern speed_t type, split local error() into local error(), warn(),
> and dolog() for fine graduated logging with syslogger.
The names "error" and "warn" are too generic (used in our c.h and
glibc err.h). It would be better to use log_err() and log_warn().
> +#undef warn
> +#undef error
mess :-(
> /* Let the login program take care of password validation. */
> execl(options.login, options.login, "--", logname, NULL);
> - error(_("%s: can't exec %s: %m"), options.tty, options.login);
> + warn(_("%s: can't exec %s: %m"), options.tty, options.login);
> + sleep(5);
> + exit(EXIT_FAILURE);
> }
Why not error() where is already sleep() and exit()?
Why we need any sleep() before exit at all? IMHO init(8) should not
rely on such odd "optimization".
Please, use "return" rather than exit() in main().
> +static void parse_args(int argc, char **argv, struct options *op)
> {
> extern char *optarg;
> extern int optind;
> - int c;
> + int c, nipple = 0, index;
>
> enum {
> VERSION_OPTION = CHAR_MAX + 1,
> @@ -360,12 +366,14 @@ void parse_args(int argc, char **argv, struct options *op)
> { NULL, 0, 0, 0 }
> };
>
> - while ((c =
> - getopt_long(argc, argv, "8cf:hH:iI:l:Lmnst:Uw", longopts,
> - NULL)) != -1) {
> + while ((c = getopt_long(argc, argv, "8cf:hH:iI:l:Lmnst:Uw", longopts,
> + &index)) != -1) {
Where in the code we need the "index" variable?
> switch (c) {
> + case 0:
> + op->flags |= nipple;
> + break;
Why? The flag in the libc struct option is always zero, so
getopt_long() does not return 0. This change seems unnecessary.
> case '8':
> - op->eightbits = 1;
> + op->flags |= F_EIGHTBITS;
> break;
> case 'c':
> op->flags |= F_KEEPCFLAGS;
> @@ -467,7 +475,7 @@ void parse_args(int argc, char **argv, struct options *op)
> debug("after getopt loop\n");
>
> if (argc < optind + 2) {
> - warnx(_("not enough arguments"));
> + warn(_("not enough arguments"));
oh.. good catch!
> +static void dolog(int priority, const char *fmt, va_list ap)
> {
> - va_list ap;
> #ifndef USE_SYSLOG
> int fd;
> #endif
> @@ -1273,7 +1276,9 @@ void error(const char *fmt, ...)
> * %m expansion is done here as well. Too bad syslog(3) does not have a
> * vsprintf() like interface.
> */
> - va_start(ap, fmt);
> +#ifdef __linux__
> + vsnprintf (bp, sizeof(buf)-strlen(buf), fmt, ap);
> +#else
> while (*fmt && bp < &buf[BUFSIZ - 1]) {
> if (strncmp(fmt, "%s", 2) == 0) {
> xstrncpy(bp, va_arg(ap, char *), &buf[BUFSIZ - 1] - bp);
^^^^^^^
You cannot remove va_start() and va_end(), you have to call in within
the #else block.
> @@ -1288,7 +1293,7 @@ void error(const char *fmt, ...)
> }
> }
> *bp = 0;
> - va_end(ap);
> +#endif
Anyway, I think that we can assume vsnprintf() everywhere, so the
#ifdef __linux__ does not make sense here. Let's use only vsnprintf()
and remove the old SystemV code.
> +static void error(const char *fmt, ...)
> +static void warn(const char *fmt, ...)
__attribute__ ((__format__ (__printf__, 1, 2)));
Karel
--
Karel Zak <kzak@xxxxxxxxxx>
http://karelzak.blogspot.com
--
To unsubscribe from this list: send the line "unsubscribe util-linux" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
