[v4,04/13] support: Add <support/readdir.h>

  It allows to read directories using the six readdir variants
without writing type-specific code or using skeleton files
that are compiled four times.

The readdir_r subtest for support_readdir_expect_error revealed
bug 32124.
---
 support/Makefile              |   2 +
 support/readdir.h             |  79 ++++++++++
 support/support_readdir.c     | 287 ++++++++++++++++++++++++++++++++++
 support/tst-support_readdir.c |  71 +++++++++
 4 files changed, 439 insertions(+)
 create mode 100644 support/readdir.h
 create mode 100644 support/support_readdir.c
 create mode 100644 support/tst-support_readdir.c
  

Florian Weimer <fweimer@redhat.com> writes:
> It allows to read directories using the six readdir variants
> without writing type-specific code or using skeleton files
> that are compiled four times.

One suggestion.
One typo.
One question.
One suspect test.

Mostly OK though.  I could be talked into a LGTM ;-)

> diff --git a/support/Makefile b/support/Makefile
> +  support_readdir \
> +  tst-support_readdir \

Ok.

> diff --git a/support/readdir.h b/support/readdir.h
> +/* Type-generic wrapper for readdir functions.
> +   Copyright (C) 2024 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#ifndef SUPPORT_READDIR_H
> +#define SUPPORT_READDIR_H
> +
> +#include <dirent.h>
> +#include <stdbool.h>
> +#include <stdint.h>
> +
> +__BEGIN_DECLS

Ok.

> +/* Definition independent of _FILE_OFFSET_BITS.  */
> +struct support_dirent
> +{
> +  uint64_t d_ino;
> +  uint64_t d_off;               /* 0 if d_off is not supported.  */
> +  uint32_t d_type;
> +  char *d_name;
> +};

Ok.

> +/* Operation to be performed by support_readdir below.  */
> +enum support_readdir_op
> +  {
> +    SUPPORT_READDIR,
> +    SUPPORT_READDIR64,
> +    SUPPORT_READDIR_R,
> +    SUPPORT_READDIR64_R,
> +    SUPPORT_READDIR64_COMPAT,
> +    SUPPORT_READDIR64_R_COMPAT,
> +  };

Ok.

> +/* Returns the last supported function.  May exclude
> +   SUPPORT_READDIR64_R_COMPAT if not implemented.  */
> +enum support_readdir_op support_readdir_op_last (void);

Ok.

> +/* Returns the name of the function that corresponds to the OP constant.  */
> +const char *support_readdir_function (enum support_readdir_op op);

Ok.

> +/* Returns the inode field width for OP.  */
> +unsigned int support_readdir_inode_width (enum support_readdir_op op);

Ok.  Suggest: Comment should specify bits or bytes.

> +/* Returns true if OP is an _r variant with name length restrictions.  */
> +bool support_readdir_r_variant (enum support_readdir_op op);

Ok.

> +/* First, free E->d_name and set the field to NULL.  Then call the
> +   readdir variant as specified by OP.  If successfully, copy fields
> +   to E, make a copy of the entry name using strdup, and write its
> +   addres sto E->d_name.
> +
> +   Return true if an entry was read, or false if the end of the
> +   directory stream was reached.  Terminates the process upon error.
> +   The caller is expected to free E->d_name if the function is not
> +   called again for this E.  */
> +bool support_readdir (DIR *stream, enum support_readdir_op op,
> +                      struct support_dirent *e);

Ok.  I note that "free E->d_name" works if E->d_name is already NULL,
although that's not mentioned in the comment.  The user will need to
initialize E->d_name to *something* and that will likely be NULL.

> +/* Checks t hat the readdir operation OP fails with errno value EXPECTED.  */

typo s/t hat/that/

> +void support_readdir_expect_error (DIR *stream, enum support_readdir_op op,
> +                                   int expected);

Ok.

> diff --git a/support/support_readdir.c b/support/support_readdir.c
> new file mode 100644
> index 0000000000..78ccf4a48d
> --- /dev/null
> +++ b/support/support_readdir.c
> @@ -0,0 +1,287 @@
> +/* Type-generic wrapper for readdir functions.
> +   Copyright (C) 2024 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#include <support/readdir.h>
> +
> +#include <dlfcn.h>
> +#include <stddef.h>
> +#include <stdlib.h>
> +#include <support/check.h>
> +#include <support/support.h>
> +#include <support/xdirent.h>

Ok.

> +/* Copied from <olddirent.h>.  */
> +struct __old_dirent64
> +  {
> +    __ino_t d_ino;
> +    __off64_t d_off;
> +    unsigned short int d_reclen;
> +    unsigned char d_type;
> +    char d_name[256];
> +  };

Ok.

