On Mon, Apr 25, 2011 at 03:58:45PM +0200, Sami Kerola wrote:
> from Karel Zak <kzak@xxxxxxxxxx>
> date Thu, Apr 14, 2011 at 13:58
> subject Re: [PATCH 1/8] agetty: use xalloc.h
>
> I have removed tailing whitespace and fixed identation (by '='
> vim command ...), but it's still not enough. It would be nice to
> clean up the code according to kernel Documentation/CodingStyle.
> -- snip
>
> The scubbing consists;
>
> o Indentation changes.
> o Removal of void casting.
> o Removal of braces from single statements.
> o Unified format for comments.
> o Comparison with W. Venema's code from 1993 to clean few comment
> mysteries.
> o Old school introduction back to the top of the source file.
> o Clean up of comments and/or marking them up with FIXME where
> attention is needed.
> o Web referrals added to further explanation to near the code or
> comments which some could find difficult to understand why.
Cool!
> +/* Displayed before the login prompt. */
> #ifdef SYSV_STYLE
> -#define ISSUE "/etc/issue" /* displayed before the login prompt */
It would be better to move this path to include/pathnames.h
> #define P_(s) s
> int main P_((int argc, char **argv));
> -void parse_args P_((int argc, char **argv, struct options *op));
> -void parse_speeds P_((struct options *op, char *arg));
> +void parse_args P_((int argc, char **argv, struct options * op));
> +void parse_speeds P_((struct options * op, char *arg));
> void update_utmp P_((char *line));
> -void open_tty P_((char *tty, struct termios *tp, int local));
> -void termio_init P_((struct termios *tp, struct options *op));
> -void auto_baud P_((struct termios *tp));
> -void do_prompt P_((struct options *op, struct termios *tp));
> -void next_speed P_((struct termios *tp, struct options *op));
> -char *get_logname P_((struct options *op, struct chardata *cp, struct termios *tp));
> -void termio_final P_((struct options *op, struct termios *tp, struct chardata *cp));
> +void open_tty P_((char *tty, struct termios * tp, int local));
> +void termio_init P_((struct termios * tp, struct options * op));
> +void auto_baud P_((struct termios * tp));
> +void do_prompt P_((struct options * op, struct termios * tp));
> +void next_speed P_((struct termios * tp, struct options * op));
> +char *get_logname
> +P_((struct options * op, struct chardata * cp, struct termios * tp));
> +void termio_final
> +P_((struct options * op, struct termios * tp, struct chardata * cp));
Please, remove the P_() thing and use "static" for the functions.
> int main(argc, argv)
> - int argc;
> - char **argv;
> +int argc;
> +char **argv;
... in 21st century:
int main(int argc, char **argv);
> case 'I':
> - {
> - char ch, *p, *q;
> - int i;
> -
> - op->initstring = xmalloc(strlen(optarg) + 1);
> -
> - /* copy optarg into op->initstring decoding \ddd octal
> - * codes into chars
> - */
> - q = op->initstring;
> - p = optarg;
> - while (*p) {
> - if (*p == '\\') { /* know \\ means \ */
> - p++;
> + {
> + char ch, *p, *q;
> + int i;
> +
> + op->initstring = xmalloc(strlen(optarg) + 1);
> +
> + /*
> + * Copy optarg into op->initstring decoding \ddd octal
> + * codes into chars.
> + */
It would be better to use a separate function for this task. It's almost
always better when the argv[] parsing (the switch() block) in main() is
simple and readable. Please, use a separate patch for this change.
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
