diff mbox series

[3/3] posix: Remove posix waitid

Message ID 20191120141535.5303-3-adhemerval.zanella@linaro.org
State Accepted
Commit 6437fecca30deb88e5901ab03168cb7b40c7e0a7
Headers show
Series [1/3] support: Add support_process_state_wait | expand

Commit Message

Adhemerval Zanella Netto Nov. 20, 2019, 2:15 p.m. UTC 
The POSIX waitid implementation is problematic in some ways:

  - It emulates using waitpid, which default implementation calls
    wait4 and wait4 returns ENOSYS as default.

  - Also by using waitpid it does not allod support the WNOWAIT,
    WEXITED, WSTOPPED, or WCONTINUED flag.  With current POSIX
    specification the flags are no longer marked as optional.

Also due BZ#23091 Hurd still uses the implementation, so it is moved
to as a Hurd arch-specific folder (with some minor cleanups).

Checked against a i686-gnu (run-built-tests=no)
---
 sysdeps/{posix => mach/hurd}/waitid.c | 70 ++++-----------------------
 1 file changed, 9 insertions(+), 61 deletions(-)
 rename sysdeps/{posix => mach/hurd}/waitid.c (65%)

-- 
2.17.1

Comments

Christian Brauner Nov. 20, 2019, 3:14 p.m. UTC | #1 
On Wed, Nov 20, 2019 at 11:15:35AM -0300, Adhemerval Zanella wrote:
> The POSIX waitid implementation is problematic in some ways:

> 

>   - It emulates using waitpid, which default implementation calls

>     wait4 and wait4 returns ENOSYS as default.

> 

>   - Also by using waitpid it does not allod support the WNOWAIT,

>     WEXITED, WSTOPPED, or WCONTINUED flag.  With current POSIX

>     specification the flags are no longer marked as optional.

> 

> Also due BZ#23091 Hurd still uses the implementation, so it is moved

> to as a Hurd arch-specific folder (with some minor cleanups).

> 

> Checked against a i686-gnu (run-built-tests=no)


I haven't followed that thread too closely, so sorry for maybe redundant
questions.
Does this mean you're removing support for the waitid() _syscall_ from
glibc? How do you intend to deal with P_PIDFD released with kernel 5.3?

Christian
Adhemerval Zanella Netto Nov. 20, 2019, 5:14 p.m. UTC | #2 
On 20/11/2019 12:14, Christian Brauner wrote:
> On Wed, Nov 20, 2019 at 11:15:35AM -0300, Adhemerval Zanella wrote:

>> The POSIX waitid implementation is problematic in some ways:

>>

>>   - It emulates using waitpid, which default implementation calls

>>     wait4 and wait4 returns ENOSYS as default.

>>

>>   - Also by using waitpid it does not allod support the WNOWAIT,

>>     WEXITED, WSTOPPED, or WCONTINUED flag.  With current POSIX

>>     specification the flags are no longer marked as optional.

>>

>> Also due BZ#23091 Hurd still uses the implementation, so it is moved

>> to as a Hurd arch-specific folder (with some minor cleanups).

>>

>> Checked against a i686-gnu (run-built-tests=no)

> 

> I haven't followed that thread too closely, so sorry for maybe redundant

> questions.

> Does this mean you're removing support for the waitid() _syscall_ from

> glibc? How do you intend to deal with P_PIDFD released with kernel 5.3?

> 

> Christian

> 


No, this patch moves the POSIX emulation at sysdeps/posix/waitid.c based
on waitpid to a Hurd specific implementation.  Linux will still be done
by the generic syscalls.list, meaning it will a direct syscall.
Adhemerval Zanella Netto Dec. 20, 2019, 12:36 p.m. UTC | #3 
Ping.

On 20/11/2019 11:15, Adhemerval Zanella wrote:
> The POSIX waitid implementation is problematic in some ways:

> 

>   - It emulates using waitpid, which default implementation calls

>     wait4 and wait4 returns ENOSYS as default.

> 

>   - Also by using waitpid it does not allod support the WNOWAIT,

>     WEXITED, WSTOPPED, or WCONTINUED flag.  With current POSIX

>     specification the flags are no longer marked as optional.

> 

> Also due BZ#23091 Hurd still uses the implementation, so it is moved

> to as a Hurd arch-specific folder (with some minor cleanups).

> 

> Checked against a i686-gnu (run-built-tests=no)

> ---

>  sysdeps/{posix => mach/hurd}/waitid.c | 70 ++++-----------------------

