[PATCH 1/2] net/tap: update netlink error code management

* [PATCH 1/2] net/tap: update netlink error code management
@ 2017-03-29  9:44 Pascal Mazon
  2017-03-29  9:44 ` [PATCH 2/2] net/tap: update redirection rule after MAC change Pascal Mazon
  0 siblings, 1 reply; 9+ messages in thread
From: Pascal Mazon @ 2017-03-29  9:44 UTC (permalink / raw)
  To: keith.wiles; +Cc: dev, Pascal Mazon

Some errors received from the kernel are acceptable, such as a -ENOENT
for a rule deletion (the rule was already no longer existing in the
kernel). Make sure we consider return codes properly. For that,
nl_recv() has been simplified.

qdisc_exists() function is no longer needed as we can check whether the
kernel returned -EEXIST when requiring the qdisc creation. It's simpler
and faster.

Add a few messages for clarity when a netlink error occurs.

Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
---
 drivers/net/tap/tap_flow.c    |  22 ++++++++-
 drivers/net/tap/tap_netlink.c |  73 +++++++++++++---------------
 drivers/net/tap/tap_tcmsgs.c  | 107 ++++++++++--------------------------------
 drivers/net/tap/tap_tcmsgs.h  |   2 -
 4 files changed, 80 insertions(+), 124 deletions(-)

diff --git a/drivers/net/tap/tap_flow.c b/drivers/net/tap/tap_flow.c
index 7f1693d40468..514e3fae5c38 100644
--- a/drivers/net/tap/tap_flow.c
+++ b/drivers/net/tap/tap_flow.c
@@ -31,6 +31,8 @@
  *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
+#include <errno.h>
+#include <string.h>
 #include <sys/queue.h>
 
 #include <rte_byteorder.h>
@@ -1165,6 +1167,9 @@ tap_flow_create(struct rte_eth_dev *dev,
 	}
 	err = nl_recv_ack(pmd->nlsk_fd);
 	if (err < 0) {
+		RTE_LOG(ERR, PMD,
+			"Kernel refused TC filter rule creation (%d): %s\n",
+			errno, strerror(errno));
 		rte_flow_error_set(error, EEXIST, RTE_FLOW_ERROR_TYPE_HANDLE,
 				   NULL, "overlapping rules");
 		goto fail;
@@ -1206,6 +1211,9 @@ tap_flow_create(struct rte_eth_dev *dev,
 		}
 		err = nl_recv_ack(pmd->nlsk_fd);
 		if (err < 0) {
+			RTE_LOG(ERR, PMD,
+				"Kernel refused TC filter rule creation (%d): %s\n",
+				errno, strerror(errno));
 			rte_flow_error_set(
 				error, ENOMEM, RTE_FLOW_ERROR_TYPE_HANDLE,
 				NULL, "overlapping rules");
@@ -1253,7 +1261,13 @@ tap_flow_destroy_pmd(struct pmd_internals *pmd,
 		goto end;
 	}
 	ret = nl_recv_ack(pmd->nlsk_fd);
+	/* If errno is ENOENT, the rule is already no longer in the kernel. */
+	if (ret < 0 && errno == ENOENT)
+		ret = 0;
 	if (ret < 0) {
+		RTE_LOG(ERR, PMD,
+			"Kernel refused TC filter rule deletion (%d): %s\n",
+			errno, strerror(errno));
 		rte_flow_error_set(
 			error, ENOTSUP, RTE_FLOW_ERROR_TYPE_HANDLE, NULL,
 			"couldn't receive kernel ack to our request");
@@ -1271,7 +1285,12 @@ tap_flow_destroy_pmd(struct pmd_internals *pmd,
 			goto end;
 		}
 		ret = nl_recv_ack(pmd->nlsk_fd);
+		if (ret < 0 && errno == ENOENT)
+			ret = 0;
 		if (ret < 0) {
+			RTE_LOG(ERR, PMD,
+				"Kernel refused TC filter rule deletion (%d): %s\n",
+				errno, strerror(errno));
 			rte_flow_error_set(
 				error, ENOMEM, RTE_FLOW_ERROR_TYPE_HANDLE,
 				NULL, "Failure trying to receive nl ack");
@@ -1386,7 +1405,8 @@ int tap_flow_implicit_create(struct pmd_internals *pmd,
 	err = nl_recv_ack(pmd->nlsk_fd);
 	if (err < 0) {
 		RTE_LOG(ERR, PMD,
-			"Kernel refused TC filter rule creation");
+			"Kernel refused TC filter rule creation (%d): %s\n",
+			errno, strerror(errno));
 		goto fail;
 	}
 	LIST_INSERT_HEAD(&pmd->implicit_flows, remote_flow, next);
diff --git a/drivers/net/tap/tap_netlink.c b/drivers/net/tap/tap_netlink.c
index 6de896ab17b6..ee92e2e7ed13 100644
--- a/drivers/net/tap/tap_netlink.c
+++ b/drivers/net/tap/tap_netlink.c
@@ -159,7 +159,7 @@ nl_send(int nlsk_fd, struct nlmsghdr *nh)
  *   The netlink socket file descriptor used for communication.
  *
  * @return
- *   0 on success, -1 otherwise.
+ *   0 on success, -1 otherwise with errno set.
  */
 int
 nl_recv_ack(int nlsk_fd)
@@ -179,14 +179,13 @@ nl_recv_ack(int nlsk_fd)
  *   Custom arguments for the callback.
  *
  * @return
- *   0 on success, -1 otherwise.
+ *   0 on success, -1 otherwise with errno set.
  */
 int
 nl_recv(int nlsk_fd, int (*cb)(struct nlmsghdr *, void *arg), void *arg)
 {
 	/* man 7 netlink EXAMPLE */
 	struct sockaddr_nl sa;
-	struct nlmsghdr *nh;
 	char buf[BUF_SIZE];
 	struct iovec iov = {
 		.iov_base = buf,
@@ -196,49 +195,43 @@ nl_recv(int nlsk_fd, int (*cb)(struct nlmsghdr *, void *arg), void *arg)
 		.msg_name = &sa,
 		.msg_namelen = sizeof(sa),
 		.msg_iov = &iov,
+		/* One message at a time */
 		.msg_iovlen = 1,
 	};
-	int recv_bytes = 0, done = 0, multipart = 0, error = 0;
+	int multipart = 0;
+	int ret = 0;
 
-read:
-	recv_bytes = recvmsg(nlsk_fd, &msg, 0);
-	if (recv_bytes < 0)
-		return -1;
-	for (nh = (struct nlmsghdr *)buf;
-	     NLMSG_OK(nh, (unsigned int)recv_bytes);
-	     nh = NLMSG_NEXT(nh, recv_bytes)) {
-		/*
-		 * Multi-part messages and their following DONE message have the
-		 * NLM_F_MULTI flag set. Make note, in order to read the DONE
-		 * message afterwards.
-		 */
-		if (nh->nlmsg_flags & NLM_F_MULTI)
-			multipart = 1;
-		if (nh->nlmsg_type == NLMSG_ERROR) {
-			struct nlmsgerr *err_data = NLMSG_DATA(nh);
+	do {
+		struct nlmsghdr *nh;
+		int recv_bytes = 0;
+
+		recv_bytes = recvmsg(nlsk_fd, &msg, 0);
+		if (recv_bytes < 0)
+			return -1;
+		for (nh = (struct nlmsghdr *)buf;
+		     NLMSG_OK(nh, (unsigned int)recv_bytes);
+		     nh = NLMSG_NEXT(nh, recv_bytes)) {
+			if (nh->nlmsg_type == NLMSG_ERROR) {
+				struct nlmsgerr *err_data = NLMSG_DATA(nh);
 
-			if (err_data->error == 0)
-				RTE_LOG(DEBUG, PMD, "%s() ack message recvd\n",
-					__func__);
-			else {
-				RTE_LOG(DEBUG, PMD,
-					"%s() error message recvd\n", __func__);
-				error = 1;
+				if (err_data->error < 0) {
+					errno = -err_data->error;
+					return -1;
+				}
+				/* Ack message. */
+				return 0;
 			}
+			/* Multi-part msgs and their trailing DONE message. */
+			if (nh->nlmsg_flags & NLM_F_MULTI) {
+				if (nh->nlmsg_type == NLMSG_DONE)
+					return 0;
+				multipart = 1;
+			}
+			if (cb)
+				ret = cb(nh, arg);
 		}
-		/* The end of multipart message. */
-		if (nh->nlmsg_type == NLMSG_DONE)
-			/* No need to call the callback for a DONE message. */
-			done = 1;
-		else if (cb)
-			if (cb(nh, arg) < 0)
-				error = 1;
-	}
-	if (multipart && !done)
-		goto read;
-	if (error)
-		return -1;
-	return 0;
+	} while (multipart);
+	return ret;
 }
 
 /**
diff --git a/drivers/net/tap/tap_tcmsgs.c b/drivers/net/tap/tap_tcmsgs.c
index af1c9aec0d22..d74ac805b184 100644
--- a/drivers/net/tap/tap_tcmsgs.c
+++ b/drivers/net/tap/tap_tcmsgs.c
@@ -94,7 +94,7 @@ tc_init_msg(struct nlmsg *msg, uint16_t ifindex, uint16_t type, uint16_t flags)
  *   Additional info to identify the QDISC (handle and parent).
  *
  * @return
- *   0 on success, -1 otherwise.
+ *   0 on success, -1 otherwise with errno set.
  */
 static int
 qdisc_del(int nlsk_fd, uint16_t ifindex, struct qdisc *qinfo)
@@ -117,12 +117,16 @@ qdisc_del(int nlsk_fd, uint16_t ifindex, struct qdisc *qinfo)
 		fd = nlsk_fd;
 	}
 	if (nl_send(fd, &msg.nh) < 0)
-		return -1;
+		goto error;
 	if (nl_recv_ack(fd) < 0)
-		return -1;
+		goto error;
 	if (!nlsk_fd)
 		return nl_final(fd);
 	return 0;
+error:
+	if (!nlsk_fd)
+		nl_final(fd);
+	return -1;
 }
 
 /**
@@ -134,7 +138,7 @@ qdisc_del(int nlsk_fd, uint16_t ifindex, struct qdisc *qinfo)
  *   The netdevice ifindex where to add the multiqueue QDISC.
  *
  * @return
- *   -1 if the qdisc cannot be added, and 0 otherwise.
+ *   0 on success, -1 otherwise with errno set.
  */
 int
 qdisc_add_multiq(int nlsk_fd, uint16_t ifindex)
@@ -164,7 +168,7 @@ qdisc_add_multiq(int nlsk_fd, uint16_t ifindex)
  *   The netdevice ifindex where the QDISC will be added.
  *
  * @return
- *   -1 if the qdisc cannot be added, and 0 otherwise.
+ *   0 on success, -1 otherwise with errno set.
  */
 int
 qdisc_add_ingress(int nlsk_fd, uint16_t ifindex)
@@ -184,34 +188,6 @@ qdisc_add_ingress(int nlsk_fd, uint16_t ifindex)
 }
 
 /**
- * Callback function to check for QDISC existence.
- * If the QDISC is found to exist, increment "exists" in the custom arg.
- *
- * @param[in] nh
- *   The netlink message to parse, received from the kernel.
- * @param[in, out] arg
- *   Custom arguments for the callback.
- *
- * @return
- *   0.
- */
-static int
-qdisc_exist_cb(struct nlmsghdr *nh, void *arg)
-{
-	struct list_args *args = (struct list_args *)arg;
-	struct qdisc_custom_arg *custom = args->custom_arg;
-	struct tcmsg *t = NLMSG_DATA(nh);
-
-	/* filter by request iface */
-	if (args->ifindex != (unsigned int)t->tcm_ifindex)
-		return 0;
-	if (t->tcm_handle != custom->handle || t->tcm_parent != custom->parent)
-		return 0;
-	custom->exists++;
-	return 0;
-}
-
-/**
  * Callback function to delete a QDISC.
  *
  * @param[in] nh
@@ -220,7 +196,7 @@ qdisc_exist_cb(struct nlmsghdr *nh, void *arg)
  *   Custom arguments for the callback.
  *
  * @return
- *   0.
+ *   0 on success, -1 otherwise with errno set.
  */
 static int
 qdisc_del_cb(struct nlmsghdr *nh, void *arg)
@@ -256,10 +232,7 @@ qdisc_del_cb(struct nlmsghdr *nh, void *arg)
  *   The arguments to provide the callback function with.
  *
  * @return
- *   -1 if either sending the netlink message failed, or if receiving the answer
- *   failed, or finally if the callback returned a negative value for that
- *   answer.
- *   0 is returned otherwise.
+ *   0 on success, -1 otherwise with errno set.
  */
 static int
 qdisc_iterate(int nlsk_fd, uint16_t ifindex,
@@ -281,36 +254,6 @@ qdisc_iterate(int nlsk_fd, uint16_t ifindex,
 }
 
 /**
- * Check whether a given QDISC already exists for the netdevice.
- *
- * @param[in] nlsk_fd
- *   The netlink socket file descriptor used for communication.
- * @param[in] ifindex
- *   The netdevice ifindex to check QDISC existence for.
- * @param[in] callback
- *   The function to call for each QDISC.
- * @param[in, out] arg
- *   The arguments to provide the callback function with.
- *
- * @return
- *   1 if the qdisc exists, 0 otherwise.
- */
-int
-qdisc_exists(int nlsk_fd, uint16_t ifindex, uint32_t handle, uint32_t parent)
-{
-	struct qdisc_custom_arg arg = {
-		.handle = handle,
-		.parent = parent,
-		.exists = 0,
-	};
-
-	qdisc_iterate(nlsk_fd, ifindex, qdisc_exist_cb, &arg);
-	if (arg.exists)
-		return 1;
-	return 0;
-}
-
-/**
  * Delete all QDISCs for a given netdevice.
  *
  * @param[in] nlsk_fd
@@ -319,7 +262,7 @@ qdisc_exists(int nlsk_fd, uint16_t ifindex, uint32_t handle, uint32_t parent)
  *   The netdevice ifindex where to find QDISCs.
  *
  * @return
- *   -1 if the lookup failed, 0 otherwise.
+ *   0 on success, -1 otherwise with errno set.
  */
 int
 qdisc_flush(int nlsk_fd, uint16_t ifindex)
@@ -342,12 +285,13 @@ qdisc_flush(int nlsk_fd, uint16_t ifindex)
 int
 qdisc_create_multiq(int nlsk_fd, uint16_t ifindex)
 {
-	if (!qdisc_exists(nlsk_fd, ifindex,
-			  TC_H_MAKE(MULTIQ_MAJOR_HANDLE, 0), TC_H_ROOT)) {
-		if (qdisc_add_multiq(nlsk_fd, ifindex) < 0) {
-			RTE_LOG(ERR, PMD, "Could not add multiq qdisc\n");
-			return -1;
-		}
+	int err = 0;
+
+	err = qdisc_add_multiq(nlsk_fd, ifindex);
+	if (err < 0 && errno != -EEXIST) {
+		RTE_LOG(ERR, PMD, "Could not add multiq qdisc (%d): %s\n",
+			errno, strerror(errno));
+		return -1;
 	}
 	return 0;
 }
@@ -367,12 +311,13 @@ qdisc_create_multiq(int nlsk_fd, uint16_t ifindex)
 int
 qdisc_create_ingress(int nlsk_fd, uint16_t ifindex)
 {
-	if (!qdisc_exists(nlsk_fd, ifindex,
-			  TC_H_MAKE(TC_H_INGRESS, 0), TC_H_INGRESS)) {
-		if (qdisc_add_ingress(nlsk_fd, ifindex) < 0) {
-			RTE_LOG(ERR, PMD, "Could not add ingress qdisc\n");
-			return -1;
-		}
+	int err = 0;
+
+	err = qdisc_add_ingress(nlsk_fd, ifindex);
+	if (err < 0 && errno != -EEXIST) {
+		RTE_LOG(ERR, PMD, "Could not add ingress qdisc (%d): %s\n",
+			errno, strerror(errno));
+		return -1;
 	}
 	return 0;
 }
diff --git a/drivers/net/tap/tap_tcmsgs.h b/drivers/net/tap/tap_tcmsgs.h
index a571a56d6964..789595771d63 100644
--- a/drivers/net/tap/tap_tcmsgs.h
+++ b/drivers/net/tap/tap_tcmsgs.h
@@ -50,8 +50,6 @@
 
 void tc_init_msg(struct nlmsg *msg, uint16_t ifindex, uint16_t type,
 		 uint16_t flags);
-int qdisc_exists(int nlsk_fd, uint16_t ifindex, uint32_t handle,
-		 uint32_t parent);
 int qdisc_list(int nlsk_fd, uint16_t ifindex);
 int qdisc_flush(int nlsk_fd, uint16_t ifindex);
 int qdisc_create_ingress(int nlsk_fd, uint16_t ifindex);
-- 
2.12.0.306.g4a9b9b3

^ permalink raw reply related	[flat|nested] 9+ messages in thread