support: Implement <support/descriptors.h> to track file descriptors

  2018-11-27  Florian Weimer  <fweimer@redhat.com>

	* support/descriptors.h: New file.
	* support/check.h (support_record_failure_is_failed): Declare.
	* support/support_descriptors.c: Likewise.
	* support/tst-support_descriptors.c: Likewise.
	* support/support_record_faile.c
	(support_record_failure_is_failed): New function.
	* support/Makefile (libsupport-routines): Add support_descriptors.
	(tests): Add tst-support_descriptors.
  

On 11/27/18 12:25 PM, Florian Weimer wrote:
> 2018-11-27  Florian Weimer  <fweimer@redhat.com>
> 
> 	* support/descriptors.h: New file.
> 	* support/check.h (support_record_failure_is_failed): Declare.
> 	* support/support_descriptors.c: Likewise.
> 	* support/tst-support_descriptors.c: Likewise.
> 	* support/support_record_faile.c
> 	(support_record_failure_is_failed): New function.
> 	* support/Makefile (libsupport-routines): Add support_descriptors.
> 	(tests): Add tst-support_descriptors.

I was wondering how you were going to implement this :-)

Two comments below.

OK to commit to master if you resolve those two comments.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> diff --git a/support/Makefile b/support/Makefile
> index a2536980d1..93a5143016 100644
> --- a/support/Makefile
> +++ b/support/Makefile
> @@ -46,6 +46,7 @@ libsupport-routines = \
>    support_chroot \
>    support_copy_file_range \
>    support_descriptor_supports_holes \
> +  support_descriptors \

OK.

>    support_enter_mount_namespace \
>    support_enter_network_namespace \
>    support_format_address_family \
> @@ -195,6 +196,7 @@ tests = \
>    tst-support-namespace \
>    tst-support_blob_repeat \
>    tst-support_capture_subprocess \
> +  tst-support_descriptors \

OK.

>    tst-support_format_dns_packet \
>    tst-support_quote_blob \
>    tst-support_quote_string \
> diff --git a/support/check.h b/support/check.h
> index e6765289f2..7ea9a86a9c 100644
> --- a/support/check.h
> +++ b/support/check.h
> @@ -183,6 +183,10 @@ int support_report_failure (int status)
>  /* Internal function used to test the failure recording framework.  */
>  void support_record_failure_reset (void);
>  
> +/* Returns true or false depending on whether there have been test
> +   failures or not.  */
> +int support_record_failure_is_failed (void);

OK.

