Re: [PATCH v5] perf record: Fix unhandled POLLHUP on non_perf_event descriptors

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The patch completely omits necessary modifications to `tools/lib/api/fd/array.c`, causing the newly added unit test to fail deterministically.
--

--- Patch [1]: [PATCH v5] perf record: Fix unhandled POLLHUP on non_perf_event descriptors ---
commit dd8edd3a80b47efe7508c7fe4d3a8d1e99fd10d2
Author: Ian Rogers <[email protected]>

perf record: Fix unhandled POLLHUP on non_perf_event descriptors

This commit attempts to fix a POLLHUP issue by updating the pollfd state in
record__update_evlist_pollfd_from_thread() and reverting logic in
fdarray__filter(). It also introduces a unit test to enforce that
nonfilterable items bypass the revents filtering loop.

> diff --git a/tools/perf/tests/fdarray.c b/tools/perf/tests/fdarray.c
> index 40983c3574b12..2d3db7b754a17 100644
> --- a/tools/perf/tests/fdarray.c
> +++ b/tools/perf/tests/fdarray.c
[ ... ]
> @@ -80,6 +80,30 @@ static int test__fdarray__filter(struct test_suite *test __maybe_unused, int sub
>  		goto out_delete;
>  	}
>  
> +	fdarray__init_revents(fda, POLLHUP);
> +	fda->priv[2].flags = fdarray_flag__nonfilterable;
> +
> +	pr_debug("\nfiltering all but fda->entries[2] (nonfilterable):");
> +	fdarray__fprintf_prefix(fda, "before", stderr);
> +	nr_fds = fdarray__filter(fda, POLLHUP, NULL, NULL);
> +	fdarray__fprintf_prefix(fda, " after", stderr);
> +
> +	if (nr_fds != 0) {
> +		pr_debug("\nfdarray__filter()=%d != 0, should be 0\n",
> +			 nr_fds);
> +		goto out_delete;
> +	}
> +	if (fda->entries[2].fd == -1) {

[Severity: Medium]
The commit message mentions reverting logic in fdarray__filter() to bypass
nonfilterable events, but the actual modifications to tools/lib/api/fd/array.c
appear to be missing from this patch.

Since the logic to bypass nonfilterable items wasn't included, does this
new test in test__fdarray__filter() fail deterministically because the
underlying behavior hasn't changed?

> +		pr_debug("\nfdarray__filter() illegally modified nonfilterable fd!");
> +		goto out_delete;
> +	}
> +	if (fda->entries[2].revents != POLLHUP) {
> +		pr_debug("\nfdarray__filter() illegally modified nonfilterable revents!");
> +		goto out_delete;
> +	}
> +
> +	fda->priv[2].flags = 0; /* reset flags */
> +
>  	pr_debug("\n");
>  
>  	err = 0;

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