On Wed, May 29, 2019 at 03:13:45PM +0200, Phil Sutter wrote:
> Use of select() after the first call to mnl_socket_recvfrom() was
> incorrect, FD_SET() was called after the call to select() returned. This
> effectively turned the FD_ISSET() check into a noop (always true
> condition).
Good catch.
> Rewrite the receive loop using mnl_nft_event_listener() as an example:
>
> * Combine the two calls to FD_ZERO(), FD_SET() and select() into one at
> loop start.
> * Check ENOBUFS condition and warn the user, also upon other errors.
> * Continue on ENOBUFS, it is not a permanent error.
>
> Signed-off-by: Phil Sutter <phil@xxxxxx>
> ---
> src/mnl.c | 39 ++++++++++++++++++++++-----------------
> 1 file changed, 22 insertions(+), 17 deletions(-)
>
> diff --git a/src/mnl.c b/src/mnl.c
> index 06280aa2cb50a..4fbfd059c0228 100644
> --- a/src/mnl.c
> +++ b/src/mnl.c
> @@ -299,34 +299,39 @@ int mnl_batch_talk(struct netlink_ctx *ctx, struct list_head *err_list)
> if (ret == -1)
> return -1;
>
> - FD_ZERO(&readfds);
> - FD_SET(fd, &readfds);
> + while (true) {
> + FD_ZERO(&readfds);
> + FD_SET(fd, &readfds);
>
> - /* receive and digest all the acknowledgments from the kernel. */
> - ret = select(fd+1, &readfds, NULL, NULL, &tv);
> - if (ret == -1)
> - return -1;
> + /* receive and digest all the acknowledgments from the kernel. */
> + ret = select(fd + 1, &readfds, NULL, NULL, &tv);
> + if (ret < 0)
> + return -1;
>
> - while (ret > 0 && FD_ISSET(fd, &readfds)) {
> - struct nlmsghdr *nlh = (struct nlmsghdr *)rcv_buf;
> + if (!FD_ISSET(fd, &readfds))
> + break;
>
> ret = mnl_socket_recvfrom(nl, rcv_buf, sizeof(rcv_buf));
> - if (ret == -1)
> - return -1;
> + if (ret < 0) {
> + if (errno == ENOBUFS) {
> + nft_print(&ctx->nft->output,
> + "# ERROR: We lost some netlink events!\n");
Probably better handling this from nft_netlink().
Could you just fix the problem you report above? Then, we make another
pass on this ENOBUFS error.
> + continue;
> + }
> + nft_print(&ctx->nft->output, "# ERROR: %s\n",
> + strerror(errno));
> + err = ret;
> + break;
> + }
>
> ret = mnl_cb_run(rcv_buf, ret, 0, portid, &netlink_echo_callback, ctx);
> /* Continue on error, make sure we get all acknowledgments */
> if (ret == -1) {
> + struct nlmsghdr *nlh = (struct nlmsghdr *)rcv_buf;
> +
> mnl_err_list_node_add(err_list, errno, nlh->nlmsg_seq);
> err = -1;
> }
> -
> - ret = select(fd+1, &readfds, NULL, NULL, &tv);
> - if (ret == -1)
> - return -1;
> -
> - FD_ZERO(&readfds);
> - FD_SET(fd, &readfds);
> }
> return err;
> }
> --
> 2.21.0
>
