Jim Meyering <[EMAIL PROTECTED]> writes: > The patches are probably ok, but I won't have time to deal with > this (adding the tests I mentioned) for a couple more days. > I prefer not to leave gnulib and coreutils out of sync like that.
I finally got to this (sorry about the delay), and have some thoughts. First, human_options should probably not output to stderr or exit, but should instead let the caller do it. That's more library-like. Second, the strncpy bothered me a bit; I would rather not have arbitrary limits like that even if they are justifiable due to our knowledge of the longest option length. I came up with a different way to do that, without the limit, but with another issue, namely, the options to be prefixed with "--" need to be marked. Here's a proposed patch to address both problems. It comes in two parts: the first to gnulib, the second to coreutils. I haven't installed the gnulib part. Here's the gnulib patch, in vc-dwim format: * NEWS: Describe interface changes to human, xstrtol. * lib/human.h: Include <xstrtol.h>. (human_options): Return enum strtol_error, not int. Remove bool arg; take int * instead. * lib/human.c: Don't include "gettext.h". (_): Remove; no longer used. Don't include <xstrtol.h>, since human.h does it. (human_options): Adjust to abovementioned interface changes. Do not report error to stderr; that's now the caller's responsibility. * lib/xstrtol.c (main) [defined TESTING_XSTRTO]: Adjust to interface change. * lib/xstrtol.h (_STRTOL_ERROR): Take Option, Arg rather than Str, Argument_type_string. All uses changed. Put " argument" in diagnostics to make them clearer. Change wording of suffix message for clarity. (STRTOL_FATAL_ERROR): Take Option, Arg rather than Str, Argument_type_string. (STRTOL_FATAL_WARN): Remove; no longer used. * modules/human (Depends-on): Remove gettext-h. Index: NEWS =================================================================== RCS file: /cvsroot/gnulib/gnulib/NEWS,v retrieving revision 1.26 diff -u -p -r1.26 NEWS --- NEWS 26 Jul 2007 08:28:56 -0000 1.26 +++ NEWS 4 Aug 2007 07:33:45 -0000 @@ -6,6 +6,17 @@ User visible incompatible changes Date Modules Changes +2007-08-04 human The function human_options no longer reports an + error to standard output; that is now the + caller's responsibility. It returns an + error code of type enum strtol_error + instead of the integer option value, and stores + the option value via a new int * argument. + xstrtol The first two arguments of STRTOL_FATAL_ERROR + are now an option name and option argument + instead of an option argument and a type string, + STRTOL_FAIL_WARN is removed. + 2007-07-14 gpl, lgpl New Texinfo versions with no sectioning commands. 2007-07-10 version-etc Output now mentions GPLv3+, not GPLv2+. Use Index: lib/human.h =================================================================== RCS file: /cvsroot/gnulib/gnulib/lib/human.h,v retrieving revision 1.17 diff -u -p -r1.17 human.h --- lib/human.h 26 Jul 2007 08:28:56 -0000 1.17 +++ lib/human.h 4 Aug 2007 07:33:45 -0000 @@ -1,7 +1,7 @@ /* human.h -- print human readable file size Copyright (C) 1996, 1997, 1998, 1999, 2000, 2001, 2002, 2003, 2004, - 2005, 2006 Free Software Foundation, Inc. + 2005, 2006, 2007 Free Software Foundation, Inc. This program is free software; you can redistribute it and/or modify it under the terms of the GNU General Public License as published by @@ -27,6 +27,8 @@ # include <stdint.h> # include <unistd.h> +# include <xstrtol.h> + /* A conservative bound on the maximum length of a human-readable string. The output can be the square of the largest uintmax_t, so double its size before converting to a bound. @@ -78,6 +80,6 @@ enum char *human_readable (uintmax_t, char *, int, uintmax_t, uintmax_t); -int human_options (char const *, bool, uintmax_t *); +enum strtol_error human_options (char const *, int *, uintmax_t *); #endif /* HUMAN_H_ */ Index: lib/human.c =================================================================== RCS file: /cvsroot/gnulib/gnulib/lib/human.c,v retrieving revision 1.36 diff -u -p -r1.36 human.c --- lib/human.c 26 Jul 2007 08:28:56 -0000 1.36 +++ lib/human.c 4 Aug 2007 07:33:45 -0000 @@ -1,7 +1,7 @@ /* human.c -- print human readable file size Copyright (C) 1996, 1997, 1998, 1999, 2000, 2001, 2002, 2003, 2004, - 2005, 2006 Free Software Foundation, Inc. + 2005, 2006, 2007 Free Software Foundation, Inc. This program is free software; you can redistribute it and/or modify it under the terms of the GNU General Public License as published by @@ -28,13 +28,9 @@ #include <stdlib.h> #include <string.h> -#include "gettext.h" -#define _(msgid) gettext (msgid) - #include <argmatch.h> #include <error.h> #include <intprops.h> -#include <xstrtol.h> /* The maximum length of a suffix like "KiB". */ #define HUMAN_READABLE_SUFFIX_LENGTH_MAX 3 @@ -463,17 +459,14 @@ humblock (char const *spec, uintmax_t *b return LONGINT_OK; } -int -human_options (char const *spec, bool report_errors, uintmax_t *block_size) +enum strtol_error +human_options (char const *spec, int *opts, uintmax_t *block_size) { - int opts; - strtol_error e = humblock (spec, block_size, &opts); + strtol_error e = humblock (spec, block_size, opts); if (*block_size == 0) { *block_size = default_block_size (); e = LONGINT_INVALID; } - if (e != LONGINT_OK && report_errors) - STRTOL_FATAL_ERROR (spec, _("block size"), e); - return opts; + return e; } Index: lib/xstrtol.c =================================================================== RCS file: /cvsroot/gnulib/gnulib/lib/xstrtol.c,v retrieving revision 1.41 diff -u -p -r1.41 xstrtol.c --- lib/xstrtol.c 8 Oct 2006 07:24:56 -0000 1.41 +++ lib/xstrtol.c 4 Aug 2007 07:33:45 -0000 @@ -1,7 +1,7 @@ /* A more useful interface to strtol. - Copyright (C) 1995, 1996, 1998, 1999, 2000, 2001, 2003, 2004, 2005, 2006 - Free Software Foundation, Inc. + Copyright (C) 1995, 1996, 1998, 1999, 2000, 2001, 2003, 2004, 2005, + 2006, 2007 Free Software Foundation, Inc. This program is free software; you can redistribute it and/or modify it under the terms of the GNU General Public License as published by @@ -254,7 +254,7 @@ main (int argc, char **argv) } else { - STRTOL_FATAL_ERROR (argv[i], "arg", s_err); + STRTOL_FATAL_ERROR ("arg", argv[i], s_err); } } exit (0); Index: lib/xstrtol.h =================================================================== RCS file: /cvsroot/gnulib/gnulib/lib/xstrtol.h,v retrieving revision 1.24 diff -u -p -r1.24 xstrtol.h --- lib/xstrtol.h 26 Jul 2007 08:28:56 -0000 1.24 +++ lib/xstrtol.h 4 Aug 2007 07:33:45 -0000 @@ -1,6 +1,6 @@ /* A more useful interface to strtol. - Copyright (C) 1995, 1996, 1998, 1999, 2001, 2002, 2003, 2004, 2006 + Copyright (C) 1995, 1996, 1998, 1999, 2001, 2002, 2003, 2004, 2006, 2007 Free Software Foundation, Inc. This program is free software; you can redistribute it and/or modify @@ -50,7 +50,13 @@ _DECLARE_XSTRTOL (xstrtoul, unsigned lon _DECLARE_XSTRTOL (xstrtoimax, intmax_t) _DECLARE_XSTRTOL (xstrtoumax, uintmax_t) -# define _STRTOL_ERROR(Exit_code, Str, Argument_type_string, Err) \ +/* Report an error for an out-of-range integer argument. + EXIT_CODE is the exit code (0 for a non-fatal error). + OPTION is the option that takes the argument + (usually starting with one or two minus signs). + ARG is the option's argument. + ERR is the error code returned by one of the xstrto* functions. */ +# define _STRTOL_ERROR(Exit_code, Option, Arg, Err) \ do \ { \ switch ((Err)) \ @@ -59,29 +65,26 @@ _DECLARE_XSTRTOL (xstrtoumax, uintmax_t) abort (); \ \ case LONGINT_INVALID: \ - error ((Exit_code), 0, gettext ("invalid %s `%s'"), \ - (Argument_type_string), (Str)); \ + error (Exit_code, 0, gettext ("invalid %s argument `%s'"), \ + Option, Arg); \ break; \ \ case LONGINT_INVALID_SUFFIX_CHAR: \ case LONGINT_INVALID_SUFFIX_CHAR | LONGINT_OVERFLOW: \ error ((Exit_code), 0, \ - gettext ("invalid character following %s in `%s'"), \ - (Argument_type_string), (Str)); \ + gettext ("invalid suffix in %s argument `%s'"), \ + Option, Arg); \ break; \ \ case LONGINT_OVERFLOW: \ - error ((Exit_code), 0, gettext ("%s `%s' too large"), \ - (Argument_type_string), (Str)); \ + error (Exit_code, 0, gettext ("%s argument `%s' too large"), \ + Option, Arg); \ break; \ } \ } \ while (0) -# define STRTOL_FATAL_ERROR(Str, Argument_type_string, Err) \ - _STRTOL_ERROR (exit_failure, Str, Argument_type_string, Err) - -# define STRTOL_FAIL_WARN(Str, Argument_type_string, Err) \ - _STRTOL_ERROR (0, Str, Argument_type_string, Err) +# define STRTOL_FATAL_ERROR(Option, Arg, Err) \ + _STRTOL_ERROR (exit_failure, Option, Arg, Err) #endif /* not XSTRTOL_H_ */ Index: modules/human =================================================================== RCS file: /cvsroot/gnulib/gnulib/modules/human,v retrieving revision 1.14 diff -u -p -r1.14 human --- modules/human 13 Oct 2006 12:40:23 -0000 1.14 +++ modules/human 4 Aug 2007 07:33:45 -0000 @@ -8,7 +8,6 @@ lib/human.c m4/human.m4 Depends-on: -gettext-h argmatch error intprops Here's the coreutils patch: * src/df.c (long_options): Prepend "--" to long options that OPT_STR might decode. * src/du.c (long_options): Likewise. * src/od.c (long_options): Likewise. * src/sort.c (long_options): Likewise. * src/df.c (main): Adjust to new human and xstrtol API. * src/du.c (main): Likewise. * src/ls.c (decode_switches): Likewise. * src/od.c (main): Likewise. * src/pr.c (first_last_page): Likewise. New argument OPTION. All callers changed. * src/sort.c (specify_sort_size): New arg OPTION. All callers changed. Adjust to new xstrtol API. * src/system.h (opt_str_storage): New static var. (OPT_STR, LONG_OPT_STR, OPT_STR_INIT): New macros. diff --git a/src/df.c b/src/df.c index 41bda87..c2d1180 100644 --- a/src/df.c +++ b/src/df.c @@ -121,7 +121,7 @@ enum static struct option const long_options[] = { {"all", no_argument, NULL, 'a'}, - {"block-size", required_argument, NULL, 'B'}, + {OPT_STR_INIT ("block-size"), required_argument, NULL, 'B'}, {"inodes", no_argument, NULL, 'i'}, {"human-readable", no_argument, NULL, 'h'}, {"si", no_argument, NULL, 'H'}, @@ -776,7 +776,6 @@ kB 1000, K 1024, MB 1000*1000, M 1024*1024, and so on for G, T, P, E, Z, Y.\n\ int main (int argc, char **argv) { - int c; struct stat *stats IF_LINT (= 0); initialize_main (&argc, &argv); @@ -798,16 +797,26 @@ main (int argc, char **argv) posix_format = false; exit_status = EXIT_SUCCESS; - while ((c = getopt_long (argc, argv, "aB:iF:hHklmPTt:vx:", long_options, NULL)) - != -1) + for (;;) { + int oi = -1; + int c = getopt_long (argc, argv, "aB:iF:hHklmPTt:vx:", long_options, + &oi); + if (c == -1) + break; + switch (c) { case 'a': show_all_fs = true; break; case 'B': - human_output_opts = human_options (optarg, true, &output_block_size); + { + enum strtol_error e = human_options (optarg, &human_output_opts, + &output_block_size); + if (e != LONGINT_OK) + STRTOL_FATAL_ERROR (OPT_STR (oi, c, long_options), optarg, e); + } break; case 'i': inode_format = true; @@ -873,8 +882,8 @@ main (int argc, char **argv) output_block_size = (getenv ("POSIXLY_CORRECT") ? 512 : 1024); } else - human_output_opts = human_options (getenv ("DF_BLOCK_SIZE"), false, - &output_block_size); + human_options (getenv ("DF_BLOCK_SIZE"), + &human_output_opts, &output_block_size); } /* Fail if the same file system type was both selected and excluded. */ diff --git a/src/du.c b/src/du.c index f9bd2e3..caacbb0 100644 --- a/src/du.c +++ b/src/du.c @@ -204,7 +204,7 @@ static struct option const long_options[] = { {"all", no_argument, NULL, 'a'}, {"apparent-size", no_argument, NULL, APPARENT_SIZE_OPTION}, - {"block-size", required_argument, NULL, 'B'}, + {OPT_STR_INIT ("block-size"), required_argument, NULL, 'B'}, {"bytes", no_argument, NULL, 'b'}, {"count-links", no_argument, NULL, 'l'}, {"dereference", no_argument, NULL, 'L'}, @@ -661,7 +661,6 @@ du_files (char **files, int bit_flags) int main (int argc, char **argv) { - int c; char *cwd_only[2]; bool max_depth_specified = false; char **files; @@ -692,12 +691,17 @@ main (int argc, char **argv) exclude = new_exclude (); - human_output_opts = human_options (getenv ("DU_BLOCK_SIZE"), false, - &output_block_size); + human_options (getenv ("DU_BLOCK_SIZE"), + &human_output_opts, &output_block_size); - while ((c = getopt_long (argc, argv, DEBUG_OPT "0abchHklmsxB:DLPSX:", - long_options, NULL)) != -1) + for (;;) { + int oi = -1; + int c = getopt_long (argc, argv, DEBUG_OPT "0abchHklmsxB:DLPSX:", + long_options, &oi); + if (c == -1) + break; + switch (c) { #if DU_DEBUG @@ -788,7 +792,12 @@ main (int argc, char **argv) break; case 'B': - human_output_opts = human_options (optarg, true, &output_block_size); + { + enum strtol_error e = human_options (optarg, &human_output_opts, + &output_block_size); + if (e != LONGINT_OK) + STRTOL_FATAL_ERROR (OPT_STR (oi, c, long_options), optarg, e); + } break; case 'D': /* This will eventually be 'H' (-H), too. */ diff --git a/src/ls.c b/src/ls.c index ee73622..064a51f 100644 --- a/src/ls.c +++ b/src/ls.c @@ -1450,8 +1450,8 @@ decode_switches (int argc, char **argv) { char const *ls_block_size = getenv ("LS_BLOCK_SIZE"); - human_output_opts = human_options (ls_block_size, false, - &output_block_size); + human_options (ls_block_size, + &human_output_opts, &output_block_size); if (ls_block_size || getenv ("BLOCK_SIZE")) file_output_block_size = output_block_size; } @@ -1793,8 +1793,13 @@ decode_switches (int argc, char **argv) break; case BLOCK_SIZE_OPTION: - human_output_opts = human_options (optarg, true, &output_block_size); - file_output_block_size = output_block_size; + { + enum strtol_error e = human_options (optarg, &human_output_opts, + &output_block_size); + if (e != LONGINT_OK) + STRTOL_FATAL_ERROR ("--block-size", optarg, e); + file_output_block_size = output_block_size; + } break; case SI_OPTION: diff --git a/src/od.c b/src/od.c index 3f2159e..472c513 100644 --- a/src/od.c +++ b/src/od.c @@ -1,5 +1,5 @@ /* od -- dump files in octal and other formats - Copyright (C) 92, 1995-2006 Free Software Foundation, Inc. + Copyright (C) 92, 1995-2007 Free Software Foundation, Inc. This program is free software: you can redistribute it and/or modify it under the terms of the GNU General Public License as published by @@ -281,14 +281,14 @@ enum static struct option const long_options[] = { - {"skip-bytes", required_argument, NULL, 'j'}, + {OPT_STR_INIT ("skip-bytes"), required_argument, NULL, 'j'}, {"address-radix", required_argument, NULL, 'A'}, - {"read-bytes", required_argument, NULL, 'N'}, + {OPT_STR_INIT ("read-bytes"), required_argument, NULL, 'N'}, {"format", required_argument, NULL, 't'}, {"output-duplicates", no_argument, NULL, 'v'}, - {"strings", optional_argument, NULL, 'S'}, + {OPT_STR_INIT ("strings"), optional_argument, NULL, 'S'}, {"traditional", no_argument, NULL, TRADITIONAL_OPTION}, - {"width", optional_argument, NULL, 'w'}, + {OPT_STR_INIT ("width"), optional_argument, NULL, 'w'}, {GETOPT_HELP_OPTION_DECL}, {GETOPT_VERSION_OPTION_DECL}, @@ -1555,7 +1555,6 @@ dump_strings (void) int main (int argc, char **argv) { - int c; int n_files; size_t i; int l_c_m; @@ -1609,11 +1608,14 @@ main (int argc, char **argv) address_pad_len = 7; flag_dump_strings = false; - while ((c = getopt_long (argc, argv, short_options, long_options, NULL)) - != -1) + for (;;) { uintmax_t tmp; enum strtol_error s_err; + int oi = -1; + int c = getopt_long (argc, argv, short_options, long_options, &oi); + if (c == -1) + break; switch (c) { @@ -1653,7 +1655,7 @@ it must be one character from [doxn]"), modern = true; s_err = xstrtoumax (optarg, NULL, 0, &n_bytes_to_skip, multipliers); if (s_err != LONGINT_OK) - STRTOL_FATAL_ERROR (optarg, _("skip argument"), s_err); + STRTOL_FATAL_ERROR (OPT_STR (oi, c, long_options), optarg, s_err); break; case 'N': @@ -1663,7 +1665,7 @@ it must be one character from [doxn]"), s_err = xstrtoumax (optarg, NULL, 0, &max_bytes_to_format, multipliers); if (s_err != LONGINT_OK) - STRTOL_FATAL_ERROR (optarg, _("limit argument"), s_err); + STRTOL_FATAL_ERROR (OPT_STR (oi, c, long_options), optarg, s_err); break; case 'S': @@ -1674,7 +1676,8 @@ it must be one character from [doxn]"), { s_err = xstrtoumax (optarg, NULL, 0, &tmp, multipliers); if (s_err != LONGINT_OK) - STRTOL_FATAL_ERROR (optarg, _("minimum string length"), s_err); + STRTOL_FATAL_ERROR (OPT_STR (oi, c, long_options), optarg, + s_err); /* The minimum string length may be no larger than SIZE_MAX, since we may allocate a buffer of this size. */ @@ -1746,7 +1749,8 @@ it must be one character from [doxn]"), uintmax_t w_tmp; s_err = xstrtoumax (optarg, NULL, 10, &w_tmp, ""); if (s_err != LONGINT_OK) - STRTOL_FATAL_ERROR (optarg, _("width specification"), s_err); + STRTOL_FATAL_ERROR (OPT_STR (oi, c, long_options), optarg, + s_err); if (SIZE_MAX < w_tmp) error (EXIT_FAILURE, 0, _("%s is too large"), optarg); desired_width = w_tmp; diff --git a/src/pr.c b/src/pr.c index f7ae015..329183b 100644 --- a/src/pr.c +++ b/src/pr.c @@ -796,14 +796,14 @@ cols_ready_to_print (void) using option +FIRST_PAGE:LAST_PAGE */ static bool -first_last_page (char const *pages) +first_last_page (char const *option, char const *pages) { char *p; uintmax_t first; uintmax_t last = UINTMAX_MAX; strtol_error err = xstrtoumax (pages, &p, 10, &first, ""); if (err != LONGINT_OK && err != LONGINT_INVALID_SUFFIX_CHAR) - _STRTOL_ERROR (EXIT_FAILURE, pages, _("page range"), err); + STRTOL_FATAL_ERROR (option, pages, err); if (p == pages || !first) return false; @@ -813,7 +813,7 @@ first_last_page (char const *pages) char const *p1 = p + 1; err = xstrtoumax (p1, &p, 10, &last, ""); if (err != LONGINT_OK) - _STRTOL_ERROR (EXIT_FAILURE, pages, _("page range"), err); + STRTOL_FATAL_ERROR (option, pages, err); if (p1 == p || last < first) return false; } @@ -902,7 +902,7 @@ main (int argc, char **argv) case 1: /* Non-option argument. */ /* long option --page dominates old `+FIRST_PAGE ...'. */ if (! (first_page_number == 0 - && *optarg == '+' && first_last_page (optarg + 1))) + && *optarg == '+' && first_last_page ("+", optarg + 1))) file_names[n_files++] = optarg; break; @@ -911,7 +911,7 @@ main (int argc, char **argv) if (! optarg) error (EXIT_FAILURE, 0, _("`--pages=FIRST_PAGE[:LAST_PAGE]' missing argument")); - else if (! first_last_page (optarg)) + else if (! first_last_page ("--pages", optarg)) error (EXIT_FAILURE, 0, _("Invalid page range %s"), quote (optarg)); break; diff --git a/src/sort.c b/src/sort.c index af23e84..dc7874a 100644 --- a/src/sort.c +++ b/src/sort.c @@ -414,7 +414,7 @@ static struct option const long_options[] = {"output", required_argument, NULL, 'o'}, {"reverse", no_argument, NULL, 'r'}, {"stable", no_argument, NULL, 's'}, - {"buffer-size", required_argument, NULL, 'S'}, + {OPT_STR_INIT ("buffer-size"), required_argument, NULL, 'S'}, {"field-separator", required_argument, NULL, 't'}, {"temporary-directory", required_argument, NULL, 'T'}, {"unique", no_argument, NULL, 'u'}, @@ -1032,7 +1032,7 @@ inittables (void) /* Specify the amount of main memory to use when sorting. */ static void -specify_sort_size (char const *s) +specify_sort_size (char const *option, char const *s) { uintmax_t n; char *suffix; @@ -1088,7 +1088,7 @@ specify_sort_size (char const *s) e = LONGINT_OVERFLOW; } - STRTOL_FATAL_ERROR (s, _("sort size"), e); + STRTOL_FATAL_ERROR (option, s, e); } /* Return the default sort size. */ @@ -2842,6 +2842,7 @@ main (int argc, char **argv) pedantic and a file was seen, unless the POSIX version predates 1003.1-2001 and -c was not seen and the operand is "-o FILE" or "-oFILE". */ + int oi = -1; if (c == -1 || (posixly_correct && nfiles != 0 @@ -2851,7 +2852,7 @@ main (int argc, char **argv) && argv[optind][0] == '-' && argv[optind][1] == 'o' && (argv[optind][2] || optind + 1 != argc))) || ((c = getopt_long (argc, argv, short_options, - long_options, NULL)) + long_options, &oi)) == -1)) { if (argc <= optind) @@ -3011,7 +3012,7 @@ main (int argc, char **argv) break; case 'S': - specify_sort_size (optarg); + specify_sort_size (OPT_STR (oi, c, long_options), optarg); break; case 't': diff --git a/src/system.h b/src/system.h index 3c7f49d..cc97f2f 100644 --- a/src/system.h +++ b/src/system.h @@ -592,3 +592,20 @@ emit_bug_reporting_address (void) bugs (typically your translation team's web or email address). */ printf (_("\nReport bugs to <%s>.\n"), PACKAGE_BUGREPORT); } + +/* Use OPT_IDX to decide whether to return either a short option + string "-C", or a long option string derived from LONG_OPTION. + OPT_IDX is -1 if the short option C was used; otherwise it is an + index into LONG_OPTIONS, which should have a name preceded by two + '-' characters. */ +static char opt_str_storage[3] = {'-', 0, 0}; +#define OPT_STR(opt_idx, c, long_options) \ + ((opt_idx) < 0 \ + ? (opt_str_storage[1] = c, opt_str_storage) \ + : LONG_OPT_STR (opt_idx, long_options)) + +/* Likewise, but assume OPT_IDX is nonnegative. */ +#define LONG_OPT_STR(opt_idx, long_options) ((long_options)[opt_idx].name - 2) + +/* Define an option string that will be used with OPT_STR or LONG_OPT_STR. */ +#define OPT_STR_INIT(name) ("--" name + 2) M ChangeLog M src/df.c M src/du.c M src/ls.c M src/od.c M src/pr.c M src/sort.c M src/system.h Committed as 53f11b33825d077a0b2857d8724482fab7fc1b75