Re: [PATCH 1/2] selftests: x86: test_vsyscall: conform test to TAP format output

[Muhammad Usama Anjum <usama.anjum@collabora.com>]

On 3/27/24 1:00 AM, Shuah Khan wrote:
> 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.
Counting total number of tests based on architecture was really difficult
until the code needed to be moved around. I'll split this patch into 2. The
first part would just simplify the test by moving functions around and use
#ifdef precisely. The seconds part would convert it to the TAP.

> 
>>   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
> 

-- 
BR,
Muhammad Usama Anjum