Received-SPF: pass (google.com: domain of linux-kernel+bounces-119851-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <a1a6f5dd-80ab-4d53-90de-eb003c996cc1@linuxfoundation.org>
Date: Tue, 26 Mar 2024 14:00:29 -0600
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 1/2] selftests: x86: test_vsyscall: conform test to TAP
 format output
Content-Language: en-US
To: Muhammad Usama Anjum <usama.anjum@collabora.com>,
 Shuah Khan <shuah@kernel.org>
Cc: kernel@collabora.com, linux-kselftest@vger.kernel.org,
 linux-kernel@vger.kernel.org, Shuah Khan <skhan@linuxfoundation.org>
References: <20240314103216.286603-1-usama.anjum@collabora.com>
From: Shuah Khan <skhan@linuxfoundation.org>
In-Reply-To: <20240314103216.286603-1-usama.anjum@collabora.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

On 3/14/24 04:32, Muhammad Usama Anjum wrote:
> Conform the layout, informational and status messages to TAP. No
> functional change is intended other than the layout of output messages.
> Without using TAP messages, the passed/failed/skip test names cannot be
> found.
> 
> Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
> ---


I am seeing lot more changes than conform and formatting changes.

>   tools/testing/selftests/x86/test_vsyscall.c | 506 +++++++++-----------
>   1 file changed, 238 insertions(+), 268 deletions(-)
> 
> diff --git a/tools/testing/selftests/x86/test_vsyscall.c b/tools/testing/selftests/x86/test_vsyscall.c
> index 47cab972807c4..d4c8e8d79d389 100644
> --- a/tools/testing/selftests/x86/test_vsyscall.c
> +++ b/tools/testing/selftests/x86/test_vsyscall.c
> @@ -21,6 +21,13 @@
>   #include <sys/uio.h>
>   
>   #include "helpers.h"
> +#include "../kselftest.h"
> +
> +#ifdef __x86_64__
> +#define TOTAL_TESTS 13
> +#else
> +#define TOTAL_TESTS 8
> +#endif
>   
>   #ifdef __x86_64__
>   # define VSYS(x) (x)
> @@ -39,18 +46,6 @@
>   /* max length of lines in /proc/self/maps - anything longer is skipped here */
>   #define MAPS_LINE_LEN 128
>   
> -static void sethandler(int sig, void (*handler)(int, siginfo_t *, void *),
> -		       int flags)
> -{
> -	struct sigaction sa;
> -	memset(&sa, 0, sizeof(sa));
> -	sa.sa_sigaction = handler;
> -	sa.sa_flags = SA_SIGINFO | flags;
> -	sigemptyset(&sa.sa_mask);
> -	if (sigaction(sig, &sa, 0))
> -		err(1, "sigaction");
> -}
> -

Why is this deleted?

>   /* vsyscalls and vDSO */
>   bool vsyscall_map_r = false, vsyscall_map_x = false;
>   
> @@ -75,83 +70,25 @@ static void init_vdso(void)
>   	if (!vdso)
>   		vdso = dlopen("linux-gate.so.1", RTLD_LAZY | RTLD_LOCAL | RTLD_NOLOAD);
>   	if (!vdso) {
> -		printf("[WARN]\tfailed to find vDSO\n");
> +		ksft_print_msg("[WARN] failed to find vDSO\n");
>   		return;
>   	}
>   
>   	vdso_gtod = (gtod_t)dlsym(vdso, "__vdso_gettimeofday");
>   	if (!vdso_gtod)
> -		printf("[WARN]\tfailed to find gettimeofday in vDSO\n");
> +		ksft_print_msg("[WARN] failed to find gettimeofday in vDSO\n");
>   
>   	vdso_gettime = (vgettime_t)dlsym(vdso, "__vdso_clock_gettime");
>   	if (!vdso_gettime)
> -		printf("[WARN]\tfailed to find clock_gettime in vDSO\n");
> +		ksft_print_msg("[WARN] failed to find clock_gettime in vDSO\n");
>   
>   	vdso_time = (time_func_t)dlsym(vdso, "__vdso_time");
>   	if (!vdso_time)
> -		printf("[WARN]\tfailed to find time in vDSO\n");
> +		ksft_print_msg("[WARN] failed to find time in vDSO\n");
>   
>   	vdso_getcpu = (getcpu_t)dlsym(vdso, "__vdso_getcpu");
>   	if (!vdso_getcpu)
> -		printf("[WARN]\tfailed to find getcpu in vDSO\n");
> -}
> -
> -static int init_vsys(void)
> -{
> -#ifdef __x86_64__
> -	int nerrs = 0;
> -	FILE *maps;
> -	char line[MAPS_LINE_LEN];
> -	bool found = false;
> -
> -	maps = fopen("/proc/self/maps", "r");
> -	if (!maps) {
> -		printf("[WARN]\tCould not open /proc/self/maps -- assuming vsyscall is r-x\n");
> -		vsyscall_map_r = true;
> -		return 0;
> -	}
> -
> -	while (fgets(line, MAPS_LINE_LEN, maps)) {
> -		char r, x;
> -		void *start, *end;
> -		char name[MAPS_LINE_LEN];
> -
> -		/* sscanf() is safe here as strlen(name) >= strlen(line) */
> -		if (sscanf(line, "%p-%p %c-%cp %*x %*x:%*x %*u %s",
> -			   &start, &end, &r, &x, name) != 5)
> -			continue;
> -
> -		if (strcmp(name, "[vsyscall]"))
> -			continue;
> -
> -		printf("\tvsyscall map: %s", line);
> -
> -		if (start != (void *)0xffffffffff600000 ||
> -		    end != (void *)0xffffffffff601000) {
> -			printf("[FAIL]\taddress range is nonsense\n");
> -			nerrs++;
> -		}
> -
> -		printf("\tvsyscall permissions are %c-%c\n", r, x);
> -		vsyscall_map_r = (r == 'r');
> -		vsyscall_map_x = (x == 'x');
> -
> -		found = true;
> -		break;
> -	}
> -
> -	fclose(maps);
> -
> -	if (!found) {
> -		printf("\tno vsyscall map in /proc/self/maps\n");
> -		vsyscall_map_r = false;
> -		vsyscall_map_x = false;
> -	}
> -
> -	return nerrs;
> -#else
> -	return 0;
> -#endif
> +		ksft_print_msg("[WARN] failed to find getcpu in vDSO\n");
>   }
>   

What's going on here?

These changes are more extensive than the changelog indicates.
Additional explanation is needed before I can accept this.

thanks,
-- Shuah