On Fri, Apr 29, 2011 at 11:14:26AM +0200, Karel Zak wrote:
> 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().
OK
>
> > +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.
This is how getopt_long() work. It returns tzhe value if flag
is NULL and 0 otherwise. I'm using flag therefore if retrn 0
here. This is what the manual page states and I've tested.
>
> > 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.
Hmmm --- IMHO it can be done as I use va_start() and va_end()
in the functions error() and warn() which are calling dolog()
during debugging those was used very heavily ;)
>
> > @@ -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)));
that is already done in the declaration at the head of agetty.c
there is no need to tdo it twice.
Werner
--
Dr. Werner Fink -- Software Engineer Consultant
SuSE LINUX Products GmbH, Maxfeldstrasse 5, Nuernberg, Germany
GF: Markus Rex, HRB 16746 (AG Nuernberg)
phone: +49-911-740-53-0, fax: +49-911-3206727, www.opensuse.org
------------------------------------------------------------------
"Having a smoking section in a restaurant is like having
a peeing section in a swimming pool." -- Edward Burr
--
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
