unix: set and clear UNP_CONNECTING in the same function

[inquire_JohnEricson.me]

Summary

unp_connectat() set the flag, unp_connect_peer() cleared it on success,
and unp_connectat() cleared it again on error. Two clearers, each with
the same KASSERT, and a contract that only a comment held together. Now
the set and the clear sit in one function, a few lines apart, where they
can be read as the pair they are.

The obstacle was the lock: unp_connect_peer() cleared the flag while
holding locks the caller cannot easily retake, since for a connection
oriented socket the peer it ends up holding is a freshly cloned one the
caller never names.

So have it drop only the peer's lock and leave ours held, and let the
caller clear the flag on both outcomes. It does not lock on the caller's
behalf: on failure it returns having taken nothing, as it always did, and
the caller -- being the one that wants the lock -- takes it. That leaves
the function with no knowledge of what its caller means to do next, which
a second caller may well not share.

return_locked becomes return_peer_locked as a result. It never said
which of the two PCBs it governed, and the answer was both; now that our
own is always returned locked, it concerns only the peer, and can say so.

No functional change intended.

Signed-off-by: John Ericson <[e-mail]>
Assisted-by: Claude Code (Claude Opus 5)

[markj]

sys/kern/uipc_usrreq.c

3009

I'd really prefer not to add comments like this one, which just explains what the code is doing.

if (error != 0)
    UNP_PCB_LOCK(unp);
UNP_PCB_LOCK_ASSERT(unp);
unp->unp_flags &= ~UNP_CONNECTING;

is already quite clear.

3222

Could we please assert that UNP_CONNECTING is set here?

[glebius]

To be fair I don't like this change. Maybe the new version is more clear to Claude, but not to me. I spent at least 15 minutes reading through the change and I needed to bent my mind on the new version, rather than on the old.

The existing contract is clear: caller sets UNP_CONNECTING and the connecting function clears it on success. On failure it leaves the flag to the caller, so that caller makes the decision what to do next with the socket. Maybe caller will retry? I know retrying is impossible with current unix(4), I'm just saying there is nothing wrong with existing contract about flag ownership.

[markj]

In D58770#1352025, @glebius wrote:

To be fair I don't like this change. Maybe the new version is more clear to Claude, but not to me. I spent at least 15 minutes reading through the change and I needed to bent my mind on the new version, rather than on the old.

The existing contract is clear: caller sets UNP_CONNECTING and the connecting function clears it on success. On failure it leaves the flag to the caller, so that caller makes the decision what to do next with the socket. Maybe caller will retry? I know retrying is impossible with current unix(4), I'm just saying there is nothing wrong with existing contract about flag ownership.

In general, functions which acquire some resource (UNP_CONNECTING is just an ad-hoc mutex) should be responsible for releasing it. Of course, this isn't always possible, but it's a good practice.

Before some recent refactoring, UNP_CONNECTING was handled this way. This patch restores that. It seems quite reasonable to me...

[inquire_JohnEricson.me]

Maybe the new version is more clear to Claude, but not to me.

Before some recent refactoring, UNP_CONNECTING was handled this way. This patch restores that. It seems quite reasonable to me...

Yes when I had Claude factore out one of the functions in the earlier landed patches, I didn't realize I had duplicated the KASSERT and broken the symmetrical acquire+release in one function. Had I noticed I would have made sure to do something like this to begin with!

So this is more like "Claude didn't care, but John does", not the other way around. :)

(That just slipped by my own self-review as I was mainly focused on the earlier functions getting the peer, than what happened once we got the peer, since that was the remaining step which wasn't changing semantically.)

I do agree having two locked PCBs unlocked in different ways (one by success/failure, one by by the boolean parameter) is a lot to think about, but we already had two different locking policies, it was just poorly described. D58772, if we like it, will get rid of the boolean parameter meaning that once again at least one of the locks is unconditionally unlocked at function exit, which I think "pays down the complexity".