Re: [LTP] [PATCH v2] userfaultfd: Use two-step handshake to probe features

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> +	/*
> +	 * Two-step handshake to check if feature is available
> +	 */

Please no obvious comments like this.

> +	uffd = SAFE_USERFAULTFD(O_CLOEXEC | O_NONBLOCK, false);
> +	uffdio_api.api = UFFD_API;
> +	SAFE_IOCTL(uffd, UFFDIO_API, &uffdio_api);
> +	if (!(uffdio_api.features & UFFD_FEATURE_PAGEFAULT_FLAG_WP))
> +		tst_brk(TCONF, "UFFD_FEATURE_PAGEFAULT_FLAG_WP not supported");
> +	SAFE_CLOSE(uffd);

I do not get why are we adding this code when we do the same just a few
lines below. Shouldn't just this suffice?

diff --git a/testcases/kernel/syscalls/userfaultfd/userfaultfd05.c b/testcases/kernel/syscalls/userfaultfd/userfaultfd05.c
index e25a227cf..b19132d0c 100644
--- a/testcases/kernel/syscalls/userfaultfd/userfaultfd05.c
+++ b/testcases/kernel/syscalls/userfaultfd/userfaultfd05.c
@@ -95,6 +95,9 @@ static void run(void)
 
        SAFE_IOCTL(uffd, UFFDIO_API, &uffdio_api);
 
+       if (!(uffdio_api.features & UFFD_FEATURE_PAGEFAULT_FLAG_WP))
+               tst_brk(TCONF, "UFFD_FEATURE_PAGEFAULT_FLAG_WP not supported");
+
        uffdio_register.range.start = (unsigned long) page;
        uffdio_register.range.len = page_size;
        uffdio_register.mode = UFFDIO_REGISTER_MODE_WP;


> index 5b1252c35..e1b3c2f6f 100644
> --- a/testcases/kernel/syscalls/userfaultfd/userfaultfd06.c
> +++ b/testcases/kernel/syscalls/userfaultfd/userfaultfd06.c
> @@ -97,18 +97,25 @@ static void run(void)
>  
>  	poison_fault_seen = 0;
>  	sigbus_seen = 0;
> -	set_pages();
>  
> +	/*
> +	 * Two-step handshake to check if feature is available
> +	 */
>  	uffd = SAFE_USERFAULTFD(O_CLOEXEC | O_NONBLOCK, false);
> +	uffdio_api.api = UFFD_API;
> +	SAFE_IOCTL(uffd, UFFDIO_API, &uffdio_api);
> +	if (!(uffdio_api.features & UFFD_FEATURE_POISON))
> +		tst_brk(TCONF, "UFFD_FEATURE_POISON not supported");
> +	SAFE_CLOSE(uffd);
>  
> +	set_pages();
> +
> +	uffd = SAFE_USERFAULTFD(O_CLOEXEC | O_NONBLOCK, false);
>  	uffdio_api.api = UFFD_API;
>  	uffdio_api.features = UFFD_FEATURE_POISON;
>
>  	SAFE_IOCTL(uffd, UFFDIO_API, &uffdio_api);
>  
> -	if (!(uffdio_api.features & UFFD_FEATURE_POISON))
> -		tst_brk(TCONF, "UFFD_FEATURE_POISON not supported");
> -

This does not seem to make any sense. The code already checks for the
feature, or do I miss something?

>  	uffdio_register.range.start = (unsigned long) page;
>  	uffdio_register.range.len = page_size;
>  	uffdio_register.mode = UFFDIO_REGISTER_MODE_MISSING;
> -- 
> 2.53.0
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp