Change in flashrom[master]: usbdev: Refactor device discovery code

[Nico Huber (Code Review)]

Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/27444 )

Change subject: usbdev: Refactor device discovery code
......................................................................

usbdev: Refactor device discovery code

Currently there is a lot of code shared between
usb_dev_get_by_vid_pid_serial() and usb_dev_get_by_vid_pid_number().
Fix this by pulling out the conditional filtering at the heart of each loop
and calling it via a function pointer.

I haven't got (two) dediprog programmers to test with but I have tested
both by...serial() and by...number() calls using a pair of Developerboxen
and a hacked driver.

Change-Id: I31ed572501e4314b9455e1b70a5e934ec96408b1
Signed-off-by: Daniel Thompson <[e-mail]>
Reviewed-on: https://review.coreboot.org/27444
Tested-by: build bot (Jenkins) <[e-mail]>
Reviewed-by: Nico Huber <[e-mail]>
---
M usbdev.c
1 file changed, 70 insertions(+), 58 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Nico Huber: Looks good to me, approved



diff --git a/usbdev.c b/usbdev.c
index 5c34ba1..d793b65 100644
--- a/usbdev.c
+++ b/usbdev.c
@@ -15,12 +15,25 @@
  * GNU General Public License for more details.
  */
 
+#include <stdbool.h>
 #include <string.h>
 #include <libusb.h>
 #include "programmer.h"
 
-struct libusb_device_handle *usb_dev_get_by_vid_pid_serial(
-		struct libusb_context *usb_ctx, uint16_t vid, uint16_t pid, const char *serialno)
+/*
+ * Check whether we should filter the current device.
+ *
+ * The main code filters by VID/PID then calls out to the filter function for extra filtering.
+ * The filter function is called twice for each device. Once with handle == NULL to allow the
+ * filter to cull devices without opening them and, assuming the first filter does not trigger,
+ * also with a real handle to allow the filter to query the device further.
+ *
+ * Returns true if the device should be skipped.
+ */
+typedef bool (*filter_func)(struct libusb_device_descriptor *desc, struct libusb_device_handle *handle, void*ctx);
+
+static struct libusb_device_handle *get_by_vid_pid_filter(struct libusb_context *usb_ctx,
+		uint16_t vid, uint16_t pid, filter_func filter_fn, void *filter_ctx)
 {
 	struct libusb_device **list;
 	ssize_t count = libusb_get_device_list(usb_ctx, &list);
@@ -48,28 +61,22 @@
 			 desc.idVendor, desc.idProduct,
 			 libusb_get_bus_number(dev), libusb_get_device_address(dev));
 
+		/* allow filters to trigger before the device is opened */
+		if (filter_fn(&desc, NULL, filter_ctx))
+			continue;
+
 		res = libusb_open(dev, &handle);
 		if (res != 0) {
-			msg_perr("Opening the USB device failed (%s)!\n", libusb_error_name(res));
-			continue;
+			msg_perr("Opening the USB device at address %d-%d failed (%s)!\n",
+				 libusb_get_bus_number(dev), libusb_get_device_address(dev),
+				 libusb_error_name(res));
+			break;
 		}
 
