On 01/30/2014 04:30 PM, Peter Stuge wrote:
> Robert Baldyga wrote:
>> v3:
> ..
>> +++ b/tools/usb/aio_multibuff/host_app/Makefile
>> @@ -0,0 +1,13 @@
>> +CC = gcc
>> +LIBUSB_CFLAGS = $(shell pkg-config --cflags libusb-1.0)
>> +LIBUSB_LIBS = $(shell pkg-config --libs libusb-1.0)
>> +WARNINGS = -Wall -Wextra
>> +CFLAGS = $(LIBUSB_CFLAGS) $(WARNINGS)
>> +LDFLAGS = $(LIBUSB_LIBS)
>> +
>> +all: test
>> +%: %.c
>> + $(CC) $(CFLAGS) -o $@ $^ $(LDFLAGS)
>> +
>> +clean:
>> + $(RM) test
>
> Nice!
>
>
>> +++ b/tools/usb/aio_multibuff/host_app/test.c
> ..
>> + cnt = libusb_get_device_list(state->ctx, &list);
>> + if (cnt < 0) {
>> + printf("no devices found\n");
>> + goto error1;
>> + }
> ..
>> +error1:
>> + libusb_free_device_list(list, 1);
>> + libusb_exit(state->ctx);
>> + return 1;
>> +}
>
> The above tries to free uninitialized memory in the error path.
Right, thanks :)
>
>
>> + for (i = 0; i < cnt; ++i) {
>> + libusb_device *dev = list[i];
>> + struct libusb_device_descriptor desc;
>> + if (libusb_get_device_descriptor(dev, &desc)) {
>> + printf("unable to get device descriptor\n");
>> + goto error1;
>> + }
>> + if (desc.idVendor == VENDOR && desc.idProduct == PRODUCT) {
>> + state->found = dev;
>> + break;
>> + }
>> + }
>> +
>> + if (state->found) {
> ...
>> + } else {
>> + printf("no devices found\n");
>> + goto error1;
>> + }
>
> A matter of taste, but I would reverse the if () condition to avoid
> the extra indent level for when a device has been found.
>
> I find that makes it more clear what part of the code handles errors
> and what part is the expected common case.
>
>
> A few other things in the same code:
>
>> + if (state->found) {
>> + printf("found device\n");
>> +
>> + printf("open device: ");
>> + if (libusb_open(state->found, &state->handle)) {
>> + printf("ERROR\n");
>> + goto error1;
>> + }
>> + printf("DONE\n");
>> +
>> + if (libusb_kernel_driver_active(state->handle, 0)) {
>> + printf("device busy.. detaching\n");
>> + if (libusb_detach_kernel_driver(state->handle, 0)) {
>> + printf("unable do deatch device\n");
>
> Typo "deatch"
>
>
>> + goto error2;
>> + }
>> + state->attached = 1;
>> + } else
>> + printf("device free from kernel\n");
>
> This isn't completely accurate, in two ways. First, it's only the
> interface and not the entire device which is claimed/detached.
> Second, it could be either another userspace program (via the
> usbfs kernel driver) or it could be a kernel driver which has
> claimed the interface.
>
> libusb_detach_kernel_driver() makes it so that no kernel driver
> (usbfs or other) is attached to the interface.
>
> libusb_claim_interface() then makes the usbfs kernel driver attach
> the interface.
>
>
>> +
>> + if (libusb_claim_interface(state->handle, 0)) {
>> + printf("failed to claim interface\n");
>> + goto error3;
>> + }
>> + } else {
>
> I still recommend the open/claim/detach logic to be:
>
> libusb_open()
> if (libusb_claim_interface() != LIBUSB_SUCCESS) {
> ret = libusb_detach_kernel_driver();
> if (ret != LIBUSB_SUCCESS) {
> printf("unable to detach kernel driver: %s\n", libusb_error_name(ret));
> goto ..
> }
> ret = libusb_claim_interface();
> if ( != LIBUSB_SUCCESS) {
> printf("cannot claim interface: %s\n", libusb_error_name(ret));
> goto ..
> }
> }
>
> If you prefer to be more conservative then don't use libusb_error_name()
> which was only introduced in late 2011 and released in 2012.
>
Thanks for feedback! I will take it into account in v4.
Best regards
Robert Baldyga
Samsung R&D Institute Poland
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
