[v3] tst: Provide test for ppoll

Message ID 20210205112751.13559-1-lukma@denx.de
State Superseded
Headers
Series [v3] tst: Provide test for ppoll |

Commit Message

Lukasz Majewski Feb. 5, 2021, 11:27 a.m. UTC
  This change adds new test to assess ppoll()'s timeout related
functionality (the struct pollfd does not provide valid fd to wait
for - just wait for timeout).

To be more specific - two use cases are checked:
- if ppoll() times out immediately when passed struct timespec has zero
values of tv_nsec and tv_sec.
- if ppoll() times out after timeout specified in passed argument

---
Changes for v2:
- Remove _GNU_SOURCE definition if not already defined
- Replace clock_gettime with xclock_gettime
- Use FAIL_EXIT1 instead of plain ret value returning from the test

Changes for v3:
- Use smaller timeout - 0.5s (TIMEOUT* defines can be removed)
- Use TEST_TIMESPEC_NOW_OR_AFTER () to check if test has properly timed
  out
---
 sysdeps/unix/sysv/linux/Makefile    |  2 +-
 sysdeps/unix/sysv/linux/tst-ppoll.c | 53 +++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+), 1 deletion(-)
 create mode 100644 sysdeps/unix/sysv/linux/tst-ppoll.c
  

Comments

Siddhesh Poyarekar Feb. 8, 2021, 3:13 p.m. UTC | #1
On 2/5/21 4:57 PM, Lukasz Majewski wrote:
> This change adds new test to assess ppoll()'s timeout related
> functionality (the struct pollfd does not provide valid fd to wait
> for - just wait for timeout).
> 
> To be more specific - two use cases are checked:
> - if ppoll() times out immediately when passed struct timespec has zero
> values of tv_nsec and tv_sec.
> - if ppoll() times out after timeout specified in passed argument
> 
> ---
> Changes for v2:
> - Remove _GNU_SOURCE definition if not already defined
> - Replace clock_gettime with xclock_gettime
> - Use FAIL_EXIT1 instead of plain ret value returning from the test
> 
> Changes for v3:
> - Use smaller timeout - 0.5s (TIMEOUT* defines can be removed)
> - Use TEST_TIMESPEC_NOW_OR_AFTER () to check if test has properly timed
>    out

Lukasz, looks like this got pushed to master without a review.  Perhaps 
it happened by accident when pushing the timerfd patch?

Siddhesh
  
Lukasz Majewski Feb. 8, 2021, 4:06 p.m. UTC | #2
Hi Siddhesh,

> On 2/5/21 4:57 PM, Lukasz Majewski wrote:
> > This change adds new test to assess ppoll()'s timeout related
> > functionality (the struct pollfd does not provide valid fd to wait
> > for - just wait for timeout).
> > 
> > To be more specific - two use cases are checked:
> > - if ppoll() times out immediately when passed struct timespec has
> > zero values of tv_nsec and tv_sec.
> > - if ppoll() times out after timeout specified in passed argument
> > 
> > ---
> > Changes for v2:
> > - Remove _GNU_SOURCE definition if not already defined
> > - Replace clock_gettime with xclock_gettime
> > - Use FAIL_EXIT1 instead of plain ret value returning from the test
> > 
> > Changes for v3:
> > - Use smaller timeout - 0.5s (TIMEOUT* defines can be removed)
> > - Use TEST_TIMESPEC_NOW_OR_AFTER () to check if test has properly
> > timed out  
> 
> Lukasz, looks like this got pushed to master without a review.

With this particular version (v3) - I've followed detailed hints
provided by Adhemerval:
https://patchwork.sourceware.org/project/glibc/patch/20210114163239.16505-1-lukma@denx.de/

> Perhaps it happened by accident when pushing the timerfd patch?