-		if (serialno) {
-			unsigned char myserial[64];
-			res = libusb_get_string_descriptor_ascii(handle, desc.iSerialNumber, myserial,
-					sizeof(myserial));
-			if (res < 0) {
-				msg_perr("Reading the USB serialno failed (%s)!\n", libusb_error_name(res));
-				libusb_close(handle);
-				continue;
-			}
-			msg_pdbg("Serial number is %s\n", myserial);
-
-			/* Reject any serial number that does not commence with serialno */
-			if (0 != strncmp(serialno, (char *)myserial, strlen(serialno))) {
-				libusb_close(handle);
-				continue;
-			}
+		/* filter can also trigger after a device is opened */
+		if (filter_fn(&desc, handle, filter_ctx)) {
+			libusb_close(handle);
+			continue;
 		}
 
 		libusb_free_device_list(list, 1);
@@ -78,6 +85,48 @@
 
 	libusb_free_device_list(list, 1);
 	return NULL;
+
+}
+
+static bool filter_by_serial(struct libusb_device_descriptor *desc, struct libusb_device_handle *handle,
+			     void *serialno)
+{
+	/* Never filter if device is not yet open or when user did not provide a serial number */
+	if (!handle || !serialno)
+		return false;
+
+	unsigned char myserial[64];
+	int res = libusb_get_string_descriptor_ascii(handle, desc->iSerialNumber, myserial, sizeof(myserial));
+	if (res < 0) {
+		msg_perr("Reading the USB serialno failed (%s)!\n", libusb_error_name(res));
+		return true;
+	}
+	msg_pdbg("Serial number is %s\n", myserial);
+
+	/* Filter out any serial number that does not commence with serialno */
+	return 0 != strncmp(serialno, (char *)myserial, strlen(serialno));
+}
+
+struct libusb_device_handle *usb_dev_get_by_vid_pid_serial(
+		struct libusb_context *usb_ctx, uint16_t vid, uint16_t pid, const char *serialno)
+{
+	return get_by_vid_pid_filter(usb_ctx, vid, pid, filter_by_serial, (void *)serialno);
+}
+
+static bool filter_by_number(struct libusb_device_descriptor *desc, struct libusb_device_handle *handle,
+			     void *ctx)
+{
+	/* This filter never triggers once it has allowed the device to be opened */
+	if (handle != NULL)
+		return false;
+
+	unsigned int *nump = ctx;
+	if (*nump) {
+		(*nump)--;
+		return true;
+	}
+
+	return false;
 }
 
 /*
@@ -89,42 +138,5 @@
 struct libusb_device_handle *usb_dev_get_by_vid_pid_number(
 		struct libusb_context *usb_ctx, uint16_t vid, uint16_t pid, unsigned int num)
 {
-	struct libusb_device **list;
-	ssize_t count = libusb_get_device_list(usb_ctx, &list);
-	if (count < 0) {
-		msg_perr("Getting the USB device list failed (%s)!\n", libusb_error_name(count));
-		return NULL;
-	}
-
-	struct libusb_device_handle *handle = NULL;
-	ssize_t i = 0;
-	for (i = 0; i < count; i++) {
-		struct libusb_device *dev = list[i];
-		struct libusb_device_descriptor desc;
-		int err = libusb_get_device_descriptor(dev, &desc);
-		if (err != 0) {
-			msg_perr("Reading the USB device descriptor failed (%s)!\n", libusb_error_name(err));
-			libusb_free_device_list(list, 1);
-			return NULL;
-		}
-		if ((desc.idVendor == vid) && (desc.idProduct == pid)) {
-			msg_pdbg("Found USB device %04"PRIx16":%04"PRIx16" at address %d-%d.\n",
-				 desc.idVendor, desc.idProduct,
-				 libusb_get_bus_number(dev), libusb_get_device_address(dev));
-			if (num == 0) {
-				err = libusb_open(dev, &handle);
-				if (err != 0) {
-					msg_perr("Opening the USB device failed (%s)!\n",
-						 libusb_error_name(err));
-					libusb_free_device_list(list, 1);
-					return NULL;
-				}
-				break;
-			}
-			num--;
-		}
-	}
-	libusb_free_device_list(list, 1);
-
-	return handle;
+	return get_by_vid_pid_filter(usb_ctx, vid, pid, filter_by_number, &num);
 }

-- 
To view, visit https://review.coreboot.org/27444
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I31ed572501e4314b9455e1b70a5e934ec96408b1
Gerrit-Change-Number: 27444
Gerrit-PatchSet: 5
Gerrit-Owner: Daniel Thompson <[e-mail]>
Gerrit-Reviewer: Daniel Thompson <[e-mail]>
Gerrit-Reviewer: Nico Huber <[e-mail]>
Gerrit-Reviewer: Paul Menzel <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>