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.
> + 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.
//Peter
--
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