I though that this patch is OK, after adding changes proposed by
Adhemerval (Adhemerval in detailed way - with working code - wrote what
he want's to see).

(Maybe I shouldn't post v3 to ML?)

> 
> Siddhesh


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
  
Siddhesh Poyarekar Feb. 8, 2021, 4:18 p.m. UTC | #3
On 2/8/21 9:36 PM, Lukasz Majewski wrote:
> Hi Siddhesh,
> 
>> On 2/5/21 4:57 PM, Lukasz Majewski wrote:
>>> This change adds new test to assess ppoll()'s timeout related
>>> functionality (the struct pollfd does not provide valid fd to wait
>>> for - just wait for timeout).
>>>
>>> To be more specific - two use cases are checked:
>>> - if ppoll() times out immediately when passed struct timespec has
>>> zero values of tv_nsec and tv_sec.
>>> - if ppoll() times out after timeout specified in passed argument
>>>
>>> ---
>>> Changes for v2:
>>> - Remove _GNU_SOURCE definition if not already defined
>>> - Replace clock_gettime with xclock_gettime
>>> - Use FAIL_EXIT1 instead of plain ret value returning from the test
>>>
>>> Changes for v3:
>>> - Use smaller timeout - 0.5s (TIMEOUT* defines can be removed)
>>> - Use TEST_TIMESPEC_NOW_OR_AFTER () to check if test has properly
>>> timed out
>>
>> Lukasz, looks like this got pushed to master without a review.
> 
> With this particular version (v3) - I've followed detailed hints
> provided by Adhemerval:
> https://patchwork.sourceware.org/project/glibc/patch/20210114163239.16505-1-lukma@denx.de/
> 
>> Perhaps it happened by accident when pushing the timerfd patch?
> 
> I though that this patch is OK, after adding changes proposed by
> Adhemerval (Adhemerval in detailed way - with working code - wrote what
> he want's to see).
> 
> (Maybe I shouldn't post v3 to ML?)

Typically we commit without additional review if changes from one 
version to another are trivial *and* the reviewer explicitly states 
that.  Given that the reviewer didn't explicitly say "OK to commit with 
nits fixed" (or similar), it's generally a good idea to wait for a 
review in such cases.  Also, given that Adhemerval gave working code, 
the commit should also contain a Co-authored-by tag.

Adhemerval, could you please confirm if this v3 is what you're looking 
for?  Given that it's just a test case, I reckon a revert is not 
necessary and Lukasz can just fix things up if you need him to add any 
more fixes.  Please note though that the patch that went into master is 
slightly different from the v3.  I suspect it's just additional includes.

Siddhesh
  
Lukasz Majewski Feb. 8, 2021, 4:38 p.m. UTC | #4
Hi Siddhesh,

> On 2/8/21 9:36 PM, Lukasz Majewski wrote:
> > Hi Siddhesh,
> >   
> >> On 2/5/21 4:57 PM, Lukasz Majewski wrote:  
> >>> This change adds new test to assess ppoll()'s timeout related
> >>> functionality (the struct pollfd does not provide valid fd to wait
> >>> for - just wait for timeout).
> >>>
> >>> To be more specific - two use cases are checked:
> >>> - if ppoll() times out immediately when passed struct timespec has
> >>> zero values of tv_nsec and tv_sec.
> >>> - if ppoll() times out after timeout specified in passed argument
> >>>
> >>> ---
> >>> Changes for v2:
> >>> - Remove _GNU_SOURCE definition if not already defined
> >>> - Replace clock_gettime with xclock_gettime
> >>> - Use FAIL_EXIT1 instead of plain ret value returning from the
> >>> test
> >>>
> >>> Changes for v3:
> >>> - Use smaller timeout - 0.5s (TIMEOUT* defines can be removed)
> >>> - Use TEST_TIMESPEC_NOW_OR_AFTER () to check if test has properly
> >>> timed out  
> >>
> >> Lukasz, looks like this got pushed to master without a review.  
> > 
> > With this particular version (v3) - I've followed detailed hints
> > provided by Adhemerval:
> > https://patchwork.sourceware.org/project/glibc/patch/20210114163239.16505-1-lukma@denx.de/
> >   
> >> Perhaps it happened by accident when pushing the timerfd patch?  
> > 
> > I though that this patch is OK, after adding changes proposed by
> > Adhemerval (Adhemerval in detailed way - with working code - wrote
> > what he want's to see).
> > 
> > (Maybe I shouldn't post v3 to ML?)  
> 
> Typically we commit without additional review if changes from one 
> version to another are trivial *and* the reviewer explicitly states 
> that.  Given that the reviewer didn't explicitly say "OK to commit
> with nits fixed" (or similar), it's generally a good idea to wait for
> a review in such cases.  Also, given that Adhemerval gave working
> code, the commit should also contain a Co-authored-by tag.
> 

Thanks for the explanation. I should have wait for the explicit OK from
Adhemerval.

> Adhemerval, could you please confirm if this v3 is what you're
> looking for?  Given that it's just a test case, I reckon a revert is
> not necessary and Lukasz can just fix things up if you need him to
> add any more fixes. 

I will fix them if needed.

> Please note though that the patch that went into
> master is slightly different from the v3.  I suspect it's just
> additional includes.
> 
> Siddhesh




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
  

Patch

diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile
index 7503b356c8..f4029a74ca 100644
--- a/sysdeps/unix/sysv/linux/Makefile
+++ b/sysdeps/unix/sysv/linux/Makefile
@@ -108,7 +108,7 @@  tests += tst-clone tst-clone2 tst-clone3 tst-fanotify tst-personality \
 	 test-errno-linux tst-memfd_create tst-mlock2 tst-pkey \
 	 tst-rlimit-infinity tst-ofdlocks tst-gettid tst-gettid-kill \
 	 tst-tgkill tst-sysvsem-linux tst-sysvmsg-linux tst-sysvshm-linux \
-	 tst-timerfd
+	 tst-timerfd tst-ppoll
 tests-internal += tst-ofdlocks-compat tst-sigcontext-get_pc
 
 CFLAGS-tst-sigcontext-get_pc.c = -fasynchronous-unwind-tables
diff --git a/sysdeps/unix/sysv/linux/tst-ppoll.c b/sysdeps/unix/sysv/linux/tst-ppoll.c
new file mode 100644
index 0000000000..29d92485e8
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/tst-ppoll.c
@@ -0,0 +1,53 @@ 
+/* Test for ppoll timeout
+   Copyright (C) 2021 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 <time.h>
+#include <poll.h>
+#include <support/check.h>
+#include <support/xtime.h>
+
+static int test_ppoll_timeout (bool zero_tmp)
+{
+  /* We wait for half a second.  */
+  struct timespec ts;
+  xclock_gettime (CLOCK_REALTIME, &ts);
+  struct timespec timeout = make_timespec (0, zero_tmo ? 0 : TIMESPEC_HZ/2);
+  ts = timespec_add (ts, timeout);
+
+  /* Ignore fds - just wait for timeout.  */
+  struct pollfd fds = { -1, 0, 0 };
+  TEST_COMPARE (ppoll (&fds, 1, &timeout, 0), 0);
+
+  TEST_TIMESPEC_NOW_OR_AFTER (CLOCK_REALTIME, ts);
+
+  return 0;
+}
+
+static int
+do_test (void)
+{
+  /* Check if ppoll exits immediately.  */
+  test_ppoll_timeout (true);
+
+  /* Check if ppoll exits after specified timeout.  */
+  test_ppoll_timeout (false);
+
+  return 0;
+}
+
+#include <support/test-driver.c>