Attention is currently required from: Edward O'Callaghan, Angel Pons, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/56413 )
Change subject: tests: Mock file i/o for linux_mtd and linux_spi tests ......................................................................
Patch Set 9:
(6 comments)
File tests/init_shutdown.c:
https://review.coreboot.org/c/flashrom/+/56413/comment/a9ea6a39_6fefa236 PS4, Line 219: //
"//" is what's in the actual path which is constructed by linux_mtd. […]
`//` is generally accepted by the syscalls. I think that means the tests should accept it as well. But actually they should accept what Linux accepts, and not just what the driver happens to produce right now.
Maybe fnmatch() could be used?
File tests/init_shutdown.c:
https://review.coreboot.org/c/flashrom/+/56413/comment/14895fee_8141c827 PS9, Line 206: io_state->fopen_path = pathname; We should keep a copy (strdup()). There is no guarantee* that the pointer stays valid.
(* assuming we want to test that the driver is functional, and not that the driver code always stays the same)
https://review.coreboot.org/c/flashrom/+/56413/comment/d8639a62_3b120d56 PS9, Line 208: return (void *)0x55AA; /* valid file descriptor */ I have to say, I only understood the comment by reading the code (and would have understood that without the comment too). Technically, the comment is wrong. It's a pointer (could refer to it as handle), not a file descriptor. The file descriptor is the `int` that is handled by the kernel and returned by open(). Which we obviously don't have here. Also, we don't need a *valid* pointer, we need one that is not `NULL`, actually no matter if it's valid.
Commonly, code is better than comments, we could introduce something like this
#define NOT_NULL ((void *)0xf000baaa)
That would move the discussion and any potential confusion to a single place and make it obvious what is going on every time the macro is used.
https://review.coreboot.org/c/flashrom/+/56413/comment/1c9a3928_37d5e058 PS9, Line 233: const struct linux_mtd_fread_mock_entry entry = fread_mock_map[i]; This creates a copy of the struct, looks like a pointer would do as well.
https://review.coreboot.org/c/flashrom/+/56413/comment/0c26de91_ea412139 PS9, Line 236: + 1 I don't think the files in sysfs provide a 0-termination? There's a linebreak sometimes, though.
File tests/io_mock.h:
https://review.coreboot.org/c/flashrom/+/56413/comment/7890fca3_a3c5e1ca PS9, Line 86: /* File I/O */ This is usually referred to as `stdio` or `Standard I/O`.
-
Nico Huber (Code Review)