[PATCH i-g-t v4 1/2] lib/igt_kmemleak: library to interact with kmemleak

[Zbigniew Kempczyński]

On Mon, Feb 03, 2025 at 10:23:00AM +0100, Peter Senna Tschudin wrote:
> Hi Kamil,
> 
> Thank you for your review. Please see below.
> 
> On 31.01.2025 13:34, Kamil Konieczny wrote:
> 
> [...]
> 
> >> +#include <stdio.h>
> >> +#include <string.h>
> >> +#include <unistd.h>
> >> +#include <fcntl.h>
> >> +#include <errno.h>
> > 
> > Sort this alphabetically with exception of unistd.h - this
> > header could be first.
> 
> That is picky, but ok.
> 
> 
> > 
> >> +
> >> +#include "igt_core.h"
> >> +#include "igt_kmemleak.h"
> >> +
> >> +/* We can change the path for unit testing, see igt_kmemleak_init() */
> >> +static char igt_kmemleak_file[256] = "/sys/kernel/debug/kmemleak";
> >> +
> >> +#define MAX_WRITE_RETRIES 5
> >> +/**
> >> + * igt_kmemleak_write - Writes the buffer to the file descriptor retrying when
> >> + * possible.
> >> + * @fd: The file descriptor to write to.
> >> + * @buf: Pointer to the data to write.
> >> + * @count: Total number of bytes to write.
> >> + *
> >> + * Returns the total number of bytes written on success, or -1 on failure.
> >> + */
> >> +static bool igt_kmemleak_write(int fd, const void *buf, size_t count)
> >> +{
> >> +	const char *ptr = buf;
> >> +	size_t remaining = count;
> >> +	ssize_t written;
> >> +	int retries = 0;
> >> +
> >> +	while (remaining > 0) {
> >> +		written = write(fd, ptr, remaining);
> >> +		if (written > 0) {
> >> +			ptr += written;
> >> +			remaining -= written;
> >> +		} else if (written == -1) {
> >> +			if (errno == EINTR || errno == EAGAIN) {
> >> +				/* Retry for recoverable errors */
> >> +				if (++retries > MAX_WRITE_RETRIES) {
> >> +					igt_warn("igt_write: Exceeded retry limit\n");
> > 
> > Do not use test prints in lib, especially those used by runner.
> 
> Oh, I will. kmemleak is about writing to disk. If that fails, I will tell the user.
> Why does this restriction applies only to me? See:
> 
> ---
> psennats at friendship7-home:~/UPSTREAM/igt-gpu-tools/lib$ git grep igt_warn|wc -l
> 164
> psennats at friendship7-home:~/UPSTREAM/igt-gpu-tools/lib$ cd ..
> psennats at friendship7-home:~/UPSTREAM/igt-gpu-tools$ git grep igt_warn|wc -l
> 324
> ---
> 
> About 50% of all references to igt_warn are in lib, what is the problem with my code?

I'm looking at the code and I have impression this should be part of
the runner, not igt library. Do I understand correctly that your
code will be used by the runner only and not by the tests?

Regarding your above question, I think runner should use as much as
minimal from lib/ directory, because this is collection of functions
designed for tests, not for runner. I think generic code like data
structures (lists) are fine, but not the others. Imagine you'll
incidentally try to use igt_require() - follow the call and see
the pitfall there.

So if my understanding is correct and this code should be part of the
runner only your test might be moved to the runner_tests.c.

BTW please rebase on top of current master, there were changes in
the runner/settings.c file that affects this series.

--
Zbigniew

> 
> 
> > 
> >> +					return false;
> >> +				}
> >> +				continue;
> >> +			} else {
> >> +				/* Log unrecoverable error */
> >> +				igt_warn("igt_write: unrecoverable write error");
> > 
> > Same here.
> We should not remove this one.
> 
> > 
> >> +				return false;
> >> +			}
> >> +		} else if (written == 0) {
> >> +			if (++retries > MAX_WRITE_RETRIES) {
> >> +				igt_warn("igt_write: Exceeded retry limit\n");
> > 
> > Same here.
> We should not remove this one.
> 
> [...]
> 
> >> +	FILE *fp;
> > 
> > Why FILE* and fopen? Could you just use open/read?
> 
> Why not?
> 
> ---
> psennats at friendship7-home:~/UPSTREAM/igt-gpu-tools$ git grep fopen|wc -l
> 97
> ---
> 
> 
> [...]
> 
> >> +bool igt_kmemleak_init(const char *unit_test_kmemleak_file)
> >> +{
> >> +	FILE *fp;
> > 
> > Same here, open/read.
> 
> Same here, why not?
> ---
> psennats at friendship7-home:~/UPSTREAM/igt-gpu-tools$ git grep fopen|wc -l
> 97
> ---
> 
> [...]
> 
> >> +#define KMEMLEAKRESFILENAME "kmemleak.txt"
> > 
> > imho better: IGT_KMEMLEAK_RESFILENAME
> 
> It reads better indeed. Thanks.
> 
> [...]