> +
>  __END_DECLS
>  
>  #endif /* SUPPORT_CHECK_H */
> diff --git a/support/descriptors.h b/support/descriptors.h
> new file mode 100644
> index 0000000000..8ec4cbbdfb
> --- /dev/null
> +++ b/support/descriptors.h
> @@ -0,0 +1,47 @@
> +/* Monitoring file descriptor usage.
> +   Copyright (C) 2018 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
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#ifndef SUPPORT_DESCRIPTORS_H
> +#define SUPPORT_DESCRIPTORS_H
> +
> +#include <stdio.h>
> +
> +/* Opaque pointer, for capturing file descriptor lists.  */
> +struct support_descriptors;
> +
> +/* Record the currently open file descriptors and store them in the
> +   returned list.  Terminate the process if the listing operation
> +   fails.  */
> +struct support_descriptors *support_descriptors_list (void);
> +
> +/* Deallocate the list of descriptors.  */
> +void support_descriptors_free (struct support_descriptors *);
> +
> +/* Write the list of descriptors to STREAM, adding PREFIX to each
> +   line.  */
> +void support_descriptors_dump (struct support_descriptors *,
> +                               const char *prefix, FILE *stream);
> +
> +/* Check for file descriptor leaks and other file descriptor changes:
> +   Compare the current list of descriptors with the passed list.
> +   Record a test failure if there are additional open descriptors,
> +   descriptors have been closed, or if a change in file descriptor can
> +   be detected.  */
> +void support_descriptors_check (struct support_descriptors *);
> +
> +#endif /* SUPPORT_DESCRIPTORS_H */
> diff --git a/support/support_descriptors.c b/support/support_descriptors.c
> new file mode 100644
> index 0000000000..99ca566802
> --- /dev/null
> +++ b/support/support_descriptors.c
> @@ -0,0 +1,265 @@
> +/* Monitoring file descriptor usage.
> +   Copyright (C) 2018 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
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <dirent.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <support/check.h>
> +#include <support/support.h>
> +#include <sys/stat.h>
> +#include <sys/sysmacros.h>
> +#include <xunistd.h>
> +
> +struct descriptor
> +{
> +  int fd;
> +  char *link_target;
> +  dev_t dev;
> +  ino64_t ino;
> +};

OK.

> +
> +/* Used with qsort.  */
> +static int
> +descriptor_compare (const void *l, const void *r)
> +{
> +  const struct descriptor *left = l;
> +  const struct descriptor *right = r;
> +  /* Cannot overflow due to limited file descriptor range.  */
> +  return left->fd - right->fd;
> +}
> +
> +#define DYNARRAY_STRUCT descriptor_list
> +#define DYNARRAY_ELEMENT struct descriptor
> +#define DYNARRAY_PREFIX descriptor_list_
> +#define DYNARRAY_ELEMENT_FREE(e) free ((e)->link_target)
> +#define DYNARRAY_INITIAL_SIZE 0
> +#include <malloc/dynarray-skeleton.c>
> +
> +struct support_descriptors
> +{
> +  struct descriptor_list list;
> +};
> +
> +struct support_descriptors *
> +support_descriptors_list (void)
> +{
> +  struct support_descriptors *result = xmalloc (sizeof (*result));
> +  descriptor_list_init (&result->list);
> +
> +  DIR *fds = opendir ("/proc/self/fd");

This makes support/descriptors.h dependent on /proc/self/fd?

Can we exit UNSUPPORTED if it doesn't exist?

> +  if (fds == NULL)
> +    FAIL_EXIT1 ("opendir (\"/proc/self/fd\"): %m");
> +
> +  while (true)
> +    {
> +      errno = 0;
> +      struct dirent64 *e = readdir64 (fds);
> +      if (e == NULL)
> +        {
> +          if (errno != 0)
> +            FAIL_EXIT1 ("readdir: %m");
> +          break;
> +        }
> +
> +      if (e->d_name[0] == '.')
> +        continue;
> +
> +      char *endptr;
> +      long fd = strtol (e->d_name, &endptr, 10);
> +      if (*endptr != '\0' || fd < 0 || fd > INT_MAX)
> +        FAIL_EXIT1 ("readdir: invalid file descriptor name: /proc/self/fd/%s",
> +                    e->d_name);
> +
> +      /* Skip the descriptor which is used to enumerate the
> +         descriptors.  */
> +      if (fd == dirfd (fds))
> +        continue;
> +
> +      char path[100];

Why not PATH_MAX?

We should not stat a truncated fd path since it might match another fd?

	 assert (strlen(e->d_name) < PATH_MAX);

> +      snprintf (path, sizeof (path), "/proc/self/fd/%ld", fd);
> +      char *target = xreadlink (path);
> +      struct stat64 st;
> +      if (fstat64 (fd, &st) != 0)
> +        FAIL_EXIT1 ("readdir: fstat64 (%ld) failed: %m", fd);
> +
> +      struct descriptor *item = descriptor_list_emplace (&result->list);
> +      if (item == NULL)
> +        FAIL_EXIT1 ("descriptor_list_emplace: %m");
> +      item->fd = fd;
> +      item->link_target = target;
> +      item->dev = st.st_dev;
> +      item->ino = st.st_ino;
> +    }
> +
> +  closedir (fds);
> +
> +  qsort (descriptor_list_begin (&result->list),
> +         descriptor_list_size (&result->list),
> +         sizeof (struct descriptor), descriptor_compare);
> +
> +  return result;
> +}
> +
> +void
> +support_descriptors_free (struct support_descriptors *descrs)
> +{
> +  descriptor_list_free (&descrs->list);
> +  free (descrs);
> +}
> +
> +void
> +support_descriptors_dump (struct support_descriptors *descrs,
> +                          const char *prefix, FILE *fp)
> +{
> +  struct descriptor *end = descriptor_list_end (&descrs->list);
> +  for (struct descriptor *d = descriptor_list_begin (&descrs->list);
> +       d != end; ++d)
> +    {
> +      char *quoted = support_quote_string (d->link_target);
> +      fprintf (fp, "%s%d: target=\"%s\" major=%lld minor=%lld ino=%lld\n",
> +               prefix, d->fd, quoted,
> +               (long long int) major (d->dev),
> +               (long long int) minor (d->dev),
> +               (long long int) d->ino);
> +      free (quoted);
> +    }
> +}
> +
> +static void
> +dump_mismatch (bool *first,
> +               struct support_descriptors *descrs,
> +               struct support_descriptors *current)
> +{
> +  if (*first)
> +    *first = false;
> +  else
> +    return;
> +
> +  puts ("error: Differences found in descriptor set");
> +  puts ("Reference descriptor set:");
> +  support_descriptors_dump (descrs, "  ", stdout);
> +  puts ("Current descriptor set:");
> +  support_descriptors_dump (current, "  ", stdout);
> +  puts ("Differences:");
> +}
> +
> +static void
> +report_closed_descriptor (bool *first,
> +                          struct support_descriptors *descrs,
> +                          struct support_descriptors *current,
> +                          struct descriptor *left)
> +{
> +  support_record_failure ();
> +  dump_mismatch (first, descrs, current);
> +  printf ("error: descriptor %d was closed\n", left->fd);
> +}
> +
> +static void
> +report_opened_descriptor (bool *first,
> +                          struct support_descriptors *descrs,
> +                          struct support_descriptors *current,
> +                          struct descriptor *right)
> +{
> +  support_record_failure ();
> +  dump_mismatch (first, descrs, current);
> +  char *quoted = support_quote_string (right->link_target);
> +  printf ("error: descriptor %d was opened (\"%s\")\n", right->fd, quoted);
> +  free (quoted);
> +}
> +
> +void
> +support_descriptors_check (struct support_descriptors *descrs)
> +{
> +  struct support_descriptors *current = support_descriptors_list ();
> +
> +  struct descriptor *left = descriptor_list_begin (&descrs->list);
> +  struct descriptor *left_end = descriptor_list_end (&descrs->list);
> +  struct descriptor *right = descriptor_list_begin (&current->list);
> +  struct descriptor *right_end = descriptor_list_end (&current->list);
> +
> +  bool first = true;
> +  while (left != left_end && right != right_end)
> +    {
> +      if (left->fd == right->fd)
> +        {
> +          if (strcmp (left->link_target, right->link_target) != 0)
> +            {
> +              support_record_failure ();
> +              char *left_quoted = support_quote_string (left->link_target);
> +              char *right_quoted = support_quote_string (right->link_target);
> +              dump_mismatch (&first, descrs, current);
> +              printf ("error: descriptor %d changed from \"%s\" to \"%s\"\n",
> +                      left->fd, left_quoted, right_quoted);
> +              free (left_quoted);
> +              free (right_quoted);
> +            }
> +          if (left->dev != right->dev)
> +            {
> +              support_record_failure ();
> +              dump_mismatch (&first, descrs, current);
> +              printf ("error: descriptor %d changed device"
> +                      " from %lld:%lld to %lld:%lld\n",
> +                      left->fd,
> +                      (long long int) major (left->dev),
> +                      (long long int) minor (left->dev),
> +                      (long long int) major (right->dev),
> +                      (long long int) minor (right->dev));
> +            }
> +          if (left->ino != right->ino)
> +            {
> +              support_record_failure ();
> +              dump_mismatch (&first, descrs, current);
> +              printf ("error: descriptor %d changed ino from %lld to %lld\n",
> +                      left->fd,
> +                      (long long int) left->ino, (long long int) right->ino);
> +            }
> +          ++left;
> +          ++right;
> +        }
> +      else if (left->fd < right->fd)
> +        {
> +          /* Gap on the right.  */
> +          report_closed_descriptor (&first, descrs, current, left);
> +          ++left;
> +        }
> +      else
> +        {
> +          /* Gap on the left.  */
> +          TEST_VERIFY_EXIT (left->fd > right->fd);
> +          report_opened_descriptor (&first, descrs, current, right);
> +          ++right;
> +        }
> +    }
> +
> +  while (left != left_end)
> +    {
> +      /* Closed descriptors (more descriptors on the left).  */
> +      report_closed_descriptor (&first, descrs, current, left);
> +      ++left;
> +    }
> +
> +  while (right != right_end)
> +    {
> +      /* Opened descriptors (more descriptors on the right).  */
> +      report_opened_descriptor (&first, descrs, current, right);
> +      ++right;
> +    }
> +
> +  support_descriptors_free (current);
> +}

OK.

> diff --git a/support/support_record_failure.c b/support/support_record_failure.c
> index 356798f556..17ab1d80ef 100644
> --- a/support/support_record_failure.c
> +++ b/support/support_record_failure.c
> @@ -104,3 +104,11 @@ support_record_failure_reset (void)
>    __atomic_store_n (&state->failed, 0, __ATOMIC_RELAXED);
>    __atomic_add_fetch (&state->counter, 0, __ATOMIC_RELAXED);
>  }
> +
> +int
> +support_record_failure_is_failed (void)
> +{
> +  /* Relaxed MO is sufficient because we need (blocking) external
> +     synchronization for reliable test error reporting anyway.  */
> +  return __atomic_load_n (&state->failed, __ATOMIC_RELAXED);
> +}

OK.

> diff --git a/support/tst-support_descriptors.c b/support/tst-support_descriptors.c
> new file mode 100644
> index 0000000000..34c5e1673f
> --- /dev/null
> +++ b/support/tst-support_descriptors.c
> @@ -0,0 +1,167 @@
> +/* Tests for monitoring file descriptor usage.
> +   Copyright (C) 2018 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
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <fcntl.h>
> +#include <stdbool.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <support/capture_subprocess.h>
> +#include <support/check.h>
> +#include <support/descriptors.h>
> +#include <support/support.h>
> +#include <support/xunistd.h>
> +
> +/* This is the next free descriptor that the subprocess will pick.  */
> +static int free_descriptor;
> +
> +static void
> +subprocess_no_change (void *closure)
> +{
> +  struct support_descriptors *descrs = support_descriptors_list ();
> +  int fd = xopen ("/dev/null", O_WRONLY, 0);
> +  TEST_COMPARE (fd, free_descriptor);
> +  xclose (fd);
> +  support_descriptors_free (descrs);
> +}
> +
> +static void
> +subprocess_closed_descriptor (void *closure)
> +{
> +  int fd = xopen ("/dev/null", O_WRONLY, 0);
> +  TEST_COMPARE (fd, free_descriptor);
> +  struct support_descriptors *descrs = support_descriptors_list ();
> +  xclose (fd);
> +  support_descriptors_check (descrs); /* Will report failure.  */
> +  puts ("EOT");
> +  support_descriptors_free (descrs);
> +}
> +
> +static void
> +subprocess_opened_descriptor (void *closure)
> +{
> +  struct support_descriptors *descrs = support_descriptors_list ();
> +  int fd = xopen ("/dev/null", O_WRONLY, 0);
> +  TEST_COMPARE (fd, free_descriptor);
> +  support_descriptors_check (descrs); /* Will report failure.  */
> +  puts ("EOT");
> +  support_descriptors_free (descrs);
> +}
> +
> +static void
> +subprocess_changed_descriptor (void *closure)
> +{
> +  int fd = xopen ("/dev/null", O_WRONLY, 0);
> +  TEST_COMPARE (fd, free_descriptor);
> +  struct support_descriptors *descrs = support_descriptors_list ();
> +  xclose (fd);
> +  TEST_COMPARE (xopen ("/dev", O_DIRECTORY | O_RDONLY, 0), fd);
> +  support_descriptors_check (descrs); /* Will report failure.  */
> +  puts ("EOT");
> +  support_descriptors_free (descrs);
> +}
> +
> +static void
> +report_subprocess_output (const char *name,
> +                          struct support_capture_subprocess *proc)
> +{
> +  printf ("info: BEGIN %s output\n"
> +          "%s"
> +          "info: END %s output\n",
> +          name, proc->out.buffer, name);
> +}
> +
> +static int
> +do_test (void)
> +{
> +  puts ("info: initial descriptor set");
> +  {
> +    struct support_descriptors *descrs = support_descriptors_list ();
> +    support_descriptors_dump (descrs, "info:  ", stdout);
> +    support_descriptors_free (descrs);
> +  }
> +
> +  free_descriptor = xopen ("/dev/null", O_WRONLY, 0);
> +  puts ("info: descriptor set with additional free descriptor");
> +  {
> +    struct support_descriptors *descrs = support_descriptors_list ();
> +    support_descriptors_dump (descrs, "info:  ", stdout);
> +    support_descriptors_free (descrs);
> +  }
> +  TEST_VERIFY (free_descriptor >= 3);
> +  xclose (free_descriptor);
> +
> +  struct support_capture_subprocess proc = support_capture_subprocess
> +    (&subprocess_no_change, NULL);
> +  support_capture_subprocess_check (&proc, "subprocess_no_change",
> +                                    0, sc_allow_none);
> +  support_capture_subprocess_free (&proc);
> +
> +  /* Use an explicit flag to preserve failure status across
> +     support_record_failure_reset calls.  */
> +  bool good = true;
> +
> +  char *expected = xasprintf ("\nDifferences:\n"
> +                              "error: descriptor %d was closed\n"
> +                              "EOT\n",
> +                              free_descriptor);
> +  good = good && !support_record_failure_is_failed ();
> +  proc = support_capture_subprocess (&subprocess_closed_descriptor, NULL);
> +  good = good && support_record_failure_is_failed ();
> +  support_record_failure_reset (); /* Discard the reported error.  */
> +  report_subprocess_output ("subprocess_closed_descriptor", &proc);
> +  TEST_VERIFY (strstr (proc.out.buffer, expected) != NULL);
> +  support_capture_subprocess_check (&proc, "subprocess_closed_descriptor",
> +                                    0, sc_allow_stdout);
> +  support_capture_subprocess_free (&proc);
> +  free (expected);
> +
> +  expected = xasprintf ("\nDifferences:\n"
> +                        "error: descriptor %d was opened (\"/dev/null\")\n"
> +                        "EOT\n",
> +                        free_descriptor);
> +  good = good && !support_record_failure_is_failed ();
> +  proc = support_capture_subprocess (&subprocess_opened_descriptor, NULL);
> +  good = good && support_record_failure_is_failed ();
> +  support_record_failure_reset (); /* Discard the reported error.  */
> +  report_subprocess_output ("subprocess_opened_descriptor", &proc);
> +  TEST_VERIFY (strstr (proc.out.buffer, expected) != NULL);
> +  support_capture_subprocess_check (&proc, "subprocess_opened_descriptor",
> +                                    0, sc_allow_stdout);
> +  support_capture_subprocess_free (&proc);
> +  free (expected);
> +
> +  expected = xasprintf ("\nDifferences:\n"
> +                        "error: descriptor %d changed from \"/dev/null\""
> +                        " to \"/dev\"\n"
> +                        "error: descriptor %d changed ino ",
> +                        free_descriptor, free_descriptor);
> +  good = good && !support_record_failure_is_failed ();
> +  proc = support_capture_subprocess (&subprocess_changed_descriptor, NULL);
> +  good = good && support_record_failure_is_failed ();
> +  support_record_failure_reset (); /* Discard the reported error.  */
> +  report_subprocess_output ("subprocess_changed_descriptor", &proc);
> +  TEST_VERIFY (strstr (proc.out.buffer, expected) != NULL);
> +  support_capture_subprocess_check (&proc, "subprocess_changed_descriptor",
> +                                    0, sc_allow_stdout);
> +  support_capture_subprocess_free (&proc);
> +  free (expected);
> +
> +  return !good;
> +}
> +
> +#include <support/test-driver.c>
> 

