This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] nptl: Fix tst-cancel4 sendto tests
- From: Adhemerval Zanella <adhemerval dot zanella at linaro dot org>
- To: libc-alpha at sourceware dot org
- Date: Fri, 2 Mar 2018 15:27:51 -0300
- Subject: Re: [PATCH] nptl: Fix tst-cancel4 sendto tests
- Authentication-results: sourceware.org; auth=none
- References: <1519328339-25282-1-git-send-email-adhemerval.zanella@linaro.org>
If no one opposes it, I will commit this shortly.
On 22/02/2018 16:38, Adhemerval Zanella wrote:
> Now that send might be implemented calling sendto syscall on Linux,
> I am seeing some issue in some kernel configurations where tst-cancel4
> sendto do not block as expected.
>
> The socket used to force the syscall blocking is used with default
> system configuration for buffer sending size, which might not be
> correct to force blocking. This patch fixes it by explicit setting
> buffer socket lower than the buffer size used. It also enabled sendto
> cancellation tests to work in both ways (since internally send is
> implemented routing to sendto on Linux kernel).
>
> The patch also removes unrequired make rules on some archictures
> for send/recv. The generic nptl Makefile already set the compiler flags
> required on some architectures for correct unwinding and libc object
> are not strictly required to support unwind (since pthread_cancel
> requires linking against libpthread).
>
> Checked on aarch64-linux-gnu and x86_64-linux-gnu. I also did a
> sniff test with tst-cancel{4,5} on a simulated mips64-linux-gnu.
>
> * nptl/tst-cancel4-common.h (set_socket_buffer): New function.
> * nptl/tst-cancel4-common.c (do_test): Call set_socket_buffer
> for socketpair endpoint.
> * nptl/tst-cancel4.c (tf_send): Call set_socket_buffer and use
> WRITE_BUFFER_SIZE as buffer size for sending socket.
> (tf_sendto): Use SOCK_STREAM instead of SOCK_DGRAM and fix an
> issue on system where send is implemented with sendto syscall.
> * sysdeps/unix/sysv/linux/mips/mips64/Makefile [$(subdir) = socket]
> (CFLAGS-recv.c, CFLAGS-send.c): Remove rules.
> [$(subdir) = nptl] (CFLAGS-recv.c, CFLAGS-send.c): Likewise.
> * sysdeps/unix/sysv/linux/riscv/rv64/Makefile: Remove file.
> ---
> ChangeLog | 14 +++++++++++
> nptl/tst-cancel4-common.c | 18 +-------------
> nptl/tst-cancel4-common.h | 14 +++++++++++
> nptl/tst-cancel4.c | 37 +++++++++++++++-------------
> sysdeps/unix/sysv/linux/mips/mips64/Makefile | 10 --------
> sysdeps/unix/sysv/linux/riscv/rv64/Makefile | 4 ---
> 6 files changed, 49 insertions(+), 48 deletions(-)
> delete mode 100644 sysdeps/unix/sysv/linux/riscv/rv64/Makefile
>
> diff --git a/nptl/tst-cancel4-common.c b/nptl/tst-cancel4-common.c
> index 5bc7e44..c6eee73 100644
> --- a/nptl/tst-cancel4-common.c
> +++ b/nptl/tst-cancel4-common.c
> @@ -20,29 +20,13 @@
> static int
> do_test (void)
> {
> - int val;
> - socklen_t len;
> -
> if (socketpair (AF_UNIX, SOCK_STREAM, PF_UNIX, fds) != 0)
> {
> perror ("socketpair");
> exit (1);
> }
>
> - val = 1;
> - len = sizeof(val);
> - setsockopt (fds[1], SOL_SOCKET, SO_SNDBUF, &val, sizeof(val));
> - if (getsockopt (fds[1], SOL_SOCKET, SO_SNDBUF, &val, &len) < 0)
> - {
> - perror ("getsockopt");
> - exit (1);
> - }
> - if (val >= WRITE_BUFFER_SIZE)
> - {
> - puts ("minimum write buffer size too large");
> - exit (1);
> - }
> - setsockopt (fds[1], SOL_SOCKET, SO_SNDBUF, &val, sizeof(val));
> + set_socket_buffer (fds[1]);
>
> if (mktemp (fifoname) == NULL)
> {
> diff --git a/nptl/tst-cancel4-common.h b/nptl/tst-cancel4-common.h
> index a526c0c..10cc4f3 100644
> --- a/nptl/tst-cancel4-common.h
> +++ b/nptl/tst-cancel4-common.h
> @@ -62,6 +62,20 @@ static pthread_barrier_t b2;
>
> #define WRITE_BUFFER_SIZE 16384
>
> +/* Set the send buffer of socket S to 1 byte so any send operation
> + done with WRITE_BUFFER_SIZE bytes will force syscall blocking. */
> +static void
> +set_socket_buffer (int s)
> +{
> + int val = 1;
> + socklen_t len = sizeof(val);
> +
> + TEST_VERIFY_EXIT (setsockopt (s, SOL_SOCKET, SO_SNDBUF, &val,
> + sizeof(val)) == 0);
> + TEST_VERIFY_EXIT (getsockopt (s, SOL_SOCKET, SO_SNDBUF, &val, &len) == 0);
> + TEST_VERIFY_EXIT (val < WRITE_BUFFER_SIZE);
> +}
> +
> /* Cleanup handling test. */
> static int cl_called;
>
> diff --git a/nptl/tst-cancel4.c b/nptl/tst-cancel4.c
> index 92a3d80..0532538 100644
> --- a/nptl/tst-cancel4.c
> +++ b/nptl/tst-cancel4.c
> @@ -726,6 +726,8 @@ tf_send (void *arg)
> if (tempfd2 == -1)
> FAIL_EXIT1 ("socket (AF_UNIX, SOCK_STREAM, 0): %m");
>
> + set_socket_buffer (tempfd2);
> +
> if (connect (tempfd2, (struct sockaddr *) &sun, sizeof (sun)) != 0)
> FAIL_EXIT1 ("connect: %m");
>
> @@ -738,8 +740,7 @@ tf_send (void *arg)
>
> pthread_cleanup_push (cl, NULL);
>
> - /* Very large block, so that the send call blocks. */
> - char mem[700000];
> + char mem[WRITE_BUFFER_SIZE];
>
> send (tempfd2, mem, arg == NULL ? sizeof (mem) : 1, 0);
>
> @@ -1230,16 +1231,11 @@ tf_msync (void *arg)
> static void *
> tf_sendto (void *arg)
> {
> - if (arg == NULL)
> - // XXX If somebody can provide a portable test case in which sendto()
> - // blocks we can enable this test to run in both rounds.
> - abort ();
> -
> struct sockaddr_un sun;
>
> - tempfd = socket (AF_UNIX, SOCK_DGRAM, 0);
> + tempfd = socket (AF_UNIX, SOCK_STREAM, 0);
> if (tempfd == -1)
> - FAIL_EXIT1 ("socket (AF_UNIX, SOCK_DGRAM, 0): %m");
> + FAIL_EXIT1 ("socket (AF_UNIX, SOCK_STREAM, 0): %m");
>
> int tries = 0;
> do
> @@ -1254,23 +1250,30 @@ tf_sendto (void *arg)
> while (bind (tempfd, (struct sockaddr *) &sun,
> offsetof (struct sockaddr_un, sun_path)
> + strlen (sun.sun_path) + 1) != 0);
> - tempfname = strdup (sun.sun_path);
>
> - tempfd2 = socket (AF_UNIX, SOCK_DGRAM, 0);
> + listen (tempfd, 5);
> +
> + tempfd2 = socket (AF_UNIX, SOCK_STREAM, 0);
> if (tempfd2 == -1)
> - FAIL_EXIT1 ("socket (AF_UNIX, SOCK_DGRAM, 0): %m");
> + FAIL_EXIT1 ("socket (AF_UNIX, SOCK_STREAM, 0): %m");
>
> - xpthread_barrier_wait (&b2);
> + set_socket_buffer (tempfd2);
> +
> + if (connect (tempfd2, (struct sockaddr *) &sun, sizeof (sun)) != 0)
> + FAIL_EXIT1 ("connect: %m");
> +
> + unlink (sun.sun_path);
>
> xpthread_barrier_wait (&b2);
>
> + if (arg != NULL)
> + xpthread_barrier_wait (&b2);
> +
> pthread_cleanup_push (cl, NULL);
>
> - char mem[1];
> + char mem[WRITE_BUFFER_SIZE];
>
> - sendto (tempfd2, mem, arg == NULL ? sizeof (mem) : 1, 0,
> - (struct sockaddr *) &sun,
> - offsetof (struct sockaddr_un, sun_path) + strlen (sun.sun_path) + 1);
> + sendto (tempfd2, mem, arg == NULL ? sizeof (mem) : 1, 0, NULL, 0);
>
> pthread_cleanup_pop (0);
>
> diff --git a/sysdeps/unix/sysv/linux/mips/mips64/Makefile b/sysdeps/unix/sysv/linux/mips/mips64/Makefile
> index b4fb190..fcb48c0 100644
> --- a/sysdeps/unix/sysv/linux/mips/mips64/Makefile
> +++ b/sysdeps/unix/sysv/linux/mips/mips64/Makefile
> @@ -1,13 +1,3 @@
> -ifeq ($(subdir),socket)
> -CFLAGS-recv.c += -fexceptions
> -CFLAGS-send.c += -fexceptions
> -endif
> -
> -ifeq ($(subdir),nptl)
> -CFLAGS-recv.c += -fexceptions
> -CFLAGS-send.c += -fexceptions
> -endif
> -
> ifeq ($(subdir),signal)
> # sigaction.c defines static functions in asms and refers to them from
> # C code, resulting in "'restore_rt' used but never defined" (which
> diff --git a/sysdeps/unix/sysv/linux/riscv/rv64/Makefile b/sysdeps/unix/sysv/linux/riscv/rv64/Makefile
> deleted file mode 100644
> index cb60d74..0000000
> --- a/sysdeps/unix/sysv/linux/riscv/rv64/Makefile
> +++ /dev/null
> @@ -1,4 +0,0 @@
> -ifeq ($(subdir),socket)
> -CFLAGS-recv.c += -fexceptions
> -CFLAGS-send.c += -fexceptions
> -endif
>