Re: [PATCH] stdbuf work in progress

[Jim Meyering]

Pádraig Brady wrote:
> http://www.pixelbeat.org/patches/stdbuf.diff

Looking at the latest, I see this:

+  else
+    {
+      setvbuf_mode = _IOFBF;
+      /* Note this is done elsewhere in coreutils:
+         verify (SIZE_MAX <= ULONG_MAX);  */

Please drop the comment and just use verify here.
The other test may disappear some day,
and it costs nothing at run-time.
-------------------

+      size = strtoul (mode, NULL, 10);
+      if (size > 0)
+        buf = malloc (size);    /* will be free by fclose()  */
+      else
+        {
+          fprintf (stderr, _("invalid buffering mode %s for %s"),
+                   mode, fileno_to_name (fileno (stream)));

s/mode/quote (mode)/ in case it contains odd bytes.

+          return;
+        }
+    }
+
+  if (setvbuf (stream, buf, setvbuf_mode, size) != 0)

The comment above says never to call setvbuf with nonzero size
and with buf == NULL.  But if malloc fails, that's what happens.

Also, s/free/freed/ in the comment.
-------------------

In usage,

+  -i, --input=MODE   Setup standard input stream buffering\n\
+  -o, --output=MODE  Setup standard output stream buffering\n\
+  -e, --error=MODE   Setup standard error stream buffering\n\

how about s/Setup/Adjust/ or s//Change/
-------------------

+static struct
+{
+  size_t size;
+  int optc;
+  char *optarg;
+} stdbuf[3];
...
+static struct option const longopts[] =
+{

Please put struct definitions and decls like these two
before the first function.  Same with #defines and enums.
-------------------

+static int
+optc_to_fileno (int c)
+{
+  if (c == 'e')
+    return STDERR_FILENO;
+  if (c == 'i')
+    return STDIN_FILENO;
+  if (c == 'o')
+    return STDOUT_FILENO;
+  return -1;
+}

Use a switch, and a single return stmt.
That avoids some duplication.
-------------------

+/* Internal error  */
+#define EXIT_CANCELED 125

Prefer an enum, since that is usable in the debugger,
while EXIT_CANCELED is not.

  enum { EXIT_CANCELED = 125 };
-------------------

+  enum strtol_error e;
+  uintmax_t tmp_size;
+  e = xstrtoumax (str, NULL, 10, &tmp_size, "EGkKMPTYZ0");

I have a slight preference for combining the decl and definition.
More concise, better visual locality.

   uintmax_t tmp_size;
   enum strtol_error e = xstrtoumax (str, NULL, 10, &tmp_size, "EGkKMPTYZ0");
-------------------

+  int len = readlink ("/proc/self/exe", tmppath, sizeof (tmppath) - 1);

That should be "ssize_t", not int.

-------------------
In this:

+           path = strdup (path);
+           for (dir = strtok (path, ":"); dir != NULL; dir = strtok (NULL, 
":"))

Did you mean xstrdup?
Otherwise, that first strtok will dereference NULL upon failed strdup.
-------------------

+  set_program_path (argv[0]);
+  if (!program_path)
+    program_path = strdup (PKGLIBDIR);  /* Need to init to non NULL.  */

Handle strdup failure?
------------------------

+        case 'e':
+        case 'i':
+        case 'o':
+          opt_fileno = optc_to_fileno (c);
+          stdbuf[opt_fileno].optc = c;
+          while (isspace (*optarg))
+            optarg++;

I have a slight bias for using c_isspace for this.
With it, this tiny aspect of argument parsing is locale *in*dependent.
-------------------------

Also, while currently it's obvious via inspection,
it'd be nice to have some assurance that this will always be true:

  0 < opt_fileno && opt_fileno <= ARRAY_CARDINALITY (stdbuf)

just before that use:

+          stdbuf[opt_fileno].optc = c;

Use assert?

-------------------------

in set_program_path, please move this decl
      char *path;
"down" into the block where it's used.

-------------------------

Comment style:

+              /* -oL will be by far the most common use of this utility,
+               * but one could easily think -iL might have the same affect,
+               * so disallow it as it could be confusing. */

s/\* /  /
-------------------------

+          if (!STREQ (optarg, "L") &&
+              parse_size (optarg, &stdbuf[opt_fileno].size) == -1)

Please put the "&&" on the continuation line.
-------------------------
Use ARRAY_CARDINALITY

+  for (i = 0; i < sizeof (stdbuf) / sizeof *(stdbuf); i++)

   for (i = 0; i < ARRAY_CARDINALITY (stdbuf); i++)
-------------------------
Diagnose failure?

+          putenv (var);

-------------------------
...
+  execvp (*argv, argv);
+
+  {
+    int exit_status = (errno == ENOENT ? EXIT_ENOENT : EXIT_CANNOT_INVOKE);
+    error (0, errno, "%s", *argv);

How about this instead:

  error (0, errno, _("failed to run command %s"), quote (argv[0]));

I know that message is slightly different from another that's
used in several programs:

  error (0, errno, _("cannot run command %s"), quote (argv[0]));

But "failed to" is more precise.
In fact, I've just changed those others.
Hmm... but there are over 150 other diagnostics that start with '"cannot '.
I'll leave those, for now ;-)