From: Florian Weimer <fweimer@redhat.com>
To: "H.J. Lu" <hjl.tools@gmail.com>
Cc: libc-alpha@sourceware.org,
Adhemerval Zanella <adhemerval.zanella@linaro.org>
Subject: Re: [PATCH v5 5/5] Add tests for __clone_internal
Date: Thu, 20 May 2021 17:08:00 +0200 [thread overview]
Message-ID: <87tumxmoj3.fsf@oldenburg.str.redhat.com> (raw)
In-Reply-To: <20210515123442.1432385-6-hjl.tools@gmail.com> (H. J. Lu's message of "Sat, 15 May 2021 05:34:42 -0700")
* 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.
> 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).
> 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.)
> + pid_t own_pid = getpid ();
> + pid_t own_tid = syscall (__NR_gettid);
We have gettid nowadays.
> +#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-)
> +#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 ).
The rest looks okay as far as I can tell.
Thanks,
Florian
next prev parent reply other threads:[~2021-05-20 15:08 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-15 12:34 [PATCH v5 0/5] Add an internal wrapper for clone, clone2 and clone3 H.J. Lu
2021-05-15 12:34 ` [PATCH v5 1/5] " H.J. Lu
2021-05-20 14:46 ` Florian Weimer
2021-05-22 1:14 ` H.J. Lu
2021-05-15 12:34 ` [PATCH v5 2/5] nptl: Always pass stack size to create_thread H.J. Lu
2021-05-20 14:26 ` Florian Weimer
2021-05-15 12:34 ` [PATCH v5 3/5] GLIBC_PRIVATE: Export __clone_internal H.J. Lu
2021-05-17 13:54 ` Andreas Schwab
2021-05-20 14:24 ` Florian Weimer
2021-05-22 1:55 ` H.J. Lu
2021-05-15 12:34 ` [PATCH v5 4/5] x86-64: Add the clone3 wrapper H.J. Lu
2021-05-20 14:53 ` Florian Weimer
2021-05-22 1:38 ` H.J. Lu
2021-05-20 18:35 ` Noah Goldstein
2021-05-20 18:39 ` Noah Goldstein
2021-05-22 1:52 ` H.J. Lu
2021-05-15 12:34 ` [PATCH v5 5/5] Add tests for __clone_internal H.J. Lu
2021-05-20 15:08 ` Florian Weimer [this message]
2021-05-22 1:54 ` H.J. Lu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87tumxmoj3.fsf@oldenburg.str.redhat.com \
--to=fweimer@redhat.com \
--cc=adhemerval.zanella@linaro.org \
--cc=hjl.tools@gmail.com \
--cc=libc-alpha@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).