>  1 file changed, 9 insertions(+), 61 deletions(-)

>  rename sysdeps/{posix => mach/hurd}/waitid.c (65%)

> 

> diff --git a/sysdeps/posix/waitid.c b/sysdeps/mach/hurd/waitid.c

> similarity index 65%

> rename from sysdeps/posix/waitid.c

> rename to sysdeps/mach/hurd/waitid.c

> index 0942623553..d28d0034da 100644

> --- a/sysdeps/posix/waitid.c

> +++ b/sysdeps/mach/hurd/waitid.c

> @@ -17,25 +17,11 @@

>     License along with the GNU C Library; if not, see

>     <https://www.gnu.org/licenses/>.  */

>  

> -#include <assert.h>

>  #include <errno.h>

> -#include <signal.h>

> -#define __need_NULL

> -#include <stddef.h>

>  #include <sys/wait.h>

> -#include <sys/types.h>

> -#include <sysdep-cancel.h>

>  

> -

> -#ifdef DO_WAITID

> -# define OUR_WAITID DO_WAITID

> -#elif !defined NO_DO_WAITID

> -# define OUR_WAITID do_waitid

> -#endif

> -

> -#ifdef OUR_WAITID

> -static int

> -OUR_WAITID (idtype_t idtype, id_t id, siginfo_t *infop, int options)

> +int

> +__waitid (idtype_t idtype, id_t id, siginfo_t *infop, int options)

>  {

>    pid_t pid, child;

>    int status;

> @@ -43,7 +29,7 @@ OUR_WAITID (idtype_t idtype, id_t id, siginfo_t *infop, int options)

>    switch (idtype)

>      {

>      case P_PID:

> -      if(id <= 0)

> +      if (id <= 0)

>  	goto invalid;

>        pid = (pid_t) id;

>        break;

> @@ -72,31 +58,10 @@ OUR_WAITID (idtype_t idtype, id_t id, siginfo_t *infop, int options)

>        return -1;

>      }

>  

> -  /* This emulation using waitpid cannot support the waitid modes in which

> -     we do not reap the child, or match only stopped and not dead children.  */

> -  if (0

> -#ifdef WNOWAIT

> -      || (options & WNOWAIT)

> -#endif

> -#ifdef WEXITED

> -      || ((options & (WEXITED|WSTOPPED|WCONTINUED))

> -	  != (WEXITED | (options & WSTOPPED)))

> -#endif

> -      )

> -    {

> -      __set_errno (ENOTSUP);

> -      return -1;

> -    }

> -

>    /* Note the waitid() is a cancellation point.  But since we call

>       waitpid() which itself is a cancellation point we do not have

>       to do anything here.  */

> -  child = __waitpid (pid, &status,

> -		     options

> -#ifdef WEXITED

> -		     &~ WEXITED

> -#endif

> -		     );

> +  child = __waitpid (pid, &status, options);

>  

>    if (child == -1)

>      /* `waitpid' set `errno' for us.  */

> @@ -104,9 +69,11 @@ OUR_WAITID (idtype_t idtype, id_t id, siginfo_t *infop, int options)

>  

>    if (child == 0)

>      {

> -      /* The WHOHANG bit in OPTIONS is set and there are children available

> -	 but none has a status for us.  The XPG docs do not mention this

> -	 case so we clear the `siginfo_t' struct and return successfully.  */

> +      /* POSIX.1-2008, Technical Corrigendum 1 XSH/TC1-2008/0713 [153] states

> +	 that if waitid returns because WNOHANG was specified and status is

> +	 not available for any process specified by idtype and id, then the

> +	 si_signo and si_pid members of the structure pointed to by infop

> +	 shall be set to zero.  */

>        infop->si_signo = 0;

>        infop->si_code = 0;

>        return 0;

> @@ -132,27 +99,8 @@ OUR_WAITID (idtype_t idtype, id_t id, siginfo_t *infop, int options)

>        infop->si_code = CLD_STOPPED;

>        infop->si_status = WSTOPSIG (status);

>      }

> -#ifdef WIFCONTINUED

> -  else if (WIFCONTINUED (status))

> -    {

> -      infop->si_code = CLD_CONTINUED;

> -      infop->si_status = SIGCONT;

> -    }

> -#endif

> -  else

> -    /* Can't happen. */

> -    assert (! "What?");

>  

>    return 0;

>  }

> -#endif

> -

> -

> -int

> -__waitid (idtype_t idtype, id_t id, siginfo_t *infop, int options)

> -{

> -  /* __waitpid should be a cancellation point.  */

> -  return do_waitid (idtype, id, infop, options);

> -}

>  weak_alias (__waitid, waitid)

>  strong_alias (__waitid, __libc_waitid)

>
diff mbox series

Patch

diff --git a/sysdeps/posix/waitid.c b/sysdeps/mach/hurd/waitid.c
similarity index 65%
rename from sysdeps/posix/waitid.c
rename to sysdeps/mach/hurd/waitid.c
index 0942623553..d28d0034da 100644
--- a/sysdeps/posix/waitid.c
+++ b/sysdeps/mach/hurd/waitid.c
@@ -17,25 +17,11 @@ 
    License along with the GNU C Library; if not, see
    <https://www.gnu.org/licenses/>.  */
 
-#include <assert.h>
 #include <errno.h>
-#include <signal.h>
-#define __need_NULL
-#include <stddef.h>
 #include <sys/wait.h>
-#include <sys/types.h>
-#include <sysdep-cancel.h>
 
-
-#ifdef DO_WAITID
-# define OUR_WAITID DO_WAITID
-#elif !defined NO_DO_WAITID
-# define OUR_WAITID do_waitid
-#endif
-
-#ifdef OUR_WAITID
-static int
-OUR_WAITID (idtype_t idtype, id_t id, siginfo_t *infop, int options)
+int
+__waitid (idtype_t idtype, id_t id, siginfo_t *infop, int options)
 {
   pid_t pid, child;
   int status;
@@ -43,7 +29,7 @@  OUR_WAITID (idtype_t idtype, id_t id, siginfo_t *infop, int options)
   switch (idtype)
     {
     case P_PID:
-      if(id <= 0)
+      if (id <= 0)
 	goto invalid;
       pid = (pid_t) id;
       break;
@@ -72,31 +58,10 @@  OUR_WAITID (idtype_t idtype, id_t id, siginfo_t *infop, int options)
       return -1;
     }
 
-  /* This emulation using waitpid cannot support the waitid modes in which
-     we do not reap the child, or match only stopped and not dead children.  */
-  if (0
-#ifdef WNOWAIT
-      || (options & WNOWAIT)
-#endif
-#ifdef WEXITED
-      || ((options & (WEXITED|WSTOPPED|WCONTINUED))
-	  != (WEXITED | (options & WSTOPPED)))
-#endif
-      )
-    {
-      __set_errno (ENOTSUP);
-      return -1;
-    }
-
   /* Note the waitid() is a cancellation point.  But since we call
      waitpid() which itself is a cancellation point we do not have
      to do anything here.  */
-  child = __waitpid (pid, &status,
-		     options
-#ifdef WEXITED
-		     &~ WEXITED
-#endif
-		     );
+  child = __waitpid (pid, &status, options);
 
   if (child == -1)
     /* `waitpid' set `errno' for us.  */
@@ -104,9 +69,11 @@  OUR_WAITID (idtype_t idtype, id_t id, siginfo_t *infop, int options)
 
   if (child == 0)
     {
-      /* The WHOHANG bit in OPTIONS is set and there are children available
-	 but none has a status for us.  The XPG docs do not mention this
-	 case so we clear the `siginfo_t' struct and return successfully.  */
+      /* POSIX.1-2008, Technical Corrigendum 1 XSH/TC1-2008/0713 [153] states
+	 that if waitid returns because WNOHANG was specified and status is
+	 not available for any process specified by idtype and id, then the
+	 si_signo and si_pid members of the structure pointed to by infop
+	 shall be set to zero.  */
       infop->si_signo = 0;
       infop->si_code = 0;
       return 0;
@@ -132,27 +99,8 @@  OUR_WAITID (idtype_t idtype, id_t id, siginfo_t *infop, int options)
       infop->si_code = CLD_STOPPED;
       infop->si_status = WSTOPSIG (status);
     }
-#ifdef WIFCONTINUED
-  else if (WIFCONTINUED (status))
-    {
-      infop->si_code = CLD_CONTINUED;
-      infop->si_status = SIGCONT;
-    }
-#endif
-  else
-    /* Can't happen. */
-    assert (! "What?");
 
   return 0;
 }
-#endif
-
-
-int
-__waitid (idtype_t idtype, id_t id, siginfo_t *infop, int options)
-{
-  /* __waitpid should be a cancellation point.  */
-  return do_waitid (idtype, id, infop, options);
-}
 weak_alias (__waitid, waitid)
 strong_alias (__waitid, __libc_waitid)