libusb(3): Expose device capabilities as libusb_bos_descriptor::dev_capability

[kevans]

Summary

Some libusb consumers in Linux-land (in this case, libusb4java) expect
a dev_capability member that they can use to enumerate the device capabilities.

No particular layout is expected of this, just that it can be traversed using the bLength
member until bNumDeviceCapabilities are read and that the consumer may then use one of the
libusb_get_*_descriptor methods to extract specific (usb 2.0 vs. ss) capability information.

[• hselasky]

lib/libusb/libusb10_desc.c

496 ↗

(On Diff #30454)

Can you make this into a range comparison, using char * instead void * ?

515 ↗

(On Diff #30454)

Same here.

[kevans]

(void *) => (char *)

[• hselasky]

lib/libusb/libusb10_desc.c

496 ↗

(On Diff #30458)

Why are you not checking dlen >= XXXX ?

[kevans]

lib/libusb/libusb10_desc.c

496 ↗

(On Diff #30458)

Do I need to? We've already validated the length for the type of capability block, then consumers of this are expected to traverse it with something generally like:

struct libusb_bos_dev_capability_descriptor *check = ptr->dev_capability;
int read = 0;
while (read < ptr->bNumDeviceCapabilities) {
    ++read;
    ...
    check = (char *)check + check->bLength;
}

so we point to the earliest (w.r.t memory layout) capability and the consumer sorts it out by length.

[cem]

I'm afraid I'm totally unqualified to review this patch, being unfamiliar with libusb.

lib/libusb/libusb10_desc.c

516 ↗

(On Diff #30458)

Why does a smaller pointer value indicate ss_cap should override the existing value? I don't understand this.

[• hselasky]

lib/libusb/libusb10_desc.c

473 ↗

(On Diff #30458)

This should read:

ptr->dev_capability = calloc(ptr->bNumDeviceCapabilities, sizeof(ptr->dev_capability[0]));

According to Linux's LibUSB ???

Make sure this structure is freed.

Be very careful to verify all descriptor lengths.

496 ↗

(On Diff #30458)

From what I can see, this change is not compatible to what libusb from Linux does.

https://github.com/libusb/libusb/blob/master/libusb/descriptor.c#L868

[• hselasky]

lib/libusb/libusb10_desc.c

496 ↗

(On Diff #30458)

Can you show me a link to that parsing code?

[• hselasky]

lib/libusb/libusb10_desc.c

473 ↗

(On Diff #30458)

Correcting myself, dev_capability should be a double pointer, so my example above is not 100% correct.

[kevans]

lib/libusb/libusb10_desc.c

473 ↗

(On Diff #30458)

To address this comment and your new comment further down:

I was attempting to avoid revamping the way we currently read these capabilities for two reasons:

• Make it easier to verify correctness of change, and
• Minimize havoc wreaked upon consumers of our current libusb for the time being

Linux libusb is more flexible than ours; we only allow up to two capabilities, one for USB 2.0 and one for USB SS, and we keep pointers to these in the BOS descriptor and their memory is allocated at the same time as the descriptor itself (See: line 458 just above this, along with the following two lines right here).

These two members aren't easily removed, so for the time being add the new member as a compatibility shim between new style and old way to access these things. I could go ahead and rewrite these bits to allocate new memory for each capability.

[kevans]

lib/libusb/libusb10_desc.c

473 ↗

(On Diff #30458)

Double pointer? =( I missed this, because it's not annotated that way in their header: https://github.com/libusb/libusb/blob/master/libusb/libusb.h#L763

[kevans]

lib/libusb/libusb10_desc.c

496 ↗

(On Diff #30458)

Yeah, so, that was a contrived example based off of https://github.com/libusb/libusb/blob/master/examples/testlibusb.c#L113 and having clearly missed the [0] index.

[• hselasky]

Updated patch.

[• hselasky]

@kevans : No problem. I just uploaded a new diff. Can you test it and report back?

[• hselasky]

@kevans : Use "commander" under action to get control back of this review :-)

[kevans]

Right, that was fairly annoying. =) This looks good to me in accordance with their usage in testlibusb.c, will test.

[kevans]

Don't error out if we don't see bNumDeviceCapabilities, reset it to last index and succeed

[kevans]

This is stupid, but see https://github.com/libusb/libusb/blob/master/libusb/descriptor.c#L877

I had a device that was reporting three capabilities, but only actually advertised 1. Double-checked this behavior, and arrived at...that.

[emaste]

Approved, from hselasky review. (I'm not sufficiently familiar with libusb to give it my Reviewed-by.)