> +static struct __old_dirent64 *(*readdir64_compat) (DIR *);
> +static int (*readdir64_r_compat) (DIR *, struct __old_dirent64 *,
> +                                  struct __old_dirent64 **);

Ok.

> +static void __attribute__ ((constructor))
> +init (void)
> +{
> +  /* These compat symbols exists on alpha, i386, m67k , powerpc, s390,
> +     sparc. at the same GLIBC_2.1 version. */
> +  readdir64_compat = dlvsym (RTLD_DEFAULT, "readdir64", "GLIBC_2.1");
> +  readdir64_r_compat = dlvsym (RTLD_DEFAULT, "readdir64_r", "GLIBC_2.1");
> +}

Ok.

> +enum support_readdir_op
> +support_readdir_op_last (void)
> +{
> +  if (readdir64_r_compat != NULL)
> +    {
> +      TEST_VERIFY (readdir64_compat != NULL);
> +      return SUPPORT_READDIR64_R_COMPAT;
> +    }
> +  else
> +    return SUPPORT_READDIR64_R;
> +}

Ok.

> +const char *
> +support_readdir_function (enum support_readdir_op op)
> +{
> +  switch (op)
> +    {
> +      case SUPPORT_READDIR:
> +        return "readdir";
> +      case SUPPORT_READDIR64:
> +        return "readdir64";
> +      case SUPPORT_READDIR_R:
> +        return "readdir_r";
> +      case SUPPORT_READDIR64_R:
> +        return "readdir64_r";
> +      case SUPPORT_READDIR64_COMPAT:
> +        return "readdir64@GBLIC_2.1";
> +      case SUPPORT_READDIR64_R_COMPAT:
> +        return "readdir64_r@GBLIC_2.1";
> +    }
> +  FAIL_EXIT1 ("invalid support_readdir_op constant: %d", op);
> +}

Ok.

> +unsigned int
> +support_readdir_inode_width (enum support_readdir_op op)
> +{
> +  switch (op)
> +    {
> +      case SUPPORT_READDIR:
> +      case SUPPORT_READDIR_R:
> +        return sizeof ((struct dirent) { 0, }.d_ino) * 8;
> +      case SUPPORT_READDIR64:
> +      case SUPPORT_READDIR64_R:
> +        return sizeof ((struct dirent64) { 0, }.d_ino) * 8;
> +      case SUPPORT_READDIR64_COMPAT:
> +      case SUPPORT_READDIR64_R_COMPAT:
> +        return sizeof ((struct __old_dirent64) { 0, }.d_ino) * 8;
> +    }
> +  FAIL_EXIT1 ("invalid support_readdir_op constant: %d", op);
> +}

Ok.

> +bool
> +support_readdir_r_variant (enum support_readdir_op op)
> +{
> +  switch (op)
> +    {
> +      case SUPPORT_READDIR:
> +      case SUPPORT_READDIR64:
> +      case SUPPORT_READDIR64_COMPAT:
> +        return false;
> +      case SUPPORT_READDIR_R:
> +      case SUPPORT_READDIR64_R:
> +      case SUPPORT_READDIR64_R_COMPAT:
> +        return true;
> +    }
> +  FAIL_EXIT1 ("invalid support_readdir_op constant: %d", op);
> +}

Ok.

> +static bool
> +copy_dirent (struct support_dirent *dst, struct dirent *src)
> +{
> +  if (src == NULL)
> +    return false;

But we don't check for dst == NULL ?

> +  dst->d_ino = src->d_ino;
> +#ifdef _DIRENT_HAVE_D_OFF
> +  dst->d_off = src->d_off;
> +#else
> +  dst->d_off = 0;
> +#endif
> +  dst->d_type = src->d_type;
> +  dst->d_name = xstrdup (src->d_name);
> +  return true;
> +}

Ok.

> +static bool
> +copy_dirent64 (struct support_dirent *dst, struct dirent64 *src)
> +{
> +  if (src == NULL)
> +    return false;
> +  dst->d_ino = src->d_ino;
> +#ifdef _DIRENT_HAVE_D_OFF
> +  dst->d_off = src->d_off;
> +#else
> +  dst->d_off = 0;
> +#endif
> +  dst->d_type = src->d_type;
> +  dst->d_name = xstrdup (src->d_name);
> +  return true;
> +}

Ok.

> +static bool
> +copy_old_dirent64 (struct support_dirent *dst, struct __old_dirent64 *src)
> +{
> +  if (src == NULL)
> +    return false;
> +  dst->d_ino = src->d_ino;
> +#ifdef _DIRENT_HAVE_D_OFF
> +  dst->d_off = src->d_off;
> +#else
> +  dst->d_off = 0;
> +#endif
> +  dst->d_type = src->d_type;
> +  dst->d_name = xstrdup (src->d_name);
> +  return true;
> +}

