On Sun, Oct 04, 2009 at 10:38:39PM +0200, Sami Kerola wrote:
> As a minor change this patch does include new error messages.
Thanks, some notes:
> int maxlength; /* longest record */
> @@ -87,6 +110,7 @@ main(int argc, char **argv)
> int ch, tflag, xflag;
> char *p;
>
> + eval = EXIT_SUCCESS;
> extern char *__progname;
> __progname = argv[0];
The proper way is to use #ifdef HAVE__PROGNAME, but I think we
needn't this __progname legacy at all. (The same thing I see in
cal.c.)
> if (ioctl(1, TIOCGWINSZ, &win) == -1 || !win.ws_col) {
> if ((p = getenv("COLUMNS")) != NULL)
> - termwidth = atoi(p);
> + termwidth = xstrtoi(p);
> } else
> termwidth = win.ws_col;
>
> tflag = xflag = 0;
> - while ((ch = getopt(argc, argv, "c:s:tx")) != -1)
> + while ((ch = getopt_long(argc, argv, "c:hs:tx", longopts, NULL)) != -1)
> switch(ch) {
> case 'c':
> - termwidth = atoi(optarg);
> + termwidth = xstrtoi(optarg);
It seems you want to use xstrtoi() as a generic function, then it
would be nice to have a "mesg" argument
termwidth = xstrtoi(p, _("COLUMNS variable"));
termwidth = xstrtoi(optarg, _("width argument"));
and use the argument in the xstrtoi error message.
> maketbl()
> {
> @@ -251,7 +271,7 @@ maketbl()
> * sizeof(wchar_t *))) ||
> !(lens = realloc(lens, ((u_int)maxcols + DEFCOLS)
> * sizeof(int))))
> - err(1, NULL);
> + err(EXIT_FAILURE, NULL);
err(EXIT_FAILURE, _("malloc failed"));
> if (!(p = malloc(size)))
> - err(1, NULL);
> + err(EXIT_FAILURE, _("malloc"));
"malloc failed"
BTW, the code is not consistent -- somewhere we have emalloc() (check
for malloc errors) and somewhere we have malloc(). It would be nice to rename
emalloc() to xmalloc() and use it everywhere. The other stupid thing
is
p = malloc()
memset(p, 0, ...)
when we have calloc(). Maybe you can split the column(1) clean up to
more patches (long options, err() stuff, malloc() stuff).
> +usage(int rc)
> +{
> + extern char *program_invocation_short_name;
I think this is unnecessary.
> + const char *p = program_invocation_short_name;
> + if (!*p) {
> + p = "column";
> + }
> +
> + printf(_("\nUsage: %s [options] [file ...]\n"), p);
> + printf(_("\nOptions:\n"));
> +
> + printf(_(
> + " -c, --width=SIZE output is formatted for a display of SIZE wide\n"
> + " -s, --separator=SEP character to be used to delimit columns\n"
> + " -t, --table create a table\n"
> + " -x fill columns before filling rows\n"
> + " -h, --help display this help text\n"));
> +
> + printf(_("\nFor more information see column(1).\n"));
> + exit(rc);
> +}
I don't have a strong opinion about it, but sometimes people assume
usage() output on stderr if the return code is EXIT_FAILURE. See for
example sys-utils/fallocate.c.
The fatal bug is that I don't see the column.1 man page update :-)
> +signed int
> +xstrtoi(const char *optarg)
> {
> + signed long val;
> + char *endp;
> +
> + errno = 0;
> + val = strtol(optarg, &endp, 10);
> + if ((errno == ERANGE || (val < 0) || (INT_MAX < val)) ||
> + (errno != 0 && val == 0)) {
> + xerr(EXIT_FAILURE, _("width argument out of range"));
> + }
> + if (optarg == endp) {
> + xerr(EXIT_FAILURE, _("digit missing from width argument"));
> + }
>
> - (void)fprintf(stderr,
> - _("usage: column [-tx] [-c columns] [file ...]\n"));
> - exit(1);
> + return (signed int) val;
> +}
> +
> +static void
> +xerr(int rc, char *s)
> +{
> + extern char *program_invocation_short_name;
> + const char *p = program_invocation_short_name;
> + if (!*p) {
> + p = "column";
> + }
> + fprintf(stderr, "%s: %s\n", p, s);
> + exit(rc);
> }
Why we need xerr() function? It seems like a duplicate to the errx()
function from err.h.
Karel
--
Karel Zak <kzak@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe util-linux-ng" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
