Attention is currently required from: Thomas Heijligen, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk.

View Change

7 comments:

• File tests/init_shutdown.c:

  • Patch Set #5, 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.)

  • Patch Set #5, 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.

  • Patch Set #5, Line 140: sprintf(raiden_debug_param, "address=%s", USB_DEVICE_ADDRESS);

    Please use snprintf().

  • Patch Set #5, 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:

  • Patch Set #5, 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.

  • Patch Set #5, 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:

  • Patch Set #4, 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 ;)

To view, visit change 57918. To unsubscribe, or for help writing mail filters, visit settings.