Ok.

> +bool
> +support_readdir (DIR *stream, enum support_readdir_op op,
> +                 struct support_dirent *e)
> +{
> +  free (e->d_name);
> +  e->d_name = NULL;
> +  switch (op)
> +    {
> +    case SUPPORT_READDIR:
> +      return copy_dirent (e, xreaddir (stream));

Ok.

> +    case SUPPORT_READDIR64:
> +      return copy_dirent64 (e, xreaddir64 (stream));

Ok.

> +      /* The functions readdir_r, readdir64_r were deprecated in glibc 2.24.  */
> +      DIAG_PUSH_NEEDS_COMMENT;
> +      DIAG_IGNORE_NEEDS_COMMENT (4.9, "-Wdeprecated-declarations");
> +
> +    case SUPPORT_READDIR_R:
> +      {
> +        struct dirent buf;
> +        if (!xreaddir_r (stream, &buf))
> +          return false;
> +        return copy_dirent (e, &buf);
> +      }

Ok.

> +    case SUPPORT_READDIR64_R:
> +      {
> +        struct dirent64 buf;
> +        if (!xreaddir64_r (stream, &buf))
> +          return false;
> +        return copy_dirent64 (e, &buf);
> +      }
> +
> +      DIAG_POP_NEEDS_COMMENT;

Ok.

> +    case SUPPORT_READDIR64_COMPAT:
> +      if (readdir64_compat == NULL)
> +        FAIL_EXIT1 ("readdir64 compat function not implemented");
> +      return copy_old_dirent64 (e, readdir64_compat (stream));

Ok.

> +    case SUPPORT_READDIR64_R_COMPAT:
> +      {
> +        if (readdir64_r_compat == NULL)
> +          FAIL_EXIT1 ("readdir64_r compat function not implemented");
> +        struct __old_dirent64 buf;
> +        struct __old_dirent64 *e1;
> +        int ret = readdir64_r_compat (stream, &buf, &e1);
> +        if (ret != 0)
> +          {
> +            errno = ret;
> +            FAIL ("readdir64_r@GLIBC_2.1: %m");
> +            return false;
> +          }
> +        if (e1 == NULL)
> +          return false;
> +        return copy_old_dirent64 (e, e1);
> +      }
> +    }
> +  FAIL_EXIT1 ("support_readdir: invalid op argument %d", (int) op);
> +}

Ok.

> +void
> +support_readdir_expect_error (DIR *stream, enum support_readdir_op op,
> +                              int expected)
> +{
> +  switch (op)
> +    {
> +    case SUPPORT_READDIR:
> +      errno = 0;
> +      TEST_VERIFY (readdir (stream) == NULL);
> +      TEST_COMPARE (errno, expected);
> +      return;

Ok.

> +    case SUPPORT_READDIR64:
> +      errno = 0;
> +      TEST_VERIFY (readdir64 (stream) == NULL);
> +      TEST_COMPARE (errno, expected);
> +      return;

Ok.

> +      /* The functions readdir_r, readdir64_r were deprecated in glibc 2.24.  */
> +      DIAG_PUSH_NEEDS_COMMENT;
> +      DIAG_IGNORE_NEEDS_COMMENT (4.9, "-Wdeprecated-declarations");
> +
> +    case SUPPORT_READDIR_R:
> +      {
> +        struct dirent buf;
> +        struct dirent *e;
> +        errno = readdir_r (stream, &buf, &e);
> +        TEST_COMPARE (errno, expected);;
> +      }
> +      return;
> +    case SUPPORT_READDIR64_R:
> +      {
> +        struct dirent64 buf;
> +        struct dirent64 *e;
> +        errno = readdir64_r (stream, &buf, &e);
> +        TEST_COMPARE (errno, expected);;
> +      }
> +      return;
> +
> +      DIAG_POP_NEEDS_COMMENT;

Ok.

> +    case SUPPORT_READDIR64_COMPAT:
> +      if (readdir64_compat == NULL)
> +        FAIL_EXIT1 ("readdir64_r compat function not implemented");
> +      errno = 0;
> +      TEST_VERIFY (readdir64_compat (stream) == NULL);
> +      TEST_COMPARE (errno, expected);
> +      return;

Ok.

> +    case SUPPORT_READDIR64_R_COMPAT:
> +      {
> +        if (readdir64_r_compat == NULL)
> +          FAIL_EXIT1 ("readdir64_r compat function not implemented");
> +        struct __old_dirent64 buf;
> +        struct __old_dirent64 *e;
> +        errno = readdir64_r_compat (stream, &buf, &e);
> +        TEST_COMPARE (errno, expected);
> +      }
> +      return;
> +    }

Ok.

> +  FAIL_EXIT1 ("support_readdir_expect_error: invalid op argument %d",
> +              (int) op);
> +}

