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

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

Dear Jonathan,

Please see my comments below.

[...]

> 
> I'm not particularly well-versed in kmemleak, but nothing about this patch seems out of the
> ordinary.  Though I can still offer some small style-related suggestions.
> Also, do you want a reviewed-by for this?  I'd give one right now but I don't think I should be
> Acking RFC patches.

I sent V1 a few instants ago, I guess it is a better place for a RB.

[...]

> 
> Maybe we can reduce the number of fclose calls by capturing the fwrite return value:
> """
> bool igt_kmemleak_cmd(const char *cmd)
> {
> 	FILE *fp;
> 	size_t wlen;
> 
> 	fp = fopen(igt_kmemleak_file, "r+");
> 	if (!fp)
> 		return false;
> 
> 	wlen = fwrite(cmd, 1, strlen(cmd), fp);
> 	fclose(fp);
> 
> 	return wlen == strlen(cmd);
> }
> """
> Though that's not strictly necessary here.

Thank you for that! This change is on V1.

[...]

> 
> Same comment as with igt_kmemleak_cmd above:
> """
> bool igt_kmemleak_found_leak(void)
> {
> 	FILE *fp;
> 	char buf[1];
> 	size_t rlen;
> 
> 	fp = fopen(igt_kmemleak_file, "r");
> 	if (!fp)
> 		return false;
> 
> 	rlen = fread(buf, 1, 1, fp);
> 	fclose(fp);
> 
> 	return rlen > 0;
> }
> """
> Though, again, this isn't strictly necessary.

Thank you for that! This change is on V1.

[...]

> 
> I don't think we need comments like this for both the implementation
> and the header file.  The former should be sufficient.

Removed comments from the header file on V1.

[...]

> 
> test_empty_file and test_non_empty_file could probably be consolidated into a single
> "test_file" or "test_kmemleak_file" test with a Boolean determining if we want to write
> to the test file or not:
> """
> static void test_kmemleak_file(bool write)
> {
> 	char tmp_file_path[256] = "/tmp/igt_kmemleak_test_XXXXXX";
> 	int fd = mkstemp(tmp_file_path);
> 	igt_assert(fd >= 0);
> 
> 	if (write)
> 		write(fd, kmemleak_file_example, strlen(kmemleak_file_example));
> 
> 	/* Set the path for the unit testing file */
> 	igt_assert(igt_kmemleak_init(tmp_file_path));
> 	
> 	/* Test igt_kmemleak_found_leaks() returns true if written */
> 	igt_assert(igt_kmemleak_found_leaks() == write);
> 
> 	close(fd);

I moved the shared part to test_kmemleak_file() but kept the two separate functions
for the differences between them.

[...]

> 
> It might be better to separate these out into their own subtests:
> """
> 	igt_subtest_f("empty-file")
> 		test_empty_file();
> 	igt_subtest_f("non-empty-file")
> 		test_non_empty_file();
> 	igt_subtest_f("cp-leaks-file")
> 		test_igt_kmemleak_cp_leaks_file();
> """

Unfortunately this does not work. These are unit testing tests, and `meson test -C build`
gets confused by the macro igt_subtest_f. That or I am doing something wrong.


> 
> Though looking at it a bit closer, I see that test_igt_kmemleak_cp_leaks_file depends
> on the completion of test_non_empty_file to run properly?  Maybe we should separate
> the test out into chunks?

Oh, test_igt_kmemleak_cp_leaks_file() was broken. I changed it so that it has no dependencies,
and that it uses crc32_file()...

> 
> Let me try prototyping it here:
> """
> --- Original file up to end of crc32_file function is unchanged ---
> 
> static void init_kmemleak_file(bool write)
> {
> --- Same as test_kmemleak_file from earlier suggestion ---
> }
> 
> static void test_kmemleak_cp_leaks_file(void)
> {
> 	char dst_file_path[256] = "/tmp/igt_kmemleak_dstfile_XXXXXX";
> 	int fd;
> 	unsigned long crc;
> 
> 	init_kmemleak_file(true);
> 
> 	fd = mkstemp(dst_file_path);
> --- Rest of test is unchanged after this point ---
> }
> 
> igt_main
> {
> 	igt_subtest_f("empty")
> 		init_kmemleak_file(false);
> 	igt_subtest_f("non-empty")
> 		init_kmemleak_file(true);
> 	igt_subtest_f("cp-leaks-file")
> 		test_kmemleak_cp_leaks_file();
> }
> """

It ended slightly different than that. Let me know what you think.

Thank you!