From: Tobias Brunner <tobias@strongswan.org>
Date: Tue, 18 Apr 2023 16:18:48 +0000 (+0200)
Subject: watcher: Prevent busy wait if callback is active and other FDs have events
X-Git-Tag: 5.9.11rc1~22
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=30803f90eb3721db178822bca203fb14696cfced;p=thirdparty%2Fstrongswan.git

watcher: Prevent busy wait if callback is active and other FDs have events

Exiting the loop previously could cause watcher to busy wait (i.e.
rebuild the array and call poll() repeatedly) until the active callback
was done.

Assume watcher observes two FDs 15 and 22, which are in the list in that
order.  FD 15 is signaled and its callback gets triggered.  The array of
FDs is rebuilt and does not include 15 anymore.  Now FD 22 is ready for
reading.  However, when enumerating all registered FDs, the loop previously
was exited when reaching FD 15 and seeing that it's active.  FD 22 was
never checked and the array was immediately rebuilt and poll() called.
If the callback for 15 took longer, this was repeated over and over.

This basically reverts d16d5a245f0b ("watcher: Avoid queueing multiple
watcher callbacks at the same time"), whose goal is quite unclear to me.
If it really wanted to allow only a single callback for all FDs, it didn't
achieve that as any FD before an active one would get notified and if
multiple FDs are ready concurrently, they'd all get triggered too.
Skipping entries with active callback makes sense as it avoids a lookup
in the FD array and subsequent revents checks.  But why would we need to
rebuild the array if we see such an entry?  Once the callback is done,
the watcher is notified and the array rebuilt anyway (also if any other
FD was ready and jobs get queued).
---

diff --git a/src/libstrongswan/processing/watcher.c b/src/libstrongswan/processing/watcher.c
index 64efa824a9..e174858ef6 100644
--- a/src/libstrongswan/processing/watcher.c
+++ b/src/libstrongswan/processing/watcher.c
@@ -388,7 +388,6 @@ static job_requeue_t watch(private_watcher_t *this)
 	entry_t *entry;
 	struct pollfd *pfd;
 	int count = 0, res;
-	bool rebuild = FALSE;
 #if DEBUG_LEVEL >= 2
 	char logbuf[BUF_LEN], *logpos, eventbuf[4], *eventpos;
 	int loglen;
@@ -451,7 +450,7 @@ static job_requeue_t watch(private_watcher_t *this)
 	}
 #endif
 
-	while (!rebuild)
+	while (TRUE)
 	{
 		int revents;
 		char buf[1];
@@ -493,7 +492,7 @@ static job_requeue_t watch(private_watcher_t *this)
 				}
 				this->pending = FALSE;
 				DBG2(DBG_JOB, "watcher got notification, rebuilding");
-				return JOB_REQUEUE_DIRECT;
+				break;
 			}
 
 			reset_log(logbuf, logpos, loglen);
@@ -502,8 +501,7 @@ static job_requeue_t watch(private_watcher_t *this)
 			{
 				if (entry->in_callback)
 				{
-					rebuild = TRUE;
-					break;
+					continue;
 				}
 				reset_event_log(eventbuf, eventpos);
 				revents = find_revents(pfd, count, entry->fd);
@@ -545,7 +543,7 @@ static job_requeue_t watch(private_watcher_t *this)
 					lib->processor->execute_job(lib->processor, job);
 				}
 				/* we temporarily disable a notified FD, rebuild FDSET */
-				return JOB_REQUEUE_DIRECT;
+				break;
 			}
 		}
 		else
@@ -554,7 +552,7 @@ static job_requeue_t watch(private_watcher_t *this)
 			{	/* complain only if no pending updates */
 				DBG1(DBG_JOB, "watcher poll() error: %s", strerror(errno));
 			}
-			return JOB_REQUEUE_DIRECT;
+			break;
 		}
 	}
 	return JOB_REQUEUE_DIRECT;