unix: split `unp_connectat()` in two
AcceptedPublic
Actions

Authored by inquire_JohnEricson.me on Wed, Jul 22, 9:36 PM.

Details

Reviewers

kib
markj

Group Reviewers

capsicum

Summary

Factor the second half — connecting to an already-resolved peer PCB —
out into a new unp_connect_peer(), leaving unp_connectat() with the
connection state machine and pathname resolution. No functional change.

The helper's contract: the caller guarantees stability of the peer PCB
(vnode lock plus unp_vp_mtxpool lock for peers found via
VOP_UNP_CONNECT()), has set UNP_CONNECTING on the connecting socket,
and clears it again on error; the helper clears it on success.

This prepares for connecting to a peer named by something other than a
pathname.

Signed-off-by: John Ericson <John.Ericson@Obsidian.Systems>

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 75027
Build 71910: arc lint + arc unit

Event Timeline

inquire_JohnEricson.me created this revision.Wed, Jul 22, 9:36 PM

Herald added subscribers: olce, glebius, imp. · View Herald TranscriptWed, Jul 22, 9:36 PM

inquire_JohnEricson.me requested review of this revision.Wed, Jul 22, 9:36 PM

Harbormaster completed remote builds in B75027: Diff 182482.Wed, Jul 22, 9:36 PM

inquire_JohnEricson.me added a child revision: D58405: unix: allow `connectat(2)` to name the peer socket by descriptor.Wed, Jul 22, 9:39 PM

inquire_JohnEricson.me added reviewers: capsicum, kib, markj.Thu, Jul 23, 4:47 AM

Looks good to me overall.

sys/kern/uipc_usrreq.c
2980	If you write this fragment like this: if (unp2 == NULL) error = ECONNREFUSED; else error = unp_connect_peer(so, unp2, &sa, td, return_locked); mtx_unlock(vplock); then you don't need the `bad2` label.

Rework error handling to avoid "bad2" as requested

Harbormaster completed remote builds in B75064: Diff 182563.Thu, Jul 23, 9:10 PM

inquire_JohnEricson.me marked an inline comment as done.Thu, Jul 23, 9:13 PM

markj accepted this revision.Fri, Jul 24, 1:45 PM

This revision is now accepted and ready to land.Fri, Jul 24, 1:45 PM

OK thanks for approving! To be clear, I don't think I have permissions to land it (not really sure how arcanist intended contributors to do this part). Will one of you land it either now, or once we've approved the whole stack?

In D58404#1340667, @inquire_JohnEricson.me wrote:

OK thanks for approving! To be clear, I don't think I have permissions to land it (not really sure how arcanist intended contributors to do this part). Will one of you land it either now, or once we've approved the whole stack?

Yeah, at this point it's up to someone with commit access to actually land the patch and push to git.

I think it's ok to land this as a standalone cleanup, but it'd be nice to conclude the discussion on D58405 first. (If you want to have a broader discussion of the proposal, starting a thread on the freebsd-hackers@ or freebsd-arch@ mailing lists would be the way to go.)

Sure, that sounds good to me. I just wanted to double-check the process. I am happy to continue discussing on D58405, and don't see a need to create a broader mailing list thread if we end up just on Phabricator with you two happy to approve it.

(BTW concurrently to this patch series, I am also working on what I proposed long ago in https://lists.freebsd.org/archives/freebsd-arch/2022-January/000140.html. That is far more involved, and so I think it *does* merit mailing list discussion, but I'll refrain from sending another email until I have a complete draft.)

inquire_JohnEricson.me added a child revision: D58459: unix: factor `unp_sun_path()` out of bind and connect.Sat, Jul 25, 8:21 PM

inquire_JohnEricson.me removed a child revision: D58405: unix: allow `connectat(2)` to name the peer socket by descriptor.Sat, Jul 25, 8:29 PM