Ok.

> diff --git a/support/tst-support_readdir.c b/support/tst-support_readdir.c
> +/* Test the support_readdir function.
> +   Copyright (C) 2024 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#include <support/readdir.h>
> +
> +#include <fcntl.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <support/check.h>
> +#include <support/xdirent.h>
> +#include <support/xunistd.h>

Ok.

> +static int
> +do_test (void)
> +{
> +  DIR *reference_stream = xopendir (".");
> +  struct dirent64 *reference = xreaddir64 (reference_stream);
> +
> +  for (enum support_readdir_op op = 0; op <= support_readdir_op_last (); ++op)
> +    {
> +      DIR *stream = xopendir (".");
> +      struct support_dirent e;
> +      memset (&e, 0xcc, sizeof (e));
> +      e.d_name = NULL;
> +      TEST_VERIFY (support_readdir (stream, op, &e));
> +      TEST_COMPARE (e.d_ino, reference->d_ino);
> +#ifdef _DIRENT_HAVE_D_OFF
> +      TEST_COMPARE (e.d_off, reference->d_off);
> +#else
> +      TEST_COMPARE (e.d_off, 0);
> +#endif
> +      TEST_COMPARE (e.d_type, reference->d_type);
> +      TEST_COMPARE_STRING (e.d_name, reference->d_name);
> +      free (e.d_name);
> +      xclosedir (stream);
> +    }
> +
> +  xclosedir (reference_stream);

This test will fail if the actual inode numbers exceed 2^32, when
testing 32-bit readdir's.  While highly unlikely, it should be accounted
for.

Otherwise OK.

> +  /* Error injection test.  */
> +  int devnull = xopen ("/dev/null", O_RDONLY, 0);
> +  for (enum support_readdir_op op = 0; op <= support_readdir_op_last (); ++op)
> +    {
> +      DIR *stream = xopendir (".");
> +      /* A descriptor incompatible with readdir.  */
> +      xdup2 (devnull, dirfd (stream));
> +      errno = -1;
> +      support_readdir_expect_error (stream, op, ENOTDIR);
> +      xclosedir (stream);
> +    }
> +  xclose (devnull);
> +
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>

Ok.
  

* DJ Delorie:

>> +/* Returns the inode field width for OP.  */
>> +unsigned int support_readdir_inode_width (enum support_readdir_op op);
>
> Ok.  Suggest: Comment should specify bits or bytes.

Fixed.

>> +/* First, free E->d_name and set the field to NULL.  Then call the
>> +   readdir variant as specified by OP.  If successfully, copy fields
>> +   to E, make a copy of the entry name using strdup, and write its
>> +   addres sto E->d_name.
>> +
>> +   Return true if an entry was read, or false if the end of the
>> +   directory stream was reached.  Terminates the process upon error.
>> +   The caller is expected to free E->d_name if the function is not
>> +   called again for this E.  */
>> +bool support_readdir (DIR *stream, enum support_readdir_op op,
>> +                      struct support_dirent *e);
>
> Ok.  I note that "free E->d_name" works if E->d_name is already NULL,
> although that's not mentioned in the comment.  The user will need to
> initialize E->d_name to *something* and that will likely be NULL.

I added:

   Note that this function assumes that E->d_name has been initialized
   to NULL or has been allocated by a previous call to this function.  */

