From: Aurelien DARRAGON <adarragon@haproxy.com>
Date: Tue, 7 Feb 2023 10:23:38 +0000 (+0100)
Subject: BUG/MEDIUM: listener: fix pause_listener() suspend return value handling
X-Git-Tag: v2.8-dev5~111
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=7a15fa58b1087db4af60d4beaca0a11bbc8c0778;p=thirdparty%2Fhaproxy.git

BUG/MEDIUM: listener: fix pause_listener() suspend return value handling

Legacy suspend() return value handling in pause_listener() has been altered
over the time.

First with fb76bd5ca ("BUG/MEDIUM: listeners: correctly report pause() errors")
Then with e03204c8e ("MEDIUM: listeners: implement protocol level
->suspend/resume() calls")

We aim to restore original function behavior and comply with resume_listener()
function description.
This is required for resume_listener() and pause_listener() to work as a whole

Now, it is made explicit that pause_listener() may stop a listener if the
listener doesn't support the LI_PAUSED state (depending on the protocol
family, ie: ABNS sockets), in this case l->state will be set to LI_ASSIGNED
and this won't be considered as an error.

This could be backported up to 2.4 after a reasonable observation period
to make sure that this change doesn't cause unwanted side-effects.

--
Backport notes:

This commit depends on:
 - "MINOR: listener: make sure we don't pause/resume bypassed listeners"

-> 2.4: manual change required because "MINOR: proxy/listener: support
for additional PAUSED state" was not backported: the contextual patch
lines don't match.

Replace this:

    |        if (px && !px->li_ready) {
    |                /* PROXY_LOCK is required */

By this:

    |        if (px && !px->li_ready) {
    |               ha_warning("Paused %s %s.\n", proxy_cap_str(px->cap), px->id);
---

diff --git a/src/listener.c b/src/listener.c
index 25b22e13c4..22b3b91685 100644
--- a/src/listener.c
+++ b/src/listener.c
@@ -399,19 +399,17 @@ void default_add_listener(struct protocol *proto, struct listener *listener)
 
 /* default function called to suspend a listener: it simply passes the call to
  * the underlying receiver. This is find for most socket-based protocols. This
- * must be called under the listener's lock. It will return non-zero on success,
- * 0 on failure. If no receiver-level suspend is provided, the operation is
- * assumed to succeed.
+ * must be called under the listener's lock. It will return < 0 in case of
+ * failure, 0 if the listener was totally stopped, or > 0 if correctly paused..
+ * If no receiver-level suspend is provided, the operation is assumed
+ * to succeed.
  */
 int default_suspend_listener(struct listener *l)
 {
-	int ret = 1;
-
 	if (!l->rx.proto->rx_suspend)
 		return 1;
 
-	ret = l->rx.proto->rx_suspend(&l->rx);
-	return ret > 0 ? ret : 0;
+	return l->rx.proto->rx_suspend(&l->rx);
 }
 
 
@@ -462,6 +460,8 @@ int default_resume_listener(struct listener *l)
  * closes upon SHUT_WR and refuses to rebind. So a common validation path
  * involves SHUT_WR && listen && SHUT_RD. In case of success, the FD's polling
  * is disabled. It normally returns non-zero, unless an error is reported.
+ * pause() may totally stop a listener if it doesn't support the PAUSED state,
+ * in which case state will be set to ASSIGNED.
  * It will need to operate under the proxy's lock and the listener's lock.
  * The caller is responsible for indicating in lpx, lli whether the respective
  * locks are already held (non-zero) or not (zero) so that the function pick
@@ -481,12 +481,27 @@ int pause_listener(struct listener *l, int lpx, int lli)
 	if (!(l->flags & LI_F_FINALIZED) || l->state <= LI_PAUSED)
 		goto end;
 
-	if (l->rx.proto->suspend)
+	if (l->rx.proto->suspend) {
 		ret = l->rx.proto->suspend(l);
+		/* if the suspend() fails, we don't want to change the
+		 * current listener state
+		 */
+		if (ret < 0)
+			goto end;
+	}
 
 	MT_LIST_DELETE(&l->wait_queue);
 
-	listener_set_state(l, LI_PAUSED);
+	/* ret == 0 means that the suspend() has been turned into
+	 * an unbind(), meaning the listener is now stopped (ie: ABNS), we need
+	 * to report this state change properly
+	 */
+	listener_set_state(l, ((ret) ? LI_PAUSED : LI_ASSIGNED));
+
+	/* at this point, everything is under control, no error should be
+	 * returned to calling function
+	 */
+	ret = 1;
 
 	if (px && !px->li_ready) {
 		/* PROXY_LOCK is required */