[PATCH] passt-repair: Send one confirmation *per command*, not *per socket*

[PATCH] passt-repair: Send one confirmation *per command*, not *per socket*

[Stefano Brivio]

It looks like me, myself and I couldn't agree on the "simple" protocol
between passt and passt-repair. The man page and passt say it's one
confirmation per command, but the passt-repair implementation had one
confirmation per socket instead.

This caused all sort of mysterious issues with repair mode
pseudo-randomly enabled, and leading to hours of fun (mostly not
mine). Oops.

Switch to one confirmation per command (of course).

Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 passt-repair.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/passt-repair.c b/passt-repair.c
index 322066a..614cee0 100644
--- a/passt-repair.c
+++ b/passt-repair.c
@@ -63,6 +63,7 @@ int main(int argc, char **argv)
 	struct cmsghdr *cmsg;
 	struct msghdr msg;
 	struct iovec iov;
+	int op;
 
 	prctl(PR_SET_DUMPABLE, 0);
 
@@ -150,25 +151,24 @@ loop:
 		_exit(1);
 	}
 
-	for (i = 0; i < n; i++) {
-		int o = cmd;
+	op = cmd;
 
-		if (setsockopt(fds[i], SOL_TCP, TCP_REPAIR, &o, sizeof(o))) {
+	for (i = 0; i < n; i++) {
+		if (setsockopt(fds[i], SOL_TCP, TCP_REPAIR, &op, sizeof(op))) {
 			fprintf(stderr,
-				"Setting TCP_REPAIR to %i on socket %i: %s", o,
+				"Setting TCP_REPAIR to %i on socket %i: %s", op,
 				fds[i], strerror(errno));
 			_exit(1);
 		}
 
 		/* Close _our_ copy */
 		close(fds[i]);
+	}
 
-		/* Confirm setting by echoing the command back */
-		if (send(s, &cmd, sizeof(cmd), 0) < 0) {
-			fprintf(stderr, "Reply to command %i: %s\n",
-				o, strerror(errno));
-			_exit(1);
-		}
+	/* Confirm setting by echoing the command back */
+	if (send(s, &cmd, sizeof(cmd), 0) < 0) {
+		fprintf(stderr, "Reply to %i: %s\n", op, strerror(errno));
+		_exit(1);
 	}
 
 	goto loop;
-- 
@@ -63,6 +63,7 @@ int main(int argc, char **argv)
 	struct cmsghdr *cmsg;
 	struct msghdr msg;
 	struct iovec iov;
+	int op;
 
 	prctl(PR_SET_DUMPABLE, 0);
 
@@ -150,25 +151,24 @@ loop:
 		_exit(1);
 	}
 
-	for (i = 0; i < n; i++) {
-		int o = cmd;
+	op = cmd;
 
-		if (setsockopt(fds[i], SOL_TCP, TCP_REPAIR, &o, sizeof(o))) {
+	for (i = 0; i < n; i++) {
+		if (setsockopt(fds[i], SOL_TCP, TCP_REPAIR, &op, sizeof(op))) {
 			fprintf(stderr,
-				"Setting TCP_REPAIR to %i on socket %i: %s", o,
+				"Setting TCP_REPAIR to %i on socket %i: %s", op,
 				fds[i], strerror(errno));
 			_exit(1);
 		}
 
 		/* Close _our_ copy */
 		close(fds[i]);
+	}
 
-		/* Confirm setting by echoing the command back */
-		if (send(s, &cmd, sizeof(cmd), 0) < 0) {
-			fprintf(stderr, "Reply to command %i: %s\n",
-				o, strerror(errno));
-			_exit(1);
-		}
+	/* Confirm setting by echoing the command back */
+	if (send(s, &cmd, sizeof(cmd), 0) < 0) {
+		fprintf(stderr, "Reply to %i: %s\n", op, strerror(errno));
+		_exit(1);
 	}
 
 	goto loop;
-- 
2.43.0

Re: [PATCH] passt-repair: Send one confirmation *per command*, not *per socket*

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2254 bytes --]

On Fri, Feb 07, 2025 at 09:03:58AM +0100, Stefano Brivio wrote:
> It looks like me, myself and I couldn't agree on the "simple" protocol
> between passt and passt-repair. The man page and passt say it's one
> confirmation per command, but the passt-repair implementation had one
> confirmation per socket instead.
> 
> This caused all sort of mysterious issues with repair mode
> pseudo-randomly enabled, and leading to hours of fun (mostly not
> mine). Oops.
> 
> Switch to one confirmation per command (of course).
> 
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Ouch.

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  passt-repair.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/passt-repair.c b/passt-repair.c
> index 322066a..614cee0 100644
> --- a/passt-repair.c
> +++ b/passt-repair.c
> @@ -63,6 +63,7 @@ int main(int argc, char **argv)
>  	struct cmsghdr *cmsg;
>  	struct msghdr msg;
>  	struct iovec iov;
> +	int op;
>  
>  	prctl(PR_SET_DUMPABLE, 0);
>  
> @@ -150,25 +151,24 @@ loop:
>  		_exit(1);
>  	}
>  
> -	for (i = 0; i < n; i++) {
> -		int o = cmd;
> +	op = cmd;
>  
> -		if (setsockopt(fds[i], SOL_TCP, TCP_REPAIR, &o, sizeof(o))) {
> +	for (i = 0; i < n; i++) {
> +		if (setsockopt(fds[i], SOL_TCP, TCP_REPAIR, &op, sizeof(op))) {
>  			fprintf(stderr,
> -				"Setting TCP_REPAIR to %i on socket %i: %s", o,
> +				"Setting TCP_REPAIR to %i on socket %i: %s", op,
>  				fds[i], strerror(errno));
>  			_exit(1);
>  		}
>  
>  		/* Close _our_ copy */
>  		close(fds[i]);
> +	}
>  
> -		/* Confirm setting by echoing the command back */
> -		if (send(s, &cmd, sizeof(cmd), 0) < 0) {
> -			fprintf(stderr, "Reply to command %i: %s\n",
> -				o, strerror(errno));
> -			_exit(1);
> -		}
> +	/* Confirm setting by echoing the command back */
> +	if (send(s, &cmd, sizeof(cmd), 0) < 0) {
> +		fprintf(stderr, "Reply to %i: %s\n", op, strerror(errno));
> +		_exit(1);
>  	}
>  
>  	goto loop;

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]