From: Pascal Mazon Date: Fri, 31 Mar 2017 13:54:09 +0000 (+0200) Subject: net/tap: update netlink error code management X-Git-Url: http://git.droids-corp.org/?a=commitdiff_plain;h=6fc6de7e0eaffaa4882ba00dfbc7910d474bed1d;p=dpdk.git net/tap: update netlink error code management 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 --- diff --git a/drivers/net/tap/tap_flow.c b/drivers/net/tap/tap_flow.c index 0973b63d69..cf1c8a26c8 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 +#include #include #include @@ -1169,6 +1171,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; @@ -1210,6 +1215,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"); @@ -1257,7 +1265,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"); @@ -1275,7 +1289,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"); @@ -1390,7 +1409,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 6de896ab17..ee92e2e7ed 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 af1c9aec0d..d74ac805b1 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) @@ -183,34 +187,6 @@ qdisc_add_ingress(int nlsk_fd, uint16_t ifindex) return 0; } -/** - * 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. * @@ -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, @@ -280,36 +253,6 @@ qdisc_iterate(int nlsk_fd, uint16_t ifindex, return 0; } -/** - * 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. * @@ -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 a571a56d69..789595771d 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);