>> +static bool
>> +copy_dirent (struct support_dirent *dst, struct dirent *src)
>> +{
>> +  if (src == NULL)
>> +    return false;
>
> But we don't check for dst == NULL ?

No, user error.  Added __non_null annotations to <support/readdir.h>.

>> +static int
>> +do_test (void)
>> +{
>> +  DIR *reference_stream = xopendir (".");
>> +  struct dirent64 *reference = xreaddir64 (reference_stream);
>> +
>> +  for (enum support_readdir_op op = 0; op <= support_readdir_op_last (); ++op)
>> +    {
>> +      DIR *stream = xopendir (".");
>> +      struct support_dirent e;
>> +      memset (&e, 0xcc, sizeof (e));
>> +      e.d_name = NULL;
>> +      TEST_VERIFY (support_readdir (stream, op, &e));
>> +      TEST_COMPARE (e.d_ino, reference->d_ino);
>> +#ifdef _DIRENT_HAVE_D_OFF
>> +      TEST_COMPARE (e.d_off, reference->d_off);
>> +#else
>> +      TEST_COMPARE (e.d_off, 0);
>> +#endif
>> +      TEST_COMPARE (e.d_type, reference->d_type);
>> +      TEST_COMPARE_STRING (e.d_name, reference->d_name);
>> +      free (e.d_name);
>> +      xclosedir (stream);
>> +    }
>> +
>> +  xclosedir (reference_stream);
>
> This test will fail if the actual inode numbers exceed 2^32, when
> testing 32-bit readdir's.  While highly unlikely, it should be accounted
> for.

Many, many tests assume that you don't get EOVERFLOW due to out of range
inode numbers.  You can't run the testsuite on systems where ino_t is
32-bit and the inode numbers do not fit.

Thanks,
Florian
  

On 30/08/24 16:52, Florian Weimer wrote:
> It allows to read directories using the six readdir variants
> without writing type-specific code or using skeleton files
> that are compiled four times.
> 
> The readdir_r subtest for support_readdir_expect_error revealed
> bug 32124.

Should we just build this for SHARED? Or maybe just support SUPPORT_READDIR,
and SUPPORT_READDIR64 such scenario?

> ---
>  support/Makefile              |   2 +
>  support/readdir.h             |  79 ++++++++++
>  support/support_readdir.c     | 287 ++++++++++++++++++++++++++++++++++
>  support/tst-support_readdir.c |  71 +++++++++
>  4 files changed, 439 insertions(+)
>  create mode 100644 support/readdir.h
>  create mode 100644 support/support_readdir.c
>  create mode 100644 support/tst-support_readdir.c
> 
> diff --git a/support/Makefile b/support/Makefile
> index ce194bfdd4..ec9793ab1e 100644
> --- a/support/Makefile
> +++ b/support/Makefile
> @@ -73,6 +73,7 @@ libsupport-routines = \
>    support_quote_blob \
>    support_quote_blob_wide \
>    support_quote_string \
> +  support_readdir \
>    support_readdir_check \
>    support_readdir_r_check \
>    support_record_failure \
> @@ -326,6 +327,7 @@ tests = \
>    tst-support_quote_blob \
>    tst-support_quote_blob_wide \
>    tst-support_quote_string \
> +  tst-support_readdir \
>    tst-support_record_failure \
>    tst-test_compare \
>    tst-test_compare_blob \
> diff --git a/support/readdir.h b/support/readdir.h
> new file mode 100644
> index 0000000000..7561f5da02
> --- /dev/null
> +++ b/support/readdir.h
> @@ -0,0 +1,79 @@
> +/* Type-generic wrapper for readdir functions.
> +   Copyright (C) 2024 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#ifndef SUPPORT_READDIR_H
> +#define SUPPORT_READDIR_H
> +
> +#include <dirent.h>
> +#include <stdbool.h>
> +#include <stdint.h>
> +
> +__BEGIN_DECLS
> +
> +/* Definition independent of _FILE_OFFSET_BITS.  */
> +struct support_dirent
> +{
> +  uint64_t d_ino;
> +  uint64_t d_off;               /* 0 if d_off is not supported.  */
> +  uint32_t d_type;
> +  char *d_name;
> +};
> +
> +/* Operation to be performed by support_readdir below.  */
> +enum support_readdir_op
> +  {
> +    SUPPORT_READDIR,
> +    SUPPORT_READDIR64,
> +    SUPPORT_READDIR_R,
> +    SUPPORT_READDIR64_R,
> +    SUPPORT_READDIR64_COMPAT,
> +    SUPPORT_READDIR64_R_COMPAT,
> +  };
> +
> +/* Returns the last supported function.  May exclude
> +   SUPPORT_READDIR64_R_COMPAT if not implemented.  */
> +enum support_readdir_op support_readdir_op_last (void);
> +
> +/* Returns the name of the function that corresponds to the OP constant.  */
> +const char *support_readdir_function (enum support_readdir_op op);
> +
> +/* Returns the inode field width for OP.  */
> +unsigned int support_readdir_inode_width (enum support_readdir_op op);
> +
> +/* Returns true if OP is an _r variant with name length restrictions.  */
> +bool support_readdir_r_variant (enum support_readdir_op op);
> +
> +/* First, free E->d_name and set the field to NULL.  Then call the
> +   readdir variant as specified by OP.  If successfully, copy fields
> +   to E, make a copy of the entry name using strdup, and write its
> +   addres sto E->d_name.
> +
> +   Return true if an entry was read, or false if the end of the
> +   directory stream was reached.  Terminates the process upon error.
> +   The caller is expected to free E->d_name if the function is not
> +   called again for this E.  */
> +bool support_readdir (DIR *stream, enum support_readdir_op op,
> +                      struct support_dirent *e);
> +
> +/* Checks t hat the readdir operation OP fails with errno value EXPECTED.  */
> +void support_readdir_expect_error (DIR *stream, enum support_readdir_op op,
> +                                   int expected);
> +
> +__END_DECLS
> +
> +#endif /* SUPPORT_READDIR_H */
> diff --git a/support/support_readdir.c b/support/support_readdir.c
> new file mode 100644
> index 0000000000..78ccf4a48d
> --- /dev/null
> +++ b/support/support_readdir.c
> @@ -0,0 +1,287 @@
> +/* Type-generic wrapper for readdir functions.
> +   Copyright (C) 2024 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#include <support/readdir.h>
> +
> +#include <dlfcn.h>
> +#include <stddef.h>
> +#include <stdlib.h>
> +#include <support/check.h>
> +#include <support/support.h>
> +#include <support/xdirent.h>
> +
> +/* Copied from <olddirent.h>.  */
> +struct __old_dirent64
> +  {
> +    __ino_t d_ino;
> +    __off64_t d_off;
> +    unsigned short int d_reclen;
> +    unsigned char d_type;
> +    char d_name[256];
> +  };
> +
> +static struct __old_dirent64 *(*readdir64_compat) (DIR *);
> +static int (*readdir64_r_compat) (DIR *, struct __old_dirent64 *,
> +                                  struct __old_dirent64 **);
> +
> +static void __attribute__ ((constructor))
> +init (void)
> +{
> +  /* These compat symbols exists on alpha, i386, m67k , powerpc, s390,
> +     sparc. at the same GLIBC_2.1 version. */
> +  readdir64_compat = dlvsym (RTLD_DEFAULT, "readdir64", "GLIBC_2.1");
> +  readdir64_r_compat = dlvsym (RTLD_DEFAULT, "readdir64_r", "GLIBC_2.1");
> +}
> +
> +enum support_readdir_op
> +support_readdir_op_last (void)
> +{
> +  if (readdir64_r_compat != NULL)
> +    {
> +      TEST_VERIFY (readdir64_compat != NULL);
> +      return SUPPORT_READDIR64_R_COMPAT;
> +    }
> +  else
> +    return SUPPORT_READDIR64_R;
> +}
> +
> +const char *
> +support_readdir_function (enum support_readdir_op op)
> +{
> +  switch (op)
> +    {
> +      case SUPPORT_READDIR:
> +        return "readdir";
> +      case SUPPORT_READDIR64:
> +        return "readdir64";
> +      case SUPPORT_READDIR_R:
> +        return "readdir_r";
> +      case SUPPORT_READDIR64_R:
> +        return "readdir64_r";
> +      case SUPPORT_READDIR64_COMPAT:
> +        return "readdir64@GBLIC_2.1";
> +      case SUPPORT_READDIR64_R_COMPAT:
> +        return "readdir64_r@GBLIC_2.1";
> +    }
> +  FAIL_EXIT1 ("invalid support_readdir_op constant: %d", op);
> +}
> +
> +unsigned int
> +support_readdir_inode_width (enum support_readdir_op op)
> +{
> +  switch (op)
> +    {
> +      case SUPPORT_READDIR:
> +      case SUPPORT_READDIR_R:
> +        return sizeof ((struct dirent) { 0, }.d_ino) * 8;
> +      case SUPPORT_READDIR64:
> +      case SUPPORT_READDIR64_R:
> +        return sizeof ((struct dirent64) { 0, }.d_ino) * 8;
> +      case SUPPORT_READDIR64_COMPAT:
> +      case SUPPORT_READDIR64_R_COMPAT:
> +        return sizeof ((struct __old_dirent64) { 0, }.d_ino) * 8;
> +    }
> +  FAIL_EXIT1 ("invalid support_readdir_op constant: %d", op);
> +}
> +
> +bool
> +support_readdir_r_variant (enum support_readdir_op op)
> +{
> +  switch (op)
> +    {
> +      case SUPPORT_READDIR:
> +      case SUPPORT_READDIR64:
> +      case SUPPORT_READDIR64_COMPAT:
> +        return false;
> +      case SUPPORT_READDIR_R:
> +      case SUPPORT_READDIR64_R:
> +      case SUPPORT_READDIR64_R_COMPAT:
> +        return true;
> +    }
> +  FAIL_EXIT1 ("invalid support_readdir_op constant: %d", op);
> +}
> +
> +static bool
> +copy_dirent (struct support_dirent *dst, struct dirent *src)
> +{
> +  if (src == NULL)
> +    return false;
> +  dst->d_ino = src->d_ino;
> +#ifdef _DIRENT_HAVE_D_OFF
> +  dst->d_off = src->d_off;
> +#else
> +  dst->d_off = 0;
> +#endif
> +  dst->d_type = src->d_type;
> +  dst->d_name = xstrdup (src->d_name);
> +  return true;
> +}
> +
> +static bool
> +copy_dirent64 (struct support_dirent *dst, struct dirent64 *src)
> +{
> +  if (src == NULL)
> +    return false;
> +  dst->d_ino = src->d_ino;
> +#ifdef _DIRENT_HAVE_D_OFF
> +  dst->d_off = src->d_off;
> +#else
> +  dst->d_off = 0;
> +#endif
> +  dst->d_type = src->d_type;
> +  dst->d_name = xstrdup (src->d_name);
> +  return true;
> +}
> +
> +static bool
> +copy_old_dirent64 (struct support_dirent *dst, struct __old_dirent64 *src)
> +{
> +  if (src == NULL)
> +    return false;
> +  dst->d_ino = src->d_ino;
> +#ifdef _DIRENT_HAVE_D_OFF
> +  dst->d_off = src->d_off;
> +#else
> +  dst->d_off = 0;
> +#endif
> +  dst->d_type = src->d_type;
> +  dst->d_name = xstrdup (src->d_name);
> +  return true;
> +}
> +
> +bool
> +support_readdir (DIR *stream, enum support_readdir_op op,
> +                 struct support_dirent *e)
> +{
> +  free (e->d_name);
> +  e->d_name = NULL;
> +  switch (op)
> +    {
> +    case SUPPORT_READDIR:
> +      return copy_dirent (e, xreaddir (stream));
> +    case SUPPORT_READDIR64:
> +      return copy_dirent64 (e, xreaddir64 (stream));
> +
> +      /* The functions readdir_r, readdir64_r were deprecated in glibc 2.24.  */
> +      DIAG_PUSH_NEEDS_COMMENT;
> +      DIAG_IGNORE_NEEDS_COMMENT (4.9, "-Wdeprecated-declarations");
> +
> +    case SUPPORT_READDIR_R:
> +      {
> +        struct dirent buf;
> +        if (!xreaddir_r (stream, &buf))
> +          return false;
> +        return copy_dirent (e, &buf);
> +      }
> +    case SUPPORT_READDIR64_R:
> +      {
> +        struct dirent64 buf;
> +        if (!xreaddir64_r (stream, &buf))
> +          return false;
> +        return copy_dirent64 (e, &buf);
> +      }
> +
> +      DIAG_POP_NEEDS_COMMENT;
> +
> +    case SUPPORT_READDIR64_COMPAT:
> +      if (readdir64_compat == NULL)
> +        FAIL_EXIT1 ("readdir64 compat function not implemented");
> +      return copy_old_dirent64 (e, readdir64_compat (stream));
> +
> +    case SUPPORT_READDIR64_R_COMPAT:
> +      {
> +        if (readdir64_r_compat == NULL)
> +          FAIL_EXIT1 ("readdir64_r compat function not implemented");
> +        struct __old_dirent64 buf;
> +        struct __old_dirent64 *e1;
> +        int ret = readdir64_r_compat (stream, &buf, &e1);
> +        if (ret != 0)
> +          {
> +            errno = ret;
> +            FAIL ("readdir64_r@GLIBC_2.1: %m");
> +            return false;
> +          }
> +        if (e1 == NULL)
> +          return false;
> +        return copy_old_dirent64 (e, e1);
> +      }
> +    }
> +  FAIL_EXIT1 ("support_readdir: invalid op argument %d", (int) op);
> +}
> +
> +void
> +support_readdir_expect_error (DIR *stream, enum support_readdir_op op,
> +                              int expected)
> +{
> +  switch (op)
> +    {
> +    case SUPPORT_READDIR:
> +      errno = 0;
> +      TEST_VERIFY (readdir (stream) == NULL);
> +      TEST_COMPARE (errno, expected);
> +      return;
> +    case SUPPORT_READDIR64:
> +      errno = 0;
> +      TEST_VERIFY (readdir64 (stream) == NULL);
> +      TEST_COMPARE (errno, expected);
> +      return;
> +
> +      /* The functions readdir_r, readdir64_r were deprecated in glibc 2.24.  */
> +      DIAG_PUSH_NEEDS_COMMENT;
> +      DIAG_IGNORE_NEEDS_COMMENT (4.9, "-Wdeprecated-declarations");
> +
> +    case SUPPORT_READDIR_R:
> +      {
> +        struct dirent buf;
> +        struct dirent *e;
> +        errno = readdir_r (stream, &buf, &e);
> +        TEST_COMPARE (errno, expected);;
> +      }
> +      return;
> +    case SUPPORT_READDIR64_R:
> +      {
> +        struct dirent64 buf;
> +        struct dirent64 *e;
> +        errno = readdir64_r (stream, &buf, &e);
> +        TEST_COMPARE (errno, expected);;
> +      }
> +      return;
> +
> +      DIAG_POP_NEEDS_COMMENT;
> +
> +    case SUPPORT_READDIR64_COMPAT:
> +      if (readdir64_compat == NULL)
> +        FAIL_EXIT1 ("readdir64_r compat function not implemented");
> +      errno = 0;
> +      TEST_VERIFY (readdir64_compat (stream) == NULL);
> +      TEST_COMPARE (errno, expected);
> +      return;
> +    case SUPPORT_READDIR64_R_COMPAT:
> +      {
> +        if (readdir64_r_compat == NULL)
> +          FAIL_EXIT1 ("readdir64_r compat function not implemented");
> +        struct __old_dirent64 buf;
> +        struct __old_dirent64 *e;
> +        errno = readdir64_r_compat (stream, &buf, &e);
> +        TEST_COMPARE (errno, expected);
> +      }
> +      return;
> +    }
> +  FAIL_EXIT1 ("support_readdir_expect_error: invalid op argument %d",
> +              (int) op);
> +}
> diff --git a/support/tst-support_readdir.c b/support/tst-support_readdir.c
> new file mode 100644
> index 0000000000..c84d523b83
> --- /dev/null
> +++ b/support/tst-support_readdir.c
> @@ -0,0 +1,71 @@
> +/* Test the support_readdir function.
> +   Copyright (C) 2024 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#include <support/readdir.h>
> +
> +#include <fcntl.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <support/check.h>
> +#include <support/xdirent.h>
> +#include <support/xunistd.h>
> +
> +static int
> +do_test (void)
> +{
> +  DIR *reference_stream = xopendir (".");
> +  struct dirent64 *reference = xreaddir64 (reference_stream);
> +
> +  for (enum support_readdir_op op = 0; op <= support_readdir_op_last (); ++op)
> +    {
> +      DIR *stream = xopendir (".");
> +      struct support_dirent e;
> +      memset (&e, 0xcc, sizeof (e));
> +      e.d_name = NULL;
> +      TEST_VERIFY (support_readdir (stream, op, &e));
> +      TEST_COMPARE (e.d_ino, reference->d_ino);
> +#ifdef _DIRENT_HAVE_D_OFF
> +      TEST_COMPARE (e.d_off, reference->d_off);
> +#else
> +      TEST_COMPARE (e.d_off, 0);
> +#endif
> +      TEST_COMPARE (e.d_type, reference->d_type);
> +      TEST_COMPARE_STRING (e.d_name, reference->d_name);
> +      free (e.d_name);
> +      xclosedir (stream);
> +    }
> +
> +  xclosedir (reference_stream);
> +
> +  /* Error injection test.  */
> +  int devnull = xopen ("/dev/null", O_RDONLY, 0);
> +  for (enum support_readdir_op op = 0; op <= support_readdir_op_last (); ++op)
> +    {
> +      DIR *stream = xopendir (".");
> +      /* A descriptor incompatible with readdir.  */
> +      xdup2 (devnull, dirfd (stream));
> +      errno = -1;
> +      support_readdir_expect_error (stream, op, ENOTDIR);
> +      xclosedir (stream);
> +    }
> +  xclose (devnull);
> +
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
  

* Adhemerval Zanella Netto:

> On 30/08/24 16:52, Florian Weimer wrote:
>> It allows to read directories using the six readdir variants
>> without writing type-specific code or using skeleton files
>> that are compiled four times.
>> 
>> The readdir_r subtest for support_readdir_expect_error revealed
>> bug 32124.
>
> Should we just build this for SHARED? Or maybe just support SUPPORT_READDIR,
> and SUPPORT_READDIR64 such scenario?

There is no SHARED build, only libsupport_nonshared.a.  To me, using
dlopen seems the simplest approach because the compat symbol has the
same version everywhere if it actually exists.

We cannot use a weak symbol because we do not support weak versions.

Thanks,
Florian
  

On 09/09/24 09:56, Florian Weimer wrote:
> * Adhemerval Zanella Netto:
> 
>> On 30/08/24 16:52, Florian Weimer wrote:
>>> It allows to read directories using the six readdir variants
>>> without writing type-specific code or using skeleton files
>>> that are compiled four times.
>>>
>>> The readdir_r subtest for support_readdir_expect_error revealed
>>> bug 32124.
>>
>> Should we just build this for SHARED? Or maybe just support SUPPORT_READDIR,
>> and SUPPORT_READDIR64 such scenario?
> 
> There is no SHARED build, only libsupport_nonshared.a.  To me, using
> dlopen seems the simplest approach because the compat symbol has the
> same version everywhere if it actually exists.
> 
> We cannot use a weak symbol because we do not support weak versions.

Yeah, I am aware.  I am just thinking if there is  a better way to warn a 
possible issue if these interface are used on static tests.  It should be
ok for now.