Re: [PATCH v3 09/12] KVM: selftests: Add KVM syscall wrappers for pthread_{cancel,join}()

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The `kvm_pthread_join` and `kvm_pthread_cancel` wrappers generate misleading error messages on failure because they use `__KVM_SYSCALL_ERROR`, which prints `errno` and `strerror(errno)`. However, `pthread_join` and `pthread_cancel` return error numbers directly and do not set `errno`.
--

--- Patch [9]: [PATCH v3 09/12] KVM: selftests: Add KVM syscall wrappers for pthread_{cancel,join}() ---
commit 26558313a1f5853d420a8a8544d24882f9b4835b
Author: Sean Christopherson <[email protected]>

KVM: selftests: Add KVM syscall wrappers for pthread_{cancel,join}()

This commit adds and uses wrappers for pthread_cancel() and pthread_join()
across KVM selftests. The wrappers aim to automatically assert on failure,
removing the need for manual assertions in the test code.

> diff --git a/tools/testing/selftests/kvm/include/kvm_syscalls.h b/tools/testing/selftests/kvm/include/kvm_syscalls.h
> index e75be83787798..d6c18e13b9c8b 100644
> --- a/tools/testing/selftests/kvm/include/kvm_syscalls.h
> +++ b/tools/testing/selftests/kvm/include/kvm_syscalls.h
> @@ -101,6 +101,8 @@ __KVM_SYSCALL_DEFINE(sched_setaffinity, 3, pid_t, pid, size_t, cpusetsize, cpu_s
>  typedef void *(*pthread_fn_t)(void *);
>  __KVM_SYSCALL_DEFINE(pthread_create, 4, pthread_t *, thread,
>  		     const pthread_attr_t *, attr, pthread_fn_t, fn, void *, arg);
> +__KVM_SYSCALL_DEFINE(pthread_join, 2, pthread_t, thread, void **, thread_return);
> +__KVM_SYSCALL_DEFINE(pthread_cancel, 1, pthread_t, thread);

[Severity: Medium]
Will this generate misleading error messages if the thread functions fail?

The pthread_join() and pthread_cancel() functions return their error codes
directly rather than setting errno. If the __KVM_SYSCALL_DEFINE macro
uses errno for its error reporting, it could log an unrelated error
string instead of the actual failure reason.

>  
>  #define kvm_free_fd(fd)		\
>  do {				\

[ ... ]

> diff --git a/tools/testing/selftests/kvm/x86/xen_shinfo_test.c b/tools/testing/selftests/kvm/x86/xen_shinfo_test.c
> index 20f518fe24506..3ec5e67b21efa 100644
> --- a/tools/testing/selftests/kvm/x86/xen_shinfo_test.c
> +++ b/tools/testing/selftests/kvm/x86/xen_shinfo_test.c
> @@ -948,11 +947,8 @@ int main(int argc, char *argv[])
>  				TEST_ASSERT(!evtchn_irq_expected,
>  					    "Expected event channel IRQ but it didn't happen");
>  
> -				ret = pthread_cancel(thread);
> -				TEST_ASSERT(ret == 0, "pthread_cancel() failed: %s", strerror(ret));
> -
> -				ret = pthread_join(thread, 0);
> -				TEST_ASSERT(ret == 0, "pthread_join() failed: %s", strerror(ret));
> +				kvm_pthread_cancel(thread);
> +				kvm_pthread_join(thread, 0);

[Severity: Medium]
Does this change replace an accurate error message with a potentially
misleading one?

The original code correctly used strerror(ret) because these functions
return the error code directly. If the new wrappers rely on errno, they
might print an unrelated error string when these functions fail.

>  				goto done;
>  
>  			case TEST_GUEST_SAW_IRQ:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=9