[PATCH] misc: Fix tst-select timeout handling (BZ#27648)

[PATCH] misc: Fix tst-select timeout handling (BZ#27648)

[Adhemerval Zanella]

Instead of polling the stderr, create two pipes and fork to check
if child timeout as expected similar to tst-pselect.c.  Also lower
the timeout value.

Checked on x86_64-linux-gnu.
---
 misc/tst-select.c | 85 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 55 insertions(+), 30 deletions(-)

diff --git a/misc/tst-select.c b/misc/tst-select.c
index 7c310256c5..530181c264 100644
--- a/misc/tst-select.c
+++ b/misc/tst-select.c
@@ -16,54 +16,79 @@
    License along with the GNU C Library; if not, see
    <https://www.gnu.org/licenses/>.  */
 
-#include <time.h>
 #include <errno.h>
-#include <stdbool.h>
-#include <sys/select.h>
+#include <support/capture_subprocess.h>
 #include <support/check.h>
-#include <support/xtime.h>
 #include <support/timespec.h>
+#include <support/xunistd.h>
+#include <support/xtime.h>
 
-#define TST_SELECT_TIMEOUT 1
-#define TST_SELECT_FD_ERR 2
+struct child_args
+{
+  int fds[2][2];
+  struct timeval tmo;
+};
 
-static int
-test_select_timeout (bool zero_tmo)
+static void
+do_test_child (void *clousure)
 {
-  const int fds = TST_SELECT_FD_ERR;
-  int timeout = TST_SELECT_TIMEOUT;
-  struct timeval to = { 0, 0 };
-  struct timespec ts;
-  fd_set rfds;
+  struct child_args *args = (struct child_args *) clousure;
 
-  FD_ZERO (&rfds);
-  FD_SET (fds, &rfds);
+  close (args->fds[0][1]);
+  close (args->fds[1][0]);
 
-  if (zero_tmo)
-    timeout = 0;
+  fd_set rfds;
+  FD_ZERO (&rfds);
+  FD_SET (args->fds[0][0], &rfds);
 
-  to.tv_sec = timeout;
-  ts = xclock_now (CLOCK_REALTIME);
-  ts = timespec_add (ts, (struct timespec) { timeout, 0 });
+  struct timespec ts = xclock_now (CLOCK_REALTIME);
+  ts = timespec_add (ts, (struct timespec) { args->tmo.tv_sec, 0 });
 
-  /* Wait for timeout.  */
-  int ret = select (fds + 1, &rfds, NULL, NULL, &to);
-  if (ret == -1)
-    FAIL_EXIT1 ("select failed: %m\n");
+  int r = select (args->fds[0][0] + 1, &rfds, NULL, NULL, &args->tmo);
+  TEST_COMPARE (r, 0);
 
   TEST_TIMESPEC_NOW_OR_AFTER (CLOCK_REALTIME, ts);
 
-  return 0;
+  xwrite (args->fds[1][1], "foo", 3);
 }
 
 static int
 do_test (void)
 {
-  /* Check if select exits immediately.  */
-  test_select_timeout (true);
-
-  /* Check if select exits after specified timeout.  */
-  test_select_timeout (false);
+  struct child_args args;
+
+  xpipe (args.fds[0]);
+  xpipe (args.fds[1]);
+
+  /* The child select should timeout and write on its pipe end.  */
+  args.tmo = (struct timeval) { .tv_sec = 0, .tv_usec = 250000 };
+  {
+    struct support_capture_subprocess result;
+    result = support_capture_subprocess (do_test_child, &args);
+    support_capture_subprocess_check (&result, "tst-select-child", 0,
+				      sc_allow_none);
+  }
+
+  /* Same with simulating a polling.  */
+  args.tmo = (struct timeval) { .tv_sec = 0, .tv_usec = 0 };
+  {
+    struct support_capture_subprocess result;
+    result = support_capture_subprocess (do_test_child, &args);
+    support_capture_subprocess_check (&result, "tst-select-child", 0,
+				      sc_allow_none);
+  }
+
+  xclose (args.fds[0][0]);
+  xclose (args.fds[1][1]);
+
+  {
+    fd_set rfds;
+    FD_ZERO (&rfds);
+    FD_SET (args.fds[1][0], &rfds);
+
+    int r = select (args.fds[1][0] + 1, &rfds, NULL, NULL, &args.tmo);
+    TEST_COMPARE (r, 1);
+  }
 
   return 0;
 }
-- 
2.27.0

Re: [PATCH] misc: Fix tst-select timeout handling (BZ#27648)

[H.J. Lu]

On Thu, Mar 25, 2021 at 1:00 PM Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
>
> Instead of polling the stderr, create two pipes and fork to check
> if child timeout as expected similar to tst-pselect.c.  Also lower
> the timeout value.
>
> Checked on x86_64-linux-gnu.
> ---
>  misc/tst-select.c | 85 ++++++++++++++++++++++++++++++-----------------
>  1 file changed, 55 insertions(+), 30 deletions(-)
>
> diff --git a/misc/tst-select.c b/misc/tst-select.c
> index 7c310256c5..530181c264 100644
> --- a/misc/tst-select.c
> +++ b/misc/tst-select.c
> @@ -16,54 +16,79 @@
>     License along with the GNU C Library; if not, see
>     <https://www.gnu.org/licenses/>.  */
>
> -#include <time.h>
>  #include <errno.h>
> -#include <stdbool.h>
> -#include <sys/select.h>
> +#include <support/capture_subprocess.h>
>  #include <support/check.h>
> -#include <support/xtime.h>
>  #include <support/timespec.h>
> +#include <support/xunistd.h>
> +#include <support/xtime.h>
>
> -#define TST_SELECT_TIMEOUT 1
> -#define TST_SELECT_FD_ERR 2
> +struct child_args
> +{
> +  int fds[2][2];
> +  struct timeval tmo;
> +};
>
> -static int
> -test_select_timeout (bool zero_tmo)
> +static void
> +do_test_child (void *clousure)
>  {
> -  const int fds = TST_SELECT_FD_ERR;
> -  int timeout = TST_SELECT_TIMEOUT;
> -  struct timeval to = { 0, 0 };
> -  struct timespec ts;
> -  fd_set rfds;
> +  struct child_args *args = (struct child_args *) clousure;
>
> -  FD_ZERO (&rfds);
> -  FD_SET (fds, &rfds);
> +  close (args->fds[0][1]);
> +  close (args->fds[1][0]);
>
> -  if (zero_tmo)
> -    timeout = 0;
> +  fd_set rfds;
> +  FD_ZERO (&rfds);
> +  FD_SET (args->fds[0][0], &rfds);
>
> -  to.tv_sec = timeout;
> -  ts = xclock_now (CLOCK_REALTIME);
> -  ts = timespec_add (ts, (struct timespec) { timeout, 0 });
> +  struct timespec ts = xclock_now (CLOCK_REALTIME);
> +  ts = timespec_add (ts, (struct timespec) { args->tmo.tv_sec, 0 });
>
> -  /* Wait for timeout.  */
> -  int ret = select (fds + 1, &rfds, NULL, NULL, &to);
> -  if (ret == -1)
> -    FAIL_EXIT1 ("select failed: %m\n");
> +  int r = select (args->fds[0][0] + 1, &rfds, NULL, NULL, &args->tmo);
> +  TEST_COMPARE (r, 0);
>
>    TEST_TIMESPEC_NOW_OR_AFTER (CLOCK_REALTIME, ts);
>
> -  return 0;
> +  xwrite (args->fds[1][1], "foo", 3);
>  }
>
>  static int
>  do_test (void)
>  {
> -  /* Check if select exits immediately.  */
> -  test_select_timeout (true);
> -
> -  /* Check if select exits after specified timeout.  */
> -  test_select_timeout (false);
> +  struct child_args args;
> +
> +  xpipe (args.fds[0]);
> +  xpipe (args.fds[1]);
> +
> +  /* The child select should timeout and write on its pipe end.  */
> +  args.tmo = (struct timeval) { .tv_sec = 0, .tv_usec = 250000 };
> +  {
> +    struct support_capture_subprocess result;
> +    result = support_capture_subprocess (do_test_child, &args);
> +    support_capture_subprocess_check (&result, "tst-select-child", 0,
> +                                     sc_allow_none);
> +  }
> +
> +  /* Same with simulating a polling.  */
> +  args.tmo = (struct timeval) { .tv_sec = 0, .tv_usec = 0 };
> +  {
> +    struct support_capture_subprocess result;
> +    result = support_capture_subprocess (do_test_child, &args);
> +    support_capture_subprocess_check (&result, "tst-select-child", 0,
> +                                     sc_allow_none);
> +  }
> +
> +  xclose (args.fds[0][0]);
> +  xclose (args.fds[1][1]);
> +
> +  {
> +    fd_set rfds;
> +    FD_ZERO (&rfds);
> +    FD_SET (args.fds[1][0], &rfds);
> +
> +    int r = select (args.fds[1][0] + 1, &rfds, NULL, NULL, &args.tmo);
> +    TEST_COMPARE (r, 1);
> +  }
>
>    return 0;
>  }
> --
> 2.27.0
>

LGTM.

Thanks.

-- 
H.J.

Re: [PATCH] misc: Fix tst-select timeout handling (BZ#27648)

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 529 bytes --]

Hi Adhemerval,

> Instead of polling the stderr, create two pipes and fork to check
> if child timeout as expected similar to tst-pselect.c.  Also lower
> the timeout value.

I'm just wondering why this approach (with pipes) is more safe than
polling stderr?


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] misc: Fix tst-select timeout handling (BZ#27648)

[Adhemerval Zanella]

On 26/03/2021 06:43, Lukasz Majewski wrote:
> Hi Adhemerval,
> 
>> Instead of polling the stderr, create two pipes and fork to check
>> if child timeout as expected similar to tst-pselect.c.  Also lower
>> the timeout value.
> 
> I'm just wondering why this approach (with pipes) is more safe than
> polling stderr?

Using the standard file descriptor is tricky because they can be
either closed or redirected by the parent.