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