Re: [PATCH v8 10/19] pesto, conf: Have pesto connect to passt and check versions

On Wed, 6 May 2026 18:21:32 +1000 David Gibson wrote:

On Wed, May 06, 2026 at 09:55:32AM +0200, Stefano Brivio wrote:

...

On Wed, 6 May 2026 15:38:30 +1000 David Gibson wrote:

...

On Wed, May 06, 2026 at 01:47:10AM +0200, Stefano Brivio wrote:

...

From: David Gibson

Start implementing pesto in earnest. Create a control/configuration socket in passt. Have pesto connect to it and retrieve a server greeting Perform some basic version checking.

Signed-off-by: David Gibson [sbrivio: Avoid potential recursive calling between conf_accept() and conf_close(), reported by clang-tidy]

Huh. For some reason that warning didn't trip for me. Although it's technically true they can mutually recurse, I believe they're both tail calls, so it shouldn't eat the stack.

...

[sbrivio: In conf(), check we're not exceeding sizeof(c->control_path) instead of sizeof(c->socket_path), and, in pesto's main(), print argv[optind] instead of argv[1] to indicate an invalid socket path, both reported by Jon Maloy] [sbrivio: In pesto's main(), drop unnecessary newline from error message, reported by Laurent] [sbrivio: Don't use SOCK_NONBLOCK on accept4(), as that only applies to the *new* file descriptor, which we don't want -- set O_NONBLOCK on the listening file descriptor using fcntl()]

Making the new (accepted) socket non-blocking was the intended behaviour here. We also want non-blocking for the listening socket, but that was already done in feab892c7 ("tap, repair: Use SOCK_NONBLOCK and SOCK_CLOEXEC on Unix sockets").

Oops, now that Laurent mentioned it, I realised I dropped it accidentally while / after debugging things on v6, and:

Ah, right.

...

...

WIth the current design, I guess we don't want non-blocking on the accepted socket, although I don't think it actually matters very much.

...this is the issue I was trying to fix: if the accepted socket is non-blocking, messages are cut short sometimes, and in general things don't work.

Hrm. I was pretty sure setting it blocking just meant you'd always get *some* data instead of EAGAIN. I don't believe it prevents either short reads or short writes.

Maybe, but something caused actual problems for me, otherwise I wouldn't have played with it at all. The current behaviour with this patch is something I tested quite heavily by now.

Both sides should already be using {read,write}_all_buf() to handle short read/writes, so I'm really not sure where it's going wrong if the accepted socket is non-blocking (other than maybe spinning on EAGAIN more than we'd like).

...

I don't remember if this was while testing things on Fedora or Debian, it only happened in one of the two environments.

So, while it was accidental (I really didn't leave any note for a cover letter, so I'm almost certain there was no other reason for me to drop it), I think it's actually a good idea to drop it for the following reasons:

- O_NONBLOCK on the accepted socket breaks things

The earlier patch doesn't affect O_NONBLOCK on the accepted socket, only on the listening socket (it's not inherited).

...

- the rest looks correct to me but fairly out of scope, and I have very limited time for testing things in detail right now, so I'd rather keep that patch for later

It's in scope because O_NONBLOCK on the listening socket is essential

This can be implemented in two lines like the current version does. The rest is out of scope.

to implementing the "concurrent client blocks" instead of "concurrent client is rejected" behaviour. Without O_NONBLOCK on the listening socket, we can't safely call conf_accept() anywhere other than in response to the epoll event - because looking for additional connections after we close one could block.

...

Without that patch, and my follow-up change to this patch, we're just adding two lines for this specific behaviour, instead of 18.

...

We will want non-blocking it when we change this to read out the updated rules incrementally, rather than all at once.

Right, so maybe we can keep that patch for that moment. Or even before,

"that patch" meaning the sock_unix() one? Again, that affects the listening socket behaviour. What we need for incrementally reading the rules is about the accepted socket behaviour.

Yes. That doesn't just affect the listening socket, it affects a bunch of things, and they can all be checked and revisited, more conveniently, together, at a later point. -- Stefano