linux_netlink: Add useful debug messages and clean up error handling

Also addresses some minor whitespace and stylistic issues.

Signed-off-by: Chris Dickens <christopher.a.dickens@gmail.com>

2 files changed, 91 insertions, 79 deletions

diff --git a/libusb/os/linux_netlink.c b/libusb/os/linux_netlink.c
index 5ad1e62..501a94d 100644
--- a/libusb/os/linux_netlink.c
+++ b/libusb/os/linux_netlink.c
@@ -22,14 +22,14 @@
 
 #include <config.h>
 
-#include <ctype.h>
-#include <dirent.h>
+#include <assert.h>
 #include <errno.h>
 #include <fcntl.h>
 #include <poll.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <unistd.h>
 #include <sys/types.h>
 
 #ifdef HAVE_ASM_TYPES_H
@@ -50,7 +50,7 @@ static pthread_t libusb_linux_event_thread;
 
 static void *linux_netlink_event_thread_main(void *arg);
 
-static struct sockaddr_nl snl = { .nl_family=AF_NETLINK, .nl_groups=KERNEL };
+static struct sockaddr_nl snl = { .nl_family = AF_NETLINK, .nl_groups = KERNEL };
 
 static int set_fd_cloexec_nb(int fd)
 {
@@ -58,22 +58,30 @@ static int set_fd_cloexec_nb(int fd)
 
 #if defined(FD_CLOEXEC)
 	flags = fcntl(fd, F_GETFD);
-	if (0 > flags) {
+	if (flags == -1) {
+		usbi_err(NULL, "failed to get netlink fd flags (%d)", errno);
 		return -1;
 	}
 
 	if (!(flags & FD_CLOEXEC)) {
-		fcntl(fd, F_SETFD, flags | FD_CLOEXEC);
+		if (fcntl(fd, F_SETFD, flags | FD_CLOEXEC) == -1) {
+			usbi_err(NULL, "failed to set netlink fd flags (%d)", errno);
+			return -1;
+		}
 	}
 #endif
 
 	flags = fcntl(fd, F_GETFL);
-	if (0 > flags) {
+	if (flags == -1) {
+		usbi_err(NULL, "failed to get netlink fd status flags (%d)", errno);
 		return -1;
 	}
 
 	if (!(flags & O_NONBLOCK)) {
-		fcntl(fd, F_SETFL, flags | O_NONBLOCK);
+		if (fcntl(fd, F_SETFL, flags | O_NONBLOCK) == -1) {
+			usbi_err(NULL, "failed to set netlink fd status flags (%d)", errno);
+			return -1;
+		}
 	}
 
 	return 0;
@@ -84,8 +92,6 @@ int linux_netlink_start_event_monitor(void)
 	int socktype = SOCK_RAW;
 	int ret;
 
-	snl.nl_groups = KERNEL;
-
 #if defined(SOCK_CLOEXEC)
 	socktype |= SOCK_CLOEXEC;
 #endif
@@ -94,25 +100,24 @@ int linux_netlink_start_event_monitor(void)
 #endif
 
 	linux_netlink_socket = socket(PF_NETLINK, socktype, NETLINK_KOBJECT_UEVENT);
-	if (-1 == linux_netlink_socket && EINVAL == errno) {
+	if (linux_netlink_socket == -1 && errno == EINVAL) {
+		usbi_dbg("failed to create netlink socket of type %d, attempting SOCK_RAW", socktype);
 		linux_netlink_socket = socket(PF_NETLINK, SOCK_RAW, NETLINK_KOBJECT_UEVENT);
 	}
 
-	if (-1 == linux_netlink_socket) {
-		return LIBUSB_ERROR_OTHER;
+	if (linux_netlink_socket == -1) {
+		usbi_err(NULL, "failed to create netlink socket (%d)", errno);
+		goto err;
 	}
 
-	ret = set_fd_cloexec_nb (linux_netlink_socket);
-	if (0 != ret) {
-		close (linux_netlink_socket);
-		linux_netlink_socket = -1;
-		return LIBUSB_ERROR_OTHER;
-	}
+	ret = set_fd_cloexec_nb(linux_netlink_socket);
+	if (ret == -1)
+		goto err_close_socket;
 
-	ret = bind(linux_netlink_socket, (struct sockaddr *) &snl, sizeof(snl));
-	if (0 != ret) {
-	        close(linux_netlink_socket);
-		return LIBUSB_ERROR_OTHER;
+	ret = bind(linux_netlink_socket, (struct sockaddr *)&snl, sizeof(snl));
+	if (ret == -1) {
+		usbi_err(NULL, "failed to bind netlink socket (%d)", errno);
+		goto err_close_socket;
 	}
 
 	/* TODO -- add authentication */
@@ -120,38 +125,43 @@ int linux_netlink_start_event_monitor(void)
 
 	ret = usbi_pipe(netlink_control_pipe);
 	if (ret) {
-		usbi_err(NULL, "could not create netlink control pipe");
-	        close(linux_netlink_socket);
-		return LIBUSB_ERROR_OTHER;
+		usbi_err(NULL, "failed to create netlink control pipe");
+		goto err_close_socket;
 	}
 
 	ret = pthread_create(&libusb_linux_event_thread, NULL, linux_netlink_event_thread_main, NULL);
-	if (0 != ret) {
-        	close(netlink_control_pipe[0]);
-        	close(netlink_control_pipe[1]);
-	        close(linux_netlink_socket);
-		return LIBUSB_ERROR_OTHER;
+	if (ret != 0) {
+		usbi_err(NULL, "failed to create netlink event thread (%d)", ret);
+		goto err_close_pipe;
 	}
 
 	return LIBUSB_SUCCESS;
+
+err_close_pipe:
+	close(netlink_control_pipe[0]);
+	close(netlink_control_pipe[1]);
+	netlink_control_pipe[0] = -1;
+	netlink_control_pipe[1] = -1;
+err_close_socket:
+	close(linux_netlink_socket);
+	linux_netlink_socket = -1;
+err:
+	return LIBUSB_ERROR_OTHER;
 }
 
 int linux_netlink_stop_event_monitor(void)
 {
-	int r;
 	char dummy = 1;
+	ssize_t r;
 
-	if (-1 == linux_netlink_socket) {
-		/* already closed. nothing to do */
-		return LIBUSB_SUCCESS;
-	}
+	assert(linux_netlink_socket != -1);
 
 	/* Write some dummy data to the control pipe and
 	 * wait for the thread to exit */
 	r = usbi_write(netlink_control_pipe[1], &dummy, sizeof(dummy));
-	if (r <= 0) {
+	if (r <= 0)
 		usbi_warn(NULL, "netlink control pipe signal failed");
-	}
+
 	pthread_join(libusb_linux_event_thread, NULL);
 
 	close(linux_netlink_socket);
@@ -166,14 +176,14 @@ int linux_netlink_stop_event_monitor(void)
 	return LIBUSB_SUCCESS;
 }
 
-static const char *netlink_message_parse (const char *buffer, size_t len, const char *key)
+static const char *netlink_message_parse(const char *buffer, size_t len, const char *key)
 {
 	size_t keylen = strlen(key);
 	size_t offset;
 
-	for (offset = 0 ; offset < len && '\0' != buffer[offset] ; offset += strlen(buffer + offset) + 1) {
-		if (0 == strncmp(buffer + offset, key, keylen) &&
-		    '=' == buffer[offset + keylen]) {
+	for (offset = 0; offset < len && buffer[offset] != '\0'; offset += strlen(buffer + offset) + 1) {
+		if (strncmp(buffer + offset, key, keylen) == 0 &&
+			buffer[offset + keylen] == '=') {
 			return buffer + offset + keylen + 1;
 		}
 	}
@@ -182,8 +192,9 @@ static const char *netlink_message_parse (const char *buffer, size_t len, const
 }
 
 /* parse parts of netlink message common to both libudev and the kernel */
-static int linux_netlink_parse(char *buffer, size_t len, int *detached, const char **sys_name,
-			       uint8_t *busnum, uint8_t *devaddr) {
+static int linux_netlink_parse(const char *buffer, size_t len, int *detached,
+	const char **sys_name, uint8_t *busnum, uint8_t *devaddr)
+{
 	const char *tmp;
 	int i;
 
@@ -194,57 +205,56 @@ static int linux_netlink_parse(char *buffer, size_t len, int *detached, const ch
 	*busnum   = 0;
 	*devaddr  = 0;
 
-	tmp = netlink_message_parse((const char *) buffer, len, "ACTION");
-	if (tmp == NULL)
+	tmp = netlink_message_parse(buffer, len, "ACTION");
+	if (!tmp) {
 		return -1;
-	if (0 == strcmp(tmp, "remove")) {
+	} else if (strcmp(tmp, "remove") == 0) {
 		*detached = 1;
-	} else if (0 != strcmp(tmp, "add")) {
+	} else if (strcmp(tmp, "add") != 0) {
 		usbi_dbg("unknown device action %s", tmp);
 		return -1;
 	}
 
 	/* check that this is a usb message */
 	tmp = netlink_message_parse(buffer, len, "SUBSYSTEM");
-	if (NULL == tmp || 0 != strcmp(tmp, "usb")) {
+	if (!tmp || strcmp(tmp, "usb") != 0) {
 		/* not usb. ignore */
 		return -1;
 	}
 
 	/* check that this is an actual usb device */
 	tmp = netlink_message_parse(buffer, len, "DEVTYPE");
-	if (NULL == tmp || 0 != strcmp(tmp, "usb_device")) {
+	if (!tmp || strcmp(tmp, "usb_device") != 0) {
 		/* not usb. ignore */
 		return -1;
 	}
 
 	tmp = netlink_message_parse(buffer, len, "BUSNUM");
-	if (NULL == tmp) {
+	if (!tmp) {
 		/* no bus number. try "DEVICE" */
 		tmp = netlink_message_parse(buffer, len, "DEVICE");
-		if (NULL == tmp) {
+		if (!tmp) {
 			/* not usb. ignore */
 			return -1;
 		}
-		
+
 		/* Parse a device path such as /dev/bus/usb/003/004 */
-		char *pLastSlash = (char*)strrchr(tmp,'/');
-		if(NULL == pLastSlash) {
+		const char *pLastSlash = strrchr(tmp, '/');
+		if (!pLastSlash)
 			return -1;
-		}
 
-		*devaddr = strtoul(pLastSlash + 1, NULL, 10);
+		*busnum = (uint8_t)(strtoul(pLastSlash - 3, NULL, 10) & 0xff);
 		if (errno) {
 			errno = 0;
 			return -1;
 		}
-		
-		*busnum = strtoul(pLastSlash - 3, NULL, 10);
+
+		*devaddr = (uint8_t)(strtoul(pLastSlash + 1, NULL, 10) & 0xff);
 		if (errno) {
 			errno = 0;
 			return -1;
 		}
-		
+
 		return 0;
 	}
 
@@ -255,9 +265,8 @@ static int linux_netlink_parse(char *buffer, size_t len, int *detached, const ch
 	}
 
 	tmp = netlink_message_parse(buffer, len, "DEVNUM");
-	if (NULL == tmp) {
+	if (!tmp)
 		return -1;
-	}
 
 	*devaddr = (uint8_t)(strtoul(tmp, NULL, 10) & 0xff);
 	if (errno) {
@@ -266,12 +275,11 @@ static int linux_netlink_parse(char *buffer, size_t len, int *detached, const ch
 	}
 
 	tmp = netlink_message_parse(buffer, len, "DEVPATH");
-	if (NULL == tmp) {
+	if (!tmp)
 		return -1;
-	}
 
-	for (i = strlen(tmp) - 1 ; i ; --i) {
-		if ('/' ==tmp[i]) {
+	for (i = strlen(tmp) - 1; i; --i) {
+		if (tmp[i] == '/') {
 			*sys_name = tmp + i + 1;
 			break;
 		}
@@ -283,14 +291,16 @@ static int linux_netlink_parse(char *buffer, size_t len, int *detached, const ch
 
 static int linux_netlink_read_message(void)
 {
-	unsigned char buffer[1024];
-	struct iovec iov = {.iov_base = buffer, .iov_len = sizeof(buffer)};
-	struct msghdr meh = { .msg_iov=&iov, .msg_iovlen=1,
-			     .msg_name=&snl, .msg_namelen=sizeof(snl) };
+	char buffer[1024];
 	const char *sys_name = NULL;
 	uint8_t busnum, devaddr;
 	int detached, r;
-	size_t len;
+	ssize_t len;
+	struct iovec iov = { .iov_base = buffer, .iov_len = sizeof(buffer) };
+	struct msghdr meh = {
+		.msg_iov = &iov, .msg_iovlen = 1,
+		.msg_name = &snl, .msg_namelen = sizeof(snl)
+	};
 
 	/* read netlink message */
 	memset(buffer, 0, sizeof(buffer));
@@ -303,8 +313,7 @@ static int linux_netlink_read_message(void)
 
 	/* TODO -- authenticate this message is from the kernel or udevd */
 
-	r = linux_netlink_parse(buffer, len, &detached, &sys_name,
-				&busnum, &devaddr);
+	r = linux_netlink_parse(buffer, (size_t)len, &detached, &sys_name, &busnum, &devaddr);
 	if (r)
 		return r;
 
@@ -323,7 +332,7 @@ static int linux_netlink_read_message(void)
 static void *linux_netlink_event_thread_main(void *arg)
 {
 	char dummy;
-	int r;
+	ssize_t r;
 	struct pollfd fds[] = {
 		{ .fd = netlink_control_pipe[0],
 		  .events = POLLIN },
@@ -333,22 +342,25 @@ static void *linux_netlink_event_thread_main(void *arg)
 
 	UNUSED(arg);
 
+	usbi_dbg("netlink event thread entering");
+
 	while (poll(fds, 2, -1) >= 0) {
 		if (fds[0].revents & POLLIN) {
 			/* activity on control pipe, read the byte and exit */
 			r = usbi_read(netlink_control_pipe[0], &dummy, sizeof(dummy));
-			if (r <= 0) {
+			if (r <= 0)
 				usbi_warn(NULL, "netlink control pipe read failed");
-			}
 			break;
 		}
 		if (fds[1].revents & POLLIN) {
-        		usbi_mutex_static_lock(&linux_hotplug_lock);
-	        	linux_netlink_read_message();
-	        	usbi_mutex_static_unlock(&linux_hotplug_lock);
+			usbi_mutex_static_lock(&linux_hotplug_lock);
+			linux_netlink_read_message();
+			usbi_mutex_static_unlock(&linux_hotplug_lock);
 		}
 	}
 
+	usbi_dbg("netlink event thread exiting");
+
 	return NULL;
 }
 
diff --git a/libusb/version_nano.h b/libusb/version_nano.h
index b7a0eb1..f055648 100644
--- a/libusb/version_nano.h
+++ b/libusb/version_nano.h
@@ -1 +1 @@
-#define LIBUSB_NANO 11050
+#define LIBUSB_NANO 11051