Attention is currently required from: Thomas Heijligen, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/57918 )
Change subject: tests: Add init-shutdown test for raiden_debug_spi ......................................................................
Patch Set 5:
(7 comments)
File tests/init_shutdown.c:
https://review.coreboot.org/c/flashrom/+/57918/comment/ca0cfa38_7a94c60e PS5, Line 60: **list = calloc(1, sizeof(*list)); This looks wrong and shouldn't be necessary. The second sizeof() should be that of a `libusb_device` struct. But as you mention below the device is opaque, so we don't need it.
Maybe it helps to imagine it without dynamic allocation. For instance, using static variables:
static libusb_device single_dev; static libusb_device *single_entry_list[1] = { [0] = &single_dev };
*list = single_entry_list; **list = &single_dev; /* redundant with the initialization above */
Now, would it make a difference if we'd use `NULL` instead of `&single_dev`? Only if the caller would explicitly for non-NULL, I'd say.
(Looking at it now, using a static array would actually work. As long the list is immutable, even multiple calls to libusb_get_device_list() would be ok.)
https://review.coreboot.org/c/flashrom/+/57918/comment/2b5b2346_2fa3c8a0 PS5, Line 122: (void *)
I added cast to avoid warnings about discarding const qualifier, hope it's ok.
I think anything else would be overkill for mocking.
NB. We don't put spaces after casts.
https://review.coreboot.org/c/flashrom/+/57918/comment/73eee25d_dcdab415 PS5, Line 140: sprintf(raiden_debug_param, "address=%s", USB_DEVICE_ADDRESS); Please use snprintf().
https://review.coreboot.org/c/flashrom/+/57918/comment/15220623_1b546a17 PS5, Line 140: USB_DEVICE_ADDRESS Why is the constant a string? Wouldn't it be easier to use %d and a number?
File tests/io_mock.h:
https://review.coreboot.org/c/flashrom/+/57918/comment/069087c4_99bb87b0 PS5, Line 43: /* Typedef to make code simpler (no need to type struct all the time). */ It also makes it oblivious to the reader that these are structs. Maybe that's obvious from the context, though.
Generally, it's discouraged in flashrom. I don't mind it in tests/, your call.
https://review.coreboot.org/c/flashrom/+/57918/comment/5e366aa6_cb560420 PS5, Line 90: ctx It's not necessary to name every parameter. For instance `ctx` is already obvious from the type so it could just be `libusb_context *,` saying a compatible pointer needs to be passed (doesn't matter how the callee names it). Same for `dev`, `desc` etc.
File tests/io_mock.h:
https://review.coreboot.org/c/flashrom/+/57918/comment/0882f082_63bb3c5f PS4, Line 42: struct libusb_endpoint_descriptor { : uint8_t offset1[2]; : uint8_t bEndpointAddress; : uint8_t bmAttributes; : uint8_t offset2[5]; : unsigned char *extra; : int extra_length; : }; : typedef struct libusb_endpoint_descriptor libusb_endpoint_descriptor; : : struct libusb_interface_descriptor { : uint8_t offset1[2]; : uint8_t bInterfaceNumber; : uint8_t bAlternateSetting; : uint8_t bNumEndpoints; : uint8_t bInterfaceClass; : uint8_t bInterfaceSubClass; : uint8_t bInterfaceProtocol; : uint8_t offset2; : struct libusb_endpoint_descriptor *endpoint; : }; : typedef struct libusb_interface_descriptor libusb_interface_descriptor; : : struct libusb_interface { : struct libusb_interface_descriptor *altsetting; : int num_altsetting; : }; : typedef struct libusb_interface libusb_interface; : : struct libusb_config_descriptor { : uint8_t offset1[4]; : uint8_t bNumInterfaces; : uint8_t bConfigurationValue; : uint8_t offset2[3]; : struct libusb_interface *interface; : }; : typedef struct libusb_config_descriptor libusb_config_descriptor; : : struct libusb_device {}; : typedef struct libusb_device libusb_device; : : struct libusb_device_descriptor { : uint8_t offset1[8]; : uint16_t idVendor; : uint16_t idProduct; : uint8_t offset2[5]; : uint8_t bNumConfigurations; : }; : typedef struct libusb_device_descriptor libusb_device_descriptor;
I included the header only once in io_mock.h and it seems to work! […]
It's already there. In `meson.build`, `flashrom_test_dep` depends on `deps` which has `libusb` added by defalult (set by the `usb` option). Don't ask me if any of it makes sense, but it's there ;)