* [nft PATCH 0/2] Fix and simplify mnl_batch_talk()
@ 2019-05-31 14:17 Phil Sutter
2019-05-31 14:17 ` [nft PATCH 1/2] mnl: Initialize fd_set before select(), not after Phil Sutter
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Phil Sutter @ 2019-05-31 14:17 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: netfilter-devel
As requested, here's the mnl_batch_talk() fix extracted from previous
series. To make things more clear, I've split this into a very minimal
FD_SET/select reordering fix and a follow-up simplifying the code a bit.
Feel free to fold into your own series and/or dismiss second patch at
your own convenience. :)
Phil Sutter (2):
mnl: Initialize fd_set before select(), not after
mnl: Simplify mnl_batch_talk()
src/mnl.c | 26 +++++++++++---------------
1 file changed, 11 insertions(+), 15 deletions(-)
--
2.21.0
^ permalink raw reply [flat|nested] 4+ messages in thread
* [nft PATCH 1/2] mnl: Initialize fd_set before select(), not after
2019-05-31 14:17 [nft PATCH 0/2] Fix and simplify mnl_batch_talk() Phil Sutter
@ 2019-05-31 14:17 ` Phil Sutter
2019-05-31 14:17 ` [nft PATCH 2/2] mnl: Simplify mnl_batch_talk() Phil Sutter
2019-05-31 16:10 ` [nft PATCH 0/2] Fix and simplify mnl_batch_talk() Pablo Neira Ayuso
2 siblings, 0 replies; 4+ messages in thread
From: Phil Sutter @ 2019-05-31 14:17 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: netfilter-devel
Calling FD_SET() in between return of select() and call to FD_ISSET()
effectively renders the whole thing useless: FD_ISSET() will always
return true no matter what select() actually did.
Fixes: a72315d2bad47 ("src: add rule batching support")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
src/mnl.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/mnl.c b/src/mnl.c
index f6363560721c1..492d7517d40e2 100644
--- a/src/mnl.c
+++ b/src/mnl.c
@@ -300,12 +300,12 @@ int mnl_batch_talk(struct netlink_ctx *ctx, struct list_head *err_list)
err = -1;
}
+ FD_ZERO(&readfds);
+ FD_SET(fd, &readfds);
+
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [nft PATCH 2/2] mnl: Simplify mnl_batch_talk()
2019-05-31 14:17 [nft PATCH 0/2] Fix and simplify mnl_batch_talk() Phil Sutter
2019-05-31 14:17 ` [nft PATCH 1/2] mnl: Initialize fd_set before select(), not after Phil Sutter
@ 2019-05-31 14:17 ` Phil Sutter
2019-05-31 16:10 ` [nft PATCH 0/2] Fix and simplify mnl_batch_talk() Pablo Neira Ayuso
2 siblings, 0 replies; 4+ messages in thread
From: Phil Sutter @ 2019-05-31 14:17 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: netfilter-devel
By mimicking mnl_nft_event_listener() code, mnl_batch_talk() may be
simplified quite a bit:
* Turn the conditional loop into an unconditional one.
* Call select() at loop start, which merges the two call sites.
* Check readfds content after select() returned instead of in loop
condition - if fd is not set, break to return error state stored in
'err' variable.
* Old code checked that select() return code is > 0, but that was
redundant: if FD_ISSET() returns true, select return code was 1.
* Move 'nlh' helper variable definition into error handling block, it is
not used outside of it.
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
src/mnl.c | 26 +++++++++++---------------
1 file changed, 11 insertions(+), 15 deletions(-)
diff --git a/src/mnl.c b/src/mnl.c
index 492d7517d40e2..724decad2e44d 100644
--- a/src/mnl.c
+++ b/src/mnl.c
@@ -278,16 +278,17 @@ 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);
-
/* receive and digest all the acknowledgments from the kernel. */
- ret = select(fd+1, &readfds, NULL, NULL, &tv);
- if (ret == -1)
- return -1;
+ while (true) {
+ FD_ZERO(&readfds);
+ FD_SET(fd, &readfds);
+
+ ret = select(fd+1, &readfds, NULL, NULL, &tv);
+ if (ret == -1)
+ 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)
@@ -296,16 +297,11 @@ int mnl_batch_talk(struct netlink_ctx *ctx, struct list_head *err_list)
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;
}
-
- FD_ZERO(&readfds);
- FD_SET(fd, &readfds);
-
- ret = select(fd+1, &readfds, NULL, NULL, &tv);
- if (ret == -1)
- return -1;
}
return err;
}
--
2.21.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [nft PATCH 0/2] Fix and simplify mnl_batch_talk()
2019-05-31 14:17 [nft PATCH 0/2] Fix and simplify mnl_batch_talk() Phil Sutter
2019-05-31 14:17 ` [nft PATCH 1/2] mnl: Initialize fd_set before select(), not after Phil Sutter
2019-05-31 14:17 ` [nft PATCH 2/2] mnl: Simplify mnl_batch_talk() Phil Sutter
@ 2019-05-31 16:10 ` Pablo Neira Ayuso
2 siblings, 0 replies; 4+ messages in thread
From: Pablo Neira Ayuso @ 2019-05-31 16:10 UTC (permalink / raw)
To: Phil Sutter; +Cc: netfilter-devel
On Fri, May 31, 2019 at 04:17:41PM +0200, Phil Sutter wrote:
> As requested, here's the mnl_batch_talk() fix extracted from previous
> series. To make things more clear, I've split this into a very minimal
> FD_SET/select reordering fix and a follow-up simplifying the code a bit.
Series applied, thanks Phil.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-05-31 16:10 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-31 14:17 [nft PATCH 0/2] Fix and simplify mnl_batch_talk() Phil Sutter
2019-05-31 14:17 ` [nft PATCH 1/2] mnl: Initialize fd_set before select(), not after Phil Sutter
2019-05-31 14:17 ` [nft PATCH 2/2] mnl: Simplify mnl_batch_talk() Phil Sutter
2019-05-31 16:10 ` [nft PATCH 0/2] Fix and simplify mnl_batch_talk() Pablo Neira Ayuso
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).