In D49463#1128242, @ngie wrote:

• Could you please verify that this doesn't regress Linux/MacOS?

Polish and apply the suggestions

Well, it was a quick one hour patch, now it's time for polishing.

In D49463#1128023, @markj wrote:

Whoa, nice. :)

What happens if tests are running in parallel? The behaviour I would naively expect is that other running tests will keep going, but new ones will not be scheduled until kyua is resumed.

Report test work dir

LGTM

In D48087#1117288, @ngie wrote:

Could requires be used instead of the term prepare? I think that would align the concept and the command more with what is being reported on.

In D47668#1120011, @jamie wrote:

I'm glad that turned out to be a workable option - thanks for adding it.

As to your comment that return an empty value for a nonexistent key instead of stopping the whole thing up with ENOENT, I see your point. But now I see another way: the NULL value, this time in the other direction. From the kernel interface side, it would be just the same as the setting direction, where receiving a NULL indicated attempted retrieval of a nonexistent key, while an empty string would only be for a key with an empty value.

It gets trickier on the user end though, where jls(8) only sometimes allows for a distinction. In particular, "jls -n meta.foo" or "jls -s meta.foo" could be made to show the same distinction between meta.foo="" and just meta.foo, but without the option there would be no way to tell. That's probably sufficient for those who care, though.

Communicate back a missing key

In D47668#1115164, @jamie wrote:

A few observations from actually running this:

Add key removal mechanism using NULL value as a trigger

Add the respective code comment

In D49039#1118150, @asomers wrote:

LGTM. Except that the commit message contains a typo: "uprivileged_user" => "unprivileged_user"

I guess it could be helpful for a couple of test cases from tests/sys/netinet/fibs_multibind_test.c and/or tests/sys/netinet/socket_afinet.c.

@ngie , it would be appreciated if you could find time to consider this. It seems that your opinion is necessary to move forward.

Update kyuafile.5 after landing of D48190

Improve the comment regarding the retry mechansim

tests: Use recently added support of is.exclusive coming from ATF side

This is a follow-up to https://reviews.freebsd.org/D48190, where it was discovered that this support is missing. It's obvious that our test suite does not use it, and this patch is for consistency and not to say "not mapped" in the kyuafile.5. Also, I found that Kyua 0.11 release notes state that it should work, so actually this is a fix.

In D48190#1100132, @ngie wrote:

I think submitting this upstream (freebsd/kyua) and filing an issue capturing the feedback/concerns would probably be ok.

In D48087#1096440, @kp wrote:

One trivial remark about 'pip? # e.g. to have scapy installed'. We have a scapy package and that's how I've always installed it. Right now it's py311-scapy-2.6.1.

In D48190#1099037, @ngie wrote:

It honestly seems like this documentation should live in another place, e.g., a "test driver" [1] (kyua-atf-driver(4)?) manpage. Putting all of this information in a single manpage seems like it would really clutter up what's being explained here, and the more "test drivers" get bolted on to kyua, the more complex the documentation will become.

1. I'm not sold on the name "test driver", but it's the best I can offer for the concept right now.

This is a follow-up to https://reviews.freebsd.org/D47671, where we discussed documenting of the mapping.

In D47671#1098794, @ngie wrote:

