1# Patch Review 2 3Anyone can and should review patches. It's the only way to get good at 4patch review and for the project to scale. 5 6## Goals of patch review 7 81. Prevent false positive test results 92. Prevent false negative test results 103. Keep the code as simple as possible, but no simpler 11 12## How to find clear errors 13 14A clear error is one where there is unlikely to be any argument if you 15provide evidence of it. Evidence being an error trace or logical proof 16that an error will occur in a common situation. 17 18The following are examples and may not be appropriate for all tests. 19 20* Merge the patch locally. It should apply cleanly to master. 21* Compile the patch with default and non-default configurations. 22 - Use sanitizers e.g. undefined behaviour, address. 23 - Compile on non-x86 24 - Compile on x86 with -m32 25* Use `make check` 26* Run effected tests in a VM 27 - Use single vCPU 28 - Use many vCPUs and enable NUMA 29 - Restrict RAM to < 1GB. 30* Run effected tests on an embedded device 31* Run effected tests on non-x86 machine in general 32* Run reproducers on a kernel where the bug is present 33* Run tests with "-i0" 34* Compare usage of system calls with man page descriptions 35* Compare usage of system calls with kernel code 36* Search the LTP library for existing helper functions 37 38## How to find subtle errors 39 40A subtle error is one where you can expect some argument because you 41do not have clear evidence of an error. It is best to state these as 42questions and not make assertions if possible. 43 44Although if it is a matter of style or "taste" then senior maintainers 45can assert what is correct to avoid bike shedding. 46 47* Ask what happens if there is an error, could it be debugged just 48 with the test output? 49* Are we testing undefined behavior? 50 - Could future kernel behaviour change without "breaking userland"? 51 - Does the kernel behave differently depending on hardware? 52 - Does it behave differently depending on kernel configuration? 53 - Does it behave differently depending on the compiler? 54 - Would it behave differently if the order of checks on syscall parameters 55 changed in the kernel? 56* Will it scale to tiny and huge systems? 57 - What happens if there are 100+ CPUs? 58 - What happens if each CPU core is very slow? 59 - What happens if there are 2TB of RAM? 60* Are we repeating a pattern that can be turned into a library function? 61* Is a single test trying to do too much? 62* Could multiple similar tests be merged? 63* Race conditions 64 - What happens if a process gets preempted? 65 - Could checkpoints or fuzzsync by used instead? 66 - Note, usually you can insert a sleep to prove a race condition 67 exists however finding them is hard 68* Is there a simpler way to achieve the same kernel coverage? 69 70## How to get patches merged 71 72Once you think a patch is good enough you should add your Reviewed-by 73and/or Tested-by tags. This means you will get some credit for getting 74the patch merged. Also some blame if there are problems. 75 76If you ran the test you can add the Tested-by tag. If you read the 77code or used static analysis tools on it, you can add the Reviewed-by 78tag. 79 80In addition you can expect others to review your patches and add their 81tags. This will speed up the process of getting your patches merged. 82 83## Maintainers Checklist 84 85Patchset should be tested locally and ideally also in maintainer's fork in 86GitHub Actions on GitHub. 87 88NOTE: GitHub Actions do only build testing, passing the CI means only that 89 the test compiles fine on variety of different distributions and releases. 90 91The test should be executed at least once locally and should PASS as well. 92 93Commit messages should have 94 95* Author's `Signed-off-by` tag 96* Committer's `Reviewed-by` or `Signed-off-by` tag 97* Check also mailing lists for other reviewers / testers tags, notes and failure reports 98* `Fixes: hash` if it fixes particular LTP commit 99* `Fixes: #N` if it fixes github issue number N, so it's automatically closed 100 101After patch is accepted or rejected, set correct state and archive in 102https://patchwork.ozlabs.org/project/ltp/list/[LTP patchwork instance]. 103 104Also update `.github/workflows/wiki-mirror.yml` script which mirrors 105`doc/*.txt` to LTP wiki (git URL https://github.com/linux-test-project/ltp.wiki.git) 106if new wiki page is added. 107 108## New tests 109New test should 110 111* Have a record in runtest file 112* Test should work fine with more than one iteration 113 (e.g. run with `-i 100`) 114* Run with `-i 0` to check that setup and cleanup are coded properly (no test is being run) 115* Have a brief description 116* License: the default license for new tests is GPL v2 or later, use 117 GPL-2.0-or-later; the licence for test (e.g. GPL-2.0) should not change 118 unless test is completely rewritten 119* Old copyrights should be kept unless test is completely rewritten 120 121### C tests 122* Use new https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#22-writing-a-test-in-c[C API] 123* Test binaries are added into corresponding `.gitignore` files 124* Check coding style with `make check` 125 (more in https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#21-c-coding-style[C coding style]) 126* Docparse documentation 127* If a test is a regression test it should include tags 128 (more in https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#2238-test-tags[Test tags]) 129* When rewriting old tests, https://en.wikipedia.org/wiki/%CE%9CClinux[uClinux] 130 support should be removed (project has been discontinued). 131 E.g. remove `#ifdef UCLINUX`, replace `FORK_OR_VFORK()` with simple `fork()` or `SAFE_FORK()`. 132 133### Shell tests 134* Use new https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#23-writing-a-testcase-in-shell[shell API] 135* Check coding style with `make check` 136 (more in https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#132-shell-coding-style[Shell coding style]) 137* If a test is a regression test it should include related kernel or glibc commits as a comment 138 139## LTP library 140For patchset touching library please check also 141https://github.com/linux-test-project/ltp/wiki/LTP-Library-API-Writing-Guidelines[LTP Library API Writing Guidelines]. 142