Attention is currently required from: Felix Singer, Thomas Heijligen.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68433 )
Change subject: tests: Add prefix to io_mock functions not to clash with macros ......................................................................
Patch Set 7:
(3 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/68433/comment/5192d2f8_becdb4e1 PS5, Line 13: stays
typo: stay (without s)
Done
https://review.coreboot.org/c/flashrom/+/68433/comment/8326775a_03b8cc92 PS5, Line 10: because the latter are allowed : to be macros. Adding a prefix to flashrom mock functions avoids : them being accidentally expanded. Standard I/O functions are : expanded and flashrom mocks stays as they are
Why is it a problem that standard IO functions are macros? <...> How does the prefix prevent that the flashrom mocks are expanded?
The situation is: there are some functions which are allowed to be macros (standard I/O). Which means, the programmer code .c file can have for example `fopen(.....)` but then during pre-processing step `fopen` can be replaced with anything. It depends on the environment, but from the point of view of flashrom unit tests we don't know what `fopen` will be replaced with. And this is totally fine, because fopen is allowed to be a macro.
Now, just by coincidence, we have function pointers in our io_mock framework, which have the same names as standard I/O. So we also have `fopen`, but it's a different one, it is a member of io_mock struct. However, because the name clashes, our own fopen from io_mock is also replaced together with fopen from standard I/O. This is wrong, and should not happen.
The name clash is a coincidence, there are no reasons for the names from io_mock to be the same as standard I/O. We can have any names in io_mock. It makes sense to keep some reference between original and mock function, to have code more readable. This is why the idea was to add a prefix, `iom_`. With the prefix, names do not clash anymore. Function with name iom_fopen will not be replaced with fopen macro. This is what we need: original functions are pre-processed as needed, but functions in io_mock stay as is.
I don't understand the relation between that function pointer and the standard I/O functions.
The relation is only logical, as the relation between original function and its custom mock. This is why we can add prefix and rename our function pointers in io_mock.
Shouldn't undefining _FORTIFY_SOURCE solve that problem? In my understanding standard I/O functions are not expanded when _FORTIFY_SOURCE is undefined.
So just to re-iterate, we have two sets of functions in question: 1) standard I/O 2) flashrom own io_mock
With the help of undefining _FORTIFY_SOURCE we can avoid one of the cases when standard I/O functions are not expanded. _FORTIFY_SOURCE helps to deal with one of the cases for 1) It is very convenient, because it reduces the number of wraps we need, less noise in the code, more readable code.
This patch solves the problem for 2) In general, there could be many more different macro expansions, which are completely independent of _FORTIFY_SOURCE. And in general, we cannot block them, we actually want the all to work normally. However, we don't want our functions 2) to get expanded. So we make names not to clash and solve this.
File tests/io_mock.h:
https://review.coreboot.org/c/flashrom/+/68433/comment/bd584e3c_cd627f88 PS5, Line 73: /* Port I/O */ : void (*outb)(void *state, unsigned char value, unsigned short port); : unsigned char (*inb)(void *state, unsigned short port); : : void (*outw)(void *state, unsigned short value, unsigned short port); : unsigned short (*inw)(void *state, unsigned short port); : : void (*outl)(void *state, unsigned int value, unsigned short port); : unsigned int (*inl)(void *state, unsigned short port); : : /* USB I/O */ : int (*libusb_init)(void *state, libusb_context **ctx); : int (*libusb_control_transfer)(void *state, : libusb_device_handle *devh, : uint8_t bmRequestType, : uint8_t bRequest, : uint16_t wValue, : uint16_t wIndex, : unsigned char *data, : uint16_t wLength, : unsigned int timeout); : ssize_t (*libusb_get_device_list)(void *state, libusb_context *, libusb_device ***list); : void (*libusb_free_device_list)(void *state, libusb_device **list, int unref_devices); : int (*libusb_get_device_descriptor)(void *state, libusb_device *, struct libusb_device_descriptor *); : int (*libusb_get_config_descriptor)(void *state, : libusb_device *, : uint8_t config_index, : struct libusb_config_descriptor **); : void (*libusb_free_config_descriptor)(void *state, struct libusb_config_descriptor *);
I am wondering, should these functions be renamed as well?
They could, but if they cannot ever be macros, then it doesn't matter.