On Mon, Apr 11, 2022 at 10:59:27PM -0600, Nathan Sharp wrote:
> .gitignore | 1 +
> configure.ac | 8 +
> include/pathnames.h | 4 +
> meson.build | 12 ++
> meson_options.txt | 2 +
> misc-utils/Makemodule.am | 7 +
> misc-utils/meson.build | 4 +
> misc-utils/pipesz.c | 347 +++++++++++++++++++++++++++++++++++++++
> 8 files changed, 385 insertions(+)
> create mode 100644 misc-utils/pipesz.c
I had time to review it finally (sorry for delay).
> +static void __attribute__((__noreturn__)) usage(void)
> +{
> + fputs(USAGE_HEADER, stdout);
> + printf(_(" %s [options] [--set <size>] [--] [command]\n"), program_invocation_short_name);
> + printf(_(" %s [options] --get\n"), program_invocation_short_name);
> +
> + fputs(USAGE_SEPARATOR, stdout);
> + /* TRANSLATORS: 'command' refers to a program argument */
> + puts(_("Set or examine pipe buffer sizes and optionally execute command."));
> +
> + fputs(USAGE_OPTIONS, stdout);
> + puts(_(" -g, --get examine pipe buffers"));
> + /* TRANSLATORS: '%s' refers to a system file */
> + printf(_(" -s, --set <size> set pipe buffer sizes\n"
> + " size defaults to %s\n"
> + ), PIPESZ_DEFAULT_SIZE_FILE);
> +
> + fputs(USAGE_SEPARATOR, stdout);
> + puts(_(" -f, --file <path> act on a file"));
> + puts(_(" -n, --fd <num> act on a file descriptor"));
> + puts(_(" -i, --stdin act on standard input"));
> + puts(_(" -o, --stdout act on standard output"));
> + puts(_(" -e, --stderr act on standard error"));
> +
> + fputs(USAGE_SEPARATOR, stdout);
> + puts(_(" -c, --check do not continue after an error"));
Would be better to be paranoid (as expected by --check) by default and
implement --force for cases when user wants something crazy? That's
usual way utils are implemented.
...
> + /* check for --help or --version */
> + while ((c = getopt_long(argc, argv, shortopts, longopts, NULL)) != -1)
> + switch (c) {
> + case 'h':
> + usage();
> + case 'V':
> + print_version(EXIT_SUCCESS);
> + }
Why we need multiple getopt_long() blocks? I guess --help and
--version could be together with other options. This is very unusual.
> + /* gather normal options */
> + optind = 1;
> + while ((c = getopt_long(argc, argv, shortopts, longopts, NULL)) != -1) {
> + err_exclusive_options(c, longopts, excl, excl_st);
> +
> + switch (c) {
> + case 'c':
> + opt_check = TRUE;
> + break;
> + case 'e':
> + ++n_opt_pipe;
> + break;
> + case 'f':
> + ++n_opt_pipe;
> + break;
> + case 'g':
> + opt_get = TRUE;
> + break;
> + case 'i':
> + ++n_opt_pipe;
> + break;
> + case 'n':
> + fd = strtos32_or_err(optarg, _("invalid fd argument"));
> + ++n_opt_pipe;
> + break;
> + case 'o':
> + ++n_opt_pipe;
> + break;
> + case 'q':
> + opt_quiet = TRUE;
> + break;
> + case 's':
> + sz = strtosize_or_err(optarg, _("invalid size argument"));
> + opt_size = sz >= INT_MAX ? INT_MAX : (int)sz;
> + break;
> + case 'v':
> + opt_verbose = TRUE;
> + break;
> + default:
> + errtryhelp(EXIT_FAILURE);
> + }
> + }
...
> + /* go through the arguments again and do the requested operations */
> + optind = 1;
> + while ((c = getopt_long(argc, argv, shortopts, longopts, NULL)) != -1)
> + switch (c) {
> + case 'e':
> + do_fd(STDERR_FILENO);
> + break;
> + case 'f':
> + do_file(optarg);
> + break;
> + case 'i':
> + do_fd(STDIN_FILENO);
> + break;
> + case 'n':
> + /* optarg was checked before, but it's best to be safe */
> + fd = strtos32_or_err(optarg, _("invalid fd argument"));
> + do_fd(fd);
> + break;
> + case 'o':
> + do_fd(STDOUT_FILENO);
> + break;
> + }
... and another getopt_long() block.
What about to define
enum {
PIPESZ_ON_STDERR = (1 << 1),
PIPESZ_ON_STDIN = (1 << 2),
PIPESZ_ON_STDOUT = (1 << 3),
PIPESZ_ON_FILE = (1 << 4),
PIPESZ_ON_FD = (1 << 5)
};
and in the while (getopt_long()) block set operation |= PIPESZ_ON_xxxxx and
later in code use it? (also with variables 'filename' and 'fd'.
Then you can keep all arguments parsing on one place, right? :-)
The rest seems good.
Karel
--
Karel Zak <kzak@xxxxxxxxxx>
http://karelzak.blogspot.com