OK.
  

On Thu, Nov 29, 2018 at 10:44:10PM -0500, Carlos O'Donell wrote:
> On 11/27/18 12:25 PM, Florian Weimer wrote:
[...]
> > +      char path[100];
> 
> Why not PATH_MAX?
> 
> We should not stat a truncated fd path since it might match another fd?
> 
> 	 assert (strlen(e->d_name) < PATH_MAX);
> 
> > +      snprintf (path, sizeof (path), "/proc/self/fd/%ld", fd);

Or just use xasprintf instead.
  

* Carlos O'Donell:

>> +  DIR *fds = opendir ("/proc/self/fd");
>
> This makes support/descriptors.h dependent on /proc/self/fd?
>
> Can we exit UNSUPPORTED if it doesn't exist?

I think even Hurd has it.

I think it's a severe environmental problem if /proc does not exist, so
a failure is acceptable in this case.

>> +      char path[100];
>
> Why not PATH_MAX?

It's not defined on Hurd.

> We should not stat a truncated fd path since it might match another fd?
>
> 	 assert (strlen(e->d_name) < PATH_MAX);

We would get a compile-time failure with current GCC if truncation is
possible, I think:

>> +      snprintf (path, sizeof (path), "/proc/self/fd/%ld", fd);

I could try to count more precisely how many bytes are actually needed,
but that seemed like a waste of mental resources.  Should I use
xasprintf instead, to make clear that there is no truncation?

