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

[Peter Senna Tschudin <peter.senna@linux.intel.com>]

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@friendship7-home:~/UPSTREAM/igt-gpu-tools/lib$ git grep igt_warn|wc -l
164
psennats@friendship7-home:~/UPSTREAM/igt-gpu-tools/lib$ cd ..
psennats@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?


> 
>> +					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@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@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.

[...]