If you could commit it directly to the freebsd/src:main, that would be helpful :) (I think it's better to get it in sooner so it gets some bake time on FreeBSD).

Now the wording is away from the internals and closer to the end users:

> ./kyua help | grep prepare
  prepare                 Prepare env and resolve requirements before testing.

My current favorite is the prepare term proposed by Kristof. So, I've changed it in the latest version of the patch for the sake of further testing and discussion. Everything is open for renaming. The internals still use requirement resolver terminology, I could not quickly find the replacement and probably it's not needed due to "prepare" is kind of higher level, anyway this is the encapsulated part with the only exposure via CLI help output.

Rename "kyua rr" to "kyua prepare"; Drop "kyua test --rr"

Make keyvalue_contention test case more accurate

Tiny code improvements, no functional change

Add keyvalue_contention test case

Fix jm_h_cut_occurrences() logic

libjail: Correctly differentiate no<boolparam> from keyvalue-based ones

Each buffer can be handled as a set of key=value\n strings

By the way, now I'm leaning towards resolve subcommand as you mentioned:

> kyua resolve kmods
> kyua test { --resolve | -r } kmods testprog

In D48087#1096189, @markj wrote:

As usual, I will quibble a bit about naming - the kyua subcommands are mostly english words, so "rr" is a bit odd. Why not "resolve" or even "resolve-requirements" (or "resolve-reqs")?

This is the very first working version of the Project B: https://lists.freebsd.org/archives/freebsd-testing/2024-November/000395.html.

In D47671#1095969, @ngie wrote:

Thanks! Please update the differential revision with the tests. Memory serves me correctly, CI on main might start failing if the tests aren't introduced.

In D47671#1095814, @ngie wrote:

Please open a PR against freebsd/atf .
Also: this deserves a test (upstream).

In D47668#1095800, @jamie wrote:

In D47668#1095796, @igoro wrote:

• The allowed chars for each buffer are very limited by default, it covers Base64, k=v\n format, and some extra bytes. It can be changed via security.jail.meta_allowedchars sysctl. For convenience (as it seems to me for now), setting it to an empty string allows everything.

Why is this a kernel issue? Aside from NUL, because it preserves the C-string nature, allowed characters would seem only to be a concern on the user side.

In D47671#1087433, @markj wrote:

The change itself looks fine to me, but don't we need to document this somewhere?

The latest update aggregates the recent discussions:

• Rename metaext/metaint to meta/env. meta is expected to be used for "tagging" and hidden from the jail. env is intended for "configuring" and readable by the jail through security.jail.env sysctl.
• The allowed chars for each buffer are very limited by default, it covers Base64, k=v\n format, and some extra bytes. It can be changed via security.jail.meta_allowedchars sysctl. For convenience (as it seems to me for now), setting it to an empty string allows everything.
• The tests and man page are upgraded respectively.

Rename to meta/env, add meta_allowedchars.

In D46653#1091544, @ngie wrote:

Regarding the change proposed on the GitHub PR. I think it's better to merge the existing PR to keep FreeBSD src/contrib/kyua and the github/freebsd/kyua in sync with absolutely the same commit. And I would open a new PR for the change requested with a reference to the previous GitHub discussion/PR. What do you think is the best here from the organizational perspective?

I don't think it would be a good idea to merge the PR as-is. I'm trying to avoid violating POLA as much as humanly possible for 0.14 and taking a look at the output definitely violates POLA. There are enough Linux/MacOS things to deal with -- I'd rather not get more questions with a UX change like has been made on main...

In D46653#1091513, @ngie wrote:

For the record, stuff like this should really be committed to freebsd/kyua first, then backported. As of right now this change is not in what's intended to go in to 0.14 and in order to do the upgrade I would need to reapply this patch. I provided a ship-it earlier, but I was swayed by the comment in the PR about this change being confusing for naive results parsers: https://github.com/freebsd/kyua/pull/230 .

In D47668#1090028, @jamie wrote:

In D47668#1089529, @igoro wrote:

Yeah, it seems that on the Jail Call of 26-Nov we came to a conclusion that for now we would keep it very simple like two buffers per jail managed from the user-land side, while keeping a wide spectrum of opportunities to extend it in the future having a more specific production need in mind. Thus, we can postpone thinking about extra complexity on the kernel side.

If I had managed to make that call, I have to say that at least the conclusion wouldn't have been unanimous. The single blob is indeed simpler to implement, but not simpler to use. Still, you mention extension, and that's a possibility. I'm thinking something like:

meta="foo=bar\0baz=bletch"
meta.foo="bar"
meta.baz="bletch"

In D47668#1089218, @crest_freebsd_rlwinm.de wrote:

In D47668#1088649, @igoro wrote:

It seems there is an agreement to split it onto two buffers per jail: both are readable by parent jail, while only one is readable by a jail itself. The updated patch reflects this concept. For now metaext and metaint naming is used as external/internal concept. The naming is open for discussion.

There is a third useful combination: (privileged) access host access only e.g. as a place to store API token or similar things without ever writing them to a file.

The problem having a fixed set of sysctls with with their designated access permissions is that all accesses have to go through a single writer and use the same data format because a single sysctl is just one string. This means anyone else is required closely coupled to that writer unless there is some standardised patch and query interface (e.g. JSON patch and JSON query) and I can't imagine anyone wanting to deal with that complexity. A flat key=value store avoids this by allowing multiple writers to lay exclusive claim to their part of the keyspace, but it doesn't absolve the need for access control.

It seems there is an agreement to split it onto two buffers per jail: both are readable by parent jail, while only one is readable by a jail itself. The updated patch reflects this concept. For now metaext and metaint naming is used as external/internal concept. The naming is open for discussion.

v2: Split onto two buffers per jail: external & internal

Great, I can continue from any point where you decide to stop and land your commits.

Thank you all for the discussion. I've refreshed the context for myself and I would like to share it:

• We have got a new -J flag for sysctl(8). It filters the variables by CTLFLAG_PRISON.
• It reminded me of a couple of existing sysctlS which are not listed with the -J flag.
• That's because they are read-only (CTLFLAG_RD) without CTLFLAG_PRISON flag set. The reasoning follows.
• The CTLFLAG_PRISON has been used as a way to allow jailed roots modify a sysctl. It works very simple way. If a sysctl sys call request means modification, i.e. a new buffer (the req->newptr) is provided, then the sys call will check if the user has PRIV_SYSCTL_WRITE privilege. It does not work for a jailed root, that's why the sys call will check for another privilege (PRIV_SYSCTL_WRITEJAIL) if the sysctl in question has CTLFLAG_PRISON flag set. That's the only use case for this flag for now.
• And it's fine that read-only per-jail variables do not have this flag, it's not needed for them.
• As long as I found this little inconsistency I proposed to update the meaning of this flag (sysctl.9) and add it to some read-only variables which are, kind of, expected to be found in the sysctl -aJ output.

Pivot the patch.

I would like to share my thoughts regarding this.

Currently, there is no accessible way to attach metadata to jails. This is commonly used elsewhere, for example in Kubernetes, to allow non-unique properties to enrich load balancers, schedulers, and volume provisioners to make more informed decisions.