Oh I didn't mean a real-world test with actual xorg, I meant a small test of connect/connect itself ensuring we get the correct error code.

Sorry about the breakage! I have no authority here, but would you mind writing a test for this? See where the other connectat test are.

Correct the agent version

Thanks so much, @markj! Recally happy and honored to see the whole patch series approved; my first major contribution to FreeBSD.

Address @kib's line comments. Also take a stab at blacklisting most
individual commands, and what future work on that would look like.

I have a global question about this change. Do we really want or need to enable all procctl(2) ops in capability mode?

Edit the patch as @kib requested

Also, there is the very pressing question of whether "embryo" or "zygote" is more apt :).

Update tests in light of big reworking of D58688

Update to be much closer to what @kib requested.

See the comment I left on D58685, I hope to come back to that someday, but not yet. That makes this not needed either.

I would love to do this and D58684 someday, but I am adopting a "weird fork but normal exec" in-between model for now, so we can get this fast pd-based spawning in without a major VM overhaul.

@kib Interesting. So my first reaction was being a bit sad, because I personally very much want to "push back on" the fork/exec model, but upon further reflection I think this is not so incompatible after all. Also, you are absolutely right that we need a way to skip the VM refactors for now, as otherwise this stuff is just impossible to review; there just isn't anyway around that.

Fixing some things based on changes to D58586

In D58586#1346656, @kib wrote:

Well, procctl(2) might be the target but might be in some future.
More interesting and with the immediate applications is the sysctl kern.proc. MIB tree.

Oh I suppose rights.4 needs to be updated, too?

BTW I had to do this to get it to build on the latest main

From a quick glance, this looks good to me. I am going to do something similar for procctl inspired by it.

A few fixes

Thank you! Unless I am missing something, I cannot push to land accepted changes anyways, so yes I'll be waiting for @markj regardless :).

Add new test and warning about discouraged functionality as requested

Keep tests more additive as @glebius suggested for clearer diff. Also, test another more situations that I thought of.

@glebius Oh haha, that was not Claude's idea but mine! It didn't do it that way originally, but then I explicitly asked it to.

Test all the fdescfs combinations explicitly

In D58675#1346285, @markj wrote:

In D58675#1346246, @inquire_JohnEricson.me wrote:

Also, do you think we've made enough cleanup/refactor PRs now that it might good to start landing them?

I ought to rebase mine on top of this for testing, I think, but I don't want to make a mess of the ones that you already approved but want to make slight changes too (per the discussion D58460, if I understand correctly).

I have a branch with all of the patches together (plus a couple more for some unrelated bugs I found recently) and am currently fuzzing them with syzkaller. If I don't see any problems in the next couple of days I'll go ahead and land them.

Rebasing is fine, please just make sure you update all of the patches in the series when you do so, as I'll fetch patches from phabricator in order to commit them. git arc tries to make review updates tolerably easy.

(I don't think it's actually necessary to rebase though? AFAICS your patches don't touch this function.)

Also, do you think we've made enough cleanup/refactor PRs now that it might good to start landing them?

Very nice! Always love a mostly-red patch.

These tests don't exercise fdescfs lookups, nor handling of O_PATH sockets. Were you planning to add something for that?

For what it's worth I find it much easier to upload patches from the command-line, e.g. with git-arc: https://wiki.freebsd.org/Phabricator#Create_a_Revision_via_command_line_.28using_git-arc.29

OK I update the diff via the web form, which I noticed messes up the diff-to-diff comparison, since the base commit is gone, but hopefully that does not matter when landing this.

Fix the return statement to match the style guide

I think the proposal as a whole is reasonable. This new behaviour for connectat() feels very similar to AT_EMPTY_PATH, so there's some precedent for it.

@markj. are you still on the fence for the motivation after what I wrote? If so, might it make sense to first look at some of the non-functional changes before this one? I am hoping that they would stand on their own, especially D58460.

Ah OK, I understand now.

Sorry, I still do not quite get the point of this. Why can't the server which hands out a listening socket to other processes to connect to, instead hand out pre-connected sockets?

Oh I did the wrong diff for this last update?

Expanded test suite to cover the new larger feature surface area

Major overhauls, now that this is part of a much larger patch series

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.

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?

Rework error handling to avoid "bad2" as requested

In D58405#1340280, @markj wrote:

This would also need updates to connectat.2 and unix.4.