[patch v2] fgets: more tests

[DJ Delorie]

Florian Weimer <fweimer@redhat.com> writes:
>> +#define _GNU_SOURCE 1
>
> Should be unnecessary?  All tests are built with _GNU_SOURCE by default,
> I think?

I needed it at one point, but no longer.  Removed.

>> +#include <libc-diag.h>
>> +DIAG_PUSH_NEEDS_COMMENT;
>> +/* We're intentionally passing an invalid size and/or NULL later.  */
>> +DIAG_IGNORE_NEEDS_COMMENT (7, "-Wnonnull");
>
> I'm surprised this works.  Shouldn't it be around the site of the
> invalid use?

It's *also* at the use site.  This is a workaround for building with
__FORTIFY_SOURCE=2 with at least gcc 11.

>> +#define PASS() printf("ok\n")
>
> Missing space before second '('.

Fixed.

>> +/*------------------------------------------------------------*/
>> +/* Implementation of our FILE stream backend.  */
>
> We generally do not use ----- separators?

We generally don't, no, but we should use some sort of separators
between groups of functionally-related code.

>> +static ssize_t
>> +io_write (void *vcookie, const char *buf, size_t size)
>> +{
>> +  VALIDATE_COOKIE ();
>> +
>> +  bytes_written += size;
>> +  return -1;
>> +}
>
> Should this simply call FAIL_EXIT1 because it's not expected to be used?

Could.  I was setting up a template for other similar tests, which would
check for that error by looking at bytes_written.

It would be better if the implementation of fopencookie allowed for NULL
functions, and did the Right Thing for us.

>> +/*------------------------------------------------------------*/
>> +/* Convenience functions.  */
>> +
>> +static char *
>> +hex (const char *b, int s)
>
> Wouldn't TEST_COMPARE_BLOB provide this for you?

Could.  It doesn't have an option for adding a message to the output,
but I can rename the relevent variables to be more self-documenting when
used by that.

However, TEST_COMPARE_BLOB does not print anything when it passes, nor
does it return a value that says if it passes.  This means a successful
test prints nothing, and one goal here is to be more verbose in noting
passing sub-tests.

>> +#define my_open(s,l,m) io_open (s, l, m, (void *) &cookie)
>
> Could be a regular function?

It wouldn't have access to the cookie.

>> +  printf ("testing base operation...");
>> +  f = my_open ("hello\n", 6, "r");
>> +  memset (buf, 0x11, sizeof (buf));
>> +  str = fgets (buf, 100, f);
>> +  if (str == NULL)
>> +    {
>> +      FAIL ("returned NULL");
>> +    }
>> +  else if (memcmp (str, "hello\n\0", 7) != 0)
>> +    {
>> +      FAIL ("returned %s instead of %s", hex (str, 7), hex ("hello\n\0", 7));
>
> Could this check a bit further, to check that the 0x11 bytes are still
> there?  Or isn't this a guarantee we want to provide?

That's a good idea.

>> +  printf ("testing zero size file...");
>> +  f = my_open ("hello\n", 0, "r");
>> +  memset (buf, 0x11, sizeof (buf));
>> +  str = fgets (buf, 100, f);
>> +  if (str != NULL)
>> +    {
>> +      FAIL ("returned %s instead of NULL", hex (str, strlen (str)));
>> +    }
>
> Likewise, verify that buffer contents is unchanged?
> Also check the stream error indicator.

Ok.

>> +  DIAG_PUSH_NEEDS_COMMENT;
>> +  /* We're intentionally passing an invalid size here.  */
>> +  DIAG_IGNORE_NEEDS_COMMENT (7, "-Wnonnull");
>> +  str = fgets (NULL, 100, f);
>> +  DIAG_POP_NEEDS_COMMENT;
>
> I think you should use a compiler barrier (perhaps:
> void *volatile null = NULL;) instead of disabling the warning.
> Getting rid of the warning won't change code generation if GCC
> replaces this with a trap (which future versions might).

Done.

>> +#ifdef IO_DEBUG
>> +  /* These tests only pass if glibc is built with -DIO_DEBUG.  */
>
> We don't build with IO_DEBUG, so these tests do not seem valuable to me,
> sorry.

True, but they demonstrate that I thought of checking those cases, and
specifically couldn't, and the conditions that would allow testing them.