Re: [PATCH] stdbuf work in progress

[Pádraig Brady]

Jim Meyering wrote:
> 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.

Of course.  I wrote the comment so I would do this
when I integrated the lib to coreutils, but forgot
to add the FIXME tag and therefore missed it :p

> -------------------
> 
> +      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.

Well I had quote() originally, but that required linking to
libcoreutils.a which causes the PIC linking errors on 64 bit.

> 
> +          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.

Well the C99 standard says NULL buffers with non zero size are allowed.
GLIBC currently ignores this combination but may support it in future.
FreeBSD currently allocates a buffer internally on the first read/write
operation on the stream.  So I thought it best to not error in this case
as it may actually succeed at a later point.  But I changed it to
fail early as you suggest.

The rest of your comments were hopefully fixed also in the latest version:

http://www.pixelbeat.org/patches/stdbuf.diff

thanks!
Pádraig.