Thanks,
Florian
  

27.11.2018 18:25 Florian Weimer <fweimer@redhat.com> wrote:
> 
> 
> 2018-11-27  Florian Weimer  <fweimer@redhat.com>
> 
> 	* support/descriptors.h: New file.
> 	* support/check.h (support_record_failure_is_failed): Declare.

OK

> 	* support/support_descriptors.c: Likewise.
> 	* support/tst-support_descriptors.c: Likewise.

These "Likewise" look like "Declare" but I'm pretty sure you meant
"New file".  Please use "New file" instead of the first "Likewise"
or maybe better just reorder the lines so that "Likewise" follows
"New file".  I think it is reasonable to put "check.h" in the first
line, this sorts the entries alphabetically and resolves the problem.

> 	* support/support_record_faile.c

^faile^failure

> 	(support_record_failure_is_failed): New function.
> [...]
> +static int
> +do_test (void)
> +{
> +  puts ("info: initial descriptor set");
> +  {
> +    struct support_descriptors *descrs = support_descriptors_list ();
> +    support_descriptors_dump (descrs, "info:  ", stdout);

Are you sure you need two spaces here? --------^^

The same again few lines below.

Otherwise I can confirm that it builds and runs correctly on x86_64
and there are more reviews from Carlos and Dmitry.

Regards,

Rafal
  

* Rafal Luzynski:

> 27.11.2018 18:25 Florian Weimer <fweimer@redhat.com> wrote:
>> 
>> 
>> 2018-11-27  Florian Weimer  <fweimer@redhat.com>
>> 
>> 	* support/descriptors.h: New file.
>> 	* support/check.h (support_record_failure_is_failed): Declare.
>
> OK
>
>> 	* support/support_descriptors.c: Likewise.
>> 	* support/tst-support_descriptors.c: Likewise.
>
> These "Likewise" look like "Declare" but I'm pretty sure you meant
> "New file".  Please use "New file" instead of the first "Likewise"
> or maybe better just reorder the lines so that "Likewise" follows
> "New file".  I think it is reasonable to put "check.h" in the first
> line, this sorts the entries alphabetically and resolves the problem.
>
>> 	* support/support_record_faile.c
>
> ^faile^failure

Thanks, will fix.

>> 	(support_record_failure_is_failed): New function.
>> [...]
>> +static int
>> +do_test (void)
>> +{
>> +  puts ("info: initial descriptor set");
>> +  {
>> +    struct support_descriptors *descrs = support_descriptors_list ();
>> +    support_descriptors_dump (descrs, "info:  ", stdout);
>
> Are you sure you need two spaces here? --------^^
>
> The same again few lines below.

Yes, it's for expected/actual alignment in the output.

Thanks,
Florian
  

On 11/29/18 11:43 PM, Florian Weimer wrote:
> I could try to count more precisely how many bytes are actually needed,
> but that seemed like a waste of mental resources.  Should I use
> xasprintf instead, to make clear that there is no truncation?

Yes, I think xasprintf solves all the problems.