Re: [PATCH v2 3/3] posix: Remove posix waitid

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 18/02/2020 11:02, 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)
> ---
>  posix/tst-waitid.c                    | 14 +++---

Ok, the changes on tst-waitid.c shouldn't be in this patch, but rather
on the second one.

>  sysdeps/{posix => mach/hurd}/waitid.c | 70 ++++-----------------------
>  2 files changed, 16 insertions(+), 68 deletions(-)
>  rename sysdeps/{posix => mach/hurd}/waitid.c (65%)
> 
> diff --git a/posix/tst-waitid.c b/posix/tst-waitid.c
> index 2331c42e66..380d807498 100644
> --- a/posix/tst-waitid.c
> +++ b/posix/tst-waitid.c
> @@ -80,10 +80,10 @@ do_test_waitd_common (idtype_t type, pid_t pid)
>  {
>    /* Adding process_state_tracing_stop ('t') allows the test to work under
>       trace programs such as ptrace.  */
> -  enum process_state_t stop_state = process_state_stopped
> -				    | process_state_tracing_stop;
> +  enum support_process_state stop_state
> +    = support_process_state_stopped | support_process_state_tracing_stop;
>  
> -  support_process_state_wait (pid, stop_state);
> +  support_process_state_wait (pid, stop_state, NULL);
>  
>    check_sigchld (CLD_STOPPED, SIGSTOP, pid);
>  
> @@ -117,7 +117,7 @@ do_test_waitd_common (idtype_t type, pid_t pid)
>      FAIL_RET ("kill (%d, SIGCONT): %m\n", pid);
>  
>    /* Wait for the child to have continued.  */
> -  support_process_state_wait (pid, process_state_sleeping);
> +  support_process_state_wait (pid, support_process_state_sleeping, NULL);
>  
>  #if WCONTINUED != 0
>    {
> @@ -175,7 +175,7 @@ do_test_waitd_common (idtype_t type, pid_t pid)
>       stopped, but if we are real quick and enter the waitid system call
>       before the SIGCHLD has been generated, then it will be discarded and
>       never delivered.  */
> -  support_process_state_wait (pid, stop_state);
> +  support_process_state_wait (pid, stop_state, NULL);
>  
>    fail = waitid (type, pid, &info, WEXITED|WSTOPPED);
>    TEST_COMPARE (fail, 0);
> @@ -190,7 +190,7 @@ do_test_waitd_common (idtype_t type, pid_t pid)
>      FAIL_RET ("kill (%d, SIGCONT): %m\n", pid);
>  
>    /* Wait for the child to have continued.  */
> -  support_process_state_wait (pid, process_state_sleeping);
> +  support_process_state_wait (pid, support_process_state_sleeping, NULL);
>  
>    check_sigchld (CLD_CONTINUED, SIGCONT, pid);
>  
> @@ -219,7 +219,7 @@ do_test_waitd_common (idtype_t type, pid_t pid)
>    TEST_COMPARE (info.si_status, SIGKILL);
>    TEST_COMPARE (info.si_pid, pid);
>  #else
> -  support_process_state_wait (pid, process_state_zombie);
> +  support_process_state_wait (pid, support_process_state_zombie, NULL);
>  #endif
>    check_sigchld (CLD_KILLED, SIGKILL, pid);
>  
> 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 f752076735..dce72339dd 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)
>