Kyua skips tests based on the jail execution environment if a system is built WITHOUT_JAIL. Thus, the test case does not need to handle it.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
@olivier , I remember this test was like a stumbling block for you in the past. I hope this patch does not impact your workflow. It's already based on jail usage and it seems logical to make it being jailed to avoid adding another exclusive test and not to do manual check & skip for the jail feature.
More patches like this may appear in the future with migration of jail-based tests to execenv=jail.
Comment Actions
Why exactly did this test need to be run in exclusive mode before? Some comments would be more useful.
I wonder if it's possible to select the execenv in the test header, rather than having it in the makefile? Here it seems a bit cleaner if we can keep all of the test metadata in one file.
| tests/sys/kern/sysctl_security_jail_children.sh | ||
|---|---|---|
| 41 | Isn't this check still needed? The test uses jail(8). | |
Comment Actions
Thanks for raising this. It seems I made it exclusive due to it counts number of jails and it will easily fail if another test is spawning jails in parallel, because of they share the same "origin" parent jail (the name from this test), which is the prison0 typically. This reasoning is kind of documented, but it's not very explicit and is within the test code, actually it covers another aspect -- that it still can fail if something else spawns jails during the test run. This is really a good idea to make it explicit and provide the comment along with the metadata property itself.
It's expected to be truly isolated if that "origin" parent jail is tests's personal temporary jail, which can be provided by execenv=jail.
I wonder if it's possible to select the execenv in the test header, rather than having it in the makefile? Here it seems a bit cleaner if we can keep all of the test metadata in one file.
Indeed. And it will be inlaid with the explicit explanation why.
| tests/sys/kern/sysctl_security_jail_children.sh | ||
|---|---|---|
| 41 | The execenv=jail feature works such way that a test case will be skipped with the respective message if the OS instance is built WITHOUT_JAIL. Thus, we already have a mechanism for this and a test case based on execenv=jail does not need to handle it manually. | |
Comment Actions
Now it looks like it does not require an extra comment, it runs in a jail to isolate from others and that's it. What do you think?
Comment Actions
This looks ok to me. Approved.
In the commit log message, please explain why we're removing the test -s jail check, as it's not very obvious.