Re: [PATCH] nptl: Fix tst-cancel4 sendto tests

[Adhemerval Zanella <adhemerval dot zanella at linaro dot 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
>