* Re: Perf review prompts
2026-05-16 1:09 ` Arnaldo Carvalho de Melo
@ 2026-05-17 3:12 ` Ian Rogers
0 siblings, 0 replies; 3+ messages in thread
From: Ian Rogers @ 2026-05-17 3:12 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo; +Cc: linux-perf-users, Namhyung Kim, Chris Mason
On Fri, May 15, 2026 at 6:09 PM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> On Fri, May 15, 2026 at 04:52:43PM -0700, Ian Rogers wrote:
> > I think we can improve the review prompts for Sashiko and others, so I
> > sent out this PR:
> > https://github.com/masoncl/review-prompts/pull/62
> > I wanted to advertise this and suggest that others contribute similarly.
>
> Thanks for working on this, from a quick look it seems sane, some
> comments:
>
> -----
> The "File Descriptor and Directory Stream Management" is describing
> the dir equivalent of open and close and saying they need to be
> paired. Assume this is base knowledge for an AI code review tool.
> -----
>
> That is completely reasonable expectation, I would be surprised if it
> wouldn't catch this, but have you tested it?
So I was able to use a sub-agent to review the changes, here is the
output from antigravity:
### Verification & Testing Plan
To verify that the baseline prompts correctly catch unclosed
directory streams without the verbose advice
in perf.md , we constructed a synthetic patch demonstrating a
directory stream ( DIR * ) leak on an
early error return path.
#### 1. Test Diff ( test_dir_leak.patch )
diff --git a/tools/perf/util/synthetic_dir_test.c
b/tools/perf/util/synthetic_dir_test.c
new file mode 100644
index 0000000..1234567
--- /dev/null
+++ b/tools/perf/util/synthetic_dir_test.c
@@ -0,0 +1,25 @@
+#include <dirent.h>
+#include <fcntl.h>
+#include <unistd.h>
+#include <stdio.h>
+
+int process_proc_fd_info(int pid_fd)
+{
+ DIR *dir;
+ struct dirent *ent;
+
+ dir = fdopendir(pid_fd);
+ if (!dir)
+ return -1;
+
+ while ((ent = readdir(dir)) != NULL) {
+ if (ent->d_name[0] == '.')
+ continue;
+ if (do_something_with_entry(ent) < 0)
+ return -1; /* BUG: Missing closedir(dir) */
+ }
+
+ closedir(dir);
+ return 0;
+}
#### 2. AI Review Execution & Empirical Results
We ran a full review session against this patch using the standard
review-core.md protocol (which loads
technical-patterns.md and the streamlined subsystem/perf.md ).
The AI reviewer successfully caught the unclosed stream on the error
branch and generated the following
exact inline review output ( review-inline.txt ):
> diff --git a/tools/perf/util/synthetic_dir_test.c
b/tools/perf/util/synthetic_dir_test.c
> new file mode 100644
> index 0000000..1234567
> --- /dev/null
> +++ b/tools/perf/util/synthetic_dir_test.c
> @@ -0,0 +1,25 @@
...
> + while ((ent = readdir(dir)) != NULL) {
> + if (ent->d_name[0] == '.')
> + continue;
> + if (do_something_with_entry(ent) < 0)
> + return -1;
If do_something_with_entry() returns an error, does this code leak the
directory stream 'dir'?
Should this call closedir(dir) before returning -1?
#### 3. Conclusion
The test proves that the AI reviewer's baseline C knowledge, combined
with the general resource lifecycle
rules in technical-patterns.md ( alloc→init→use→cleanup→free ) and
the remaining Quick Checks in
subsystem/perf.md , is fully sufficient to catch missing directory
stream cleanups. The removed section
was indeed redundant.
> For the validation of perf.data headers I plan to contribute what I
> learned while working on:
>
> [PATCH 00/28] perf: Harden perf.data parsing against crafted/corrupted files
> https://lore.kernel.org/all/20260510033424.255812-1-acme@kernel.org>
>
> That I hope soon to satisfy the last review comments from sashiko
> addressed.
>
> Anyway, I think this can be added FWIW:
>
> Reviewed-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Thanks!
Ian
> - Arnaldo
^ permalink raw reply [flat|nested] 3+ messages in thread