Re: [PATCH v5 5/5] Add tests for __clone_internal

["H.J. Lu" <hjl.tools@gmail.com>]

On Thu, May 20, 2021 at 8:08 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu:
>
> > diff --git a/sysdeps/unix/sysv/linux/tst-align-clone-internal.c b/sysdeps/unix/sysv/linux/tst-align-clone-internal.c
> > new file mode 100644
> > index 0000000000..eccc39e255
> > --- /dev/null
> > +++ b/sysdeps/unix/sysv/linux/tst-align-clone-internal.c
>
> > +  int e;
> > +  if (waitpid (p, &e, __WCLONE) != p)
> > +    {
> > +      puts ("waitpid failed");
> > +      kill (p, SIGKILL);
> > +      return 1;
> > +    }
>
> This could use xwaitpid.  The same comment applies to other tests.

Fixed.

> > diff --git a/sysdeps/unix/sysv/linux/tst-clone-internal.c b/sysdeps/unix/sysv/linux/tst-clone-internal.c
> > new file mode 100644
> > index 0000000000..587d519bf2
> > --- /dev/null
> > +++ b/sysdeps/unix/sysv/linux/tst-clone-internal.c
>
> > +static int
> > +do_test (void)
> > +{
> > +  int result;
> > +
> > +  result = __clone_internal (NULL, child_fn, NULL);
> > +
> > +  if (errno != EINVAL || result != -1)
> > +    {
> > +      printf ("FAIL: clone()=%d (wanted -1) errno=%d (wanted %d)\n",
> > +              result, errno, EINVAL);
> > +      return 1;
> > +    }
> > +
> > +  puts ("All OK");
> > +  return 0;
> > +}
>
> I think this test is invalid for the internal function (the comment
> about not checking arguments).

Removed.

> > diff --git a/sysdeps/unix/sysv/linux/tst-clone2-internal.c b/sysdeps/unix/sysv/linux/tst-clone2-internal.c
> > new file mode 100644
> > index 0000000000..dd8f32c24b
> > --- /dev/null
> > +++ b/sysdeps/unix/sysv/linux/tst-clone2-internal.c
> > @@ -0,0 +1,142 @@
>
> > +
> > +static int
> > +f (void *a)
> > +{
> > +  close (pipefd[0]);
> > +
> > +  pid_t ppid = getppid ();
> > +  pid_t pid = getpid ();
> > +  pid_t tid = syscall (__NR_gettid);
> > +
> > +  if (write (pipefd[1], &ppid, sizeof ppid) != sizeof (ppid))
> > +    FAIL_EXIT1 ("write ppid failed\n");
> > +  if (write (pipefd[1], &pid, sizeof pid) != sizeof (pid))
> > +    FAIL_EXIT1 ("write pid failed\n");
> > +  if (write (pipefd[1], &tid, sizeof tid) != sizeof (tid))
> > +    FAIL_EXIT1 ("write tid failed\n");
> > +
> > +  return 0;
> > +}
>
> You could use support_shared_allocate for the parent/child communication
> instead of a pipe.  The MAP_SHARED mapping overrides the lack of
> CLONE_VM.
>
> > +static int
> > +do_test (void)
> > +{
> > +  sig = SIGRTMIN;
> > +  sigset_t ss;
> > +  sigemptyset (&ss);
> > +  sigaddset (&ss, sig);
> > +  if (sigprocmask (SIG_BLOCK, &ss, NULL) != 0)
> > +    FAIL_EXIT1 ("sigprocmask failed: %m");
>
> You could use xpthread_sigmask.  (Applies to tst-getpid1-internal.c as
> well.)

Since this test is copied from the previous test and doesn't use
thread, I'd like to keep it this way.

>
> > +  pid_t own_pid = getpid ();
> > +  pid_t own_tid = syscall (__NR_gettid);
>
> We have gettid nowadays.

Fixed.

> > +#include <support/test-driver.c>
> > diff --git a/sysdeps/unix/sysv/linux/tst-clone3-internal.c b/sysdeps/unix/sysv/linux/tst-clone3-internal.c
> > new file mode 100644
> > index 0000000000..61863e1504
> > --- /dev/null
> > +++ b/sysdeps/unix/sysv/linux/tst-clone3-internal.c
> > +#include <stackinfo.h>  /* For _STACK_GROWS_{UP,DOWN}.  */
>
> No longer needed! 8-)

Fixed.

> > +#include <support/check.h>
> > +#include <stdatomic.h>
> > +#include <clone_internal.h>
> > +
> > +/* Test if clone call with CLONE_THREAD does not call exit_group.  The 'f'
> > +   function returns '1', which will be used by clone thread to call the
> > +   'exit' syscall directly.  If _exit is used instead, exit_group will be
> > +   used and thus the thread group will finish with return value of '1'
> > +   (where '2' from main thread is expected.  */
>
> Missing ).

Fixed.

> The rest looks okay as far as I can tell.
>
> Thanks,
> Florian
>

Thanks.

-- 
H.J.