Code Review Guide ***************** This document highlights what reviewers such as maintainers and committers look for when reviewing your code. It sets expectations for code authors and provides a framework for code reviewers. Before we start, it is important to remember that the primary purpose of a a code review is to identify any bugs or potential bugs in the code. Most code reviews are relatively straight-forward and do not require re-writing the submitted code substantially. The document provides advice on how to structure larger patch series and provides pointers on code author's and reviewer's workflows. Sometimes it happens that a submitted patch series made wrong assumptions or has a flawed design or architecture. This can be frustrating for contributors and code reviewers. Note that this document does contain `a section `_ that provides suggestions on how to minimize the impact for most stake-holders and also on how to avoid such situations. This document does **not cover** the following topics: * :doc:`Communication Best Practice ` * :doc:`Resolving Disagreement ` * `Patch Submission Workflow <3_>`_ * `Managing Patch Submission with Git <4_>`_ What we look for in Code Reviews ================================ When performing a code review, reviewers typically look for the following things Is the change necessary to accomplish the goals? ------------------------------------------------ * Is it clear what the goals are? * Do we need to make a change, or can the goals be met with existing functionality? Architecture / Interface ------------------------ * Is this the best way to solve the problem? * Is this the right part of the code to modify? * Is this the right level of abstraction? * Is the interface general enough? Too general? Forward compatible? Functionality ------------- * Does it do what it’s trying to do? * Is it doing it in the most efficient way? * Does it handle all the corner / error cases correctly? Maintainability / Robustness ---------------------------- * Is the code clear? Appropriately commented? * Does it duplicate another piece of code? * Does the code make hidden assumptions? * Does it introduce sections which need to be kept **in sync** with other sections? * Are there other **traps** someone modifying this code might fall into? **Note:** Sometimes you will work in areas which have identified maintainability and/or robustness issues. In such cases, maintainers may ask you to make additional changes, such that your submitted code does not make things worse or point you to other patches are already being worked on. System properties ----------------- In some areas of the code, system properties such as * Code size * Performance * Scalability * Latency * Complexity * &c are also important during code reviews. Style ----- * Comments, carriage returns, **snuggly braces**, &c * See `CODING_STYLE <5_>`_ and `tools/libxl/CODING_STYLE <6_>`_ * No extraneous whitespace changes Documentation and testing ------------------------- * If there is pre-existing documentation in the tree, such as man pages, design documents, etc. a contributor may be asked to update the documentation alongside the change. Documentation is typically present in the `docs <7_>`_ folder. * When adding new features that have an impact on the end-user, a contributor should include an update to the `SUPPORT.md <8_>`_ file. Typically, more complex features require several patch series before it is ready to be advertised in SUPPORT.md * When adding new features, a contributor may be asked to provide tests or ensure that existing tests pass Testing for the Xen Project Hypervisor -------------------------------------- Tests are typically located in one of the following directories * **Unit tests**: `tools/tests <9_>`_ or `xen/test `_ Unit testing is hard for a system like Xen and typically requires building a subsystem of your tree. If your change can be easily unit tested, you should consider submitting tests with your patch. * **Build and smoke test**: see `Xen GitLab CI `_ Runs build tests for a combination of various distros and compilers against changes committed to staging. Developers can join as members and test their development branches **before** submitting a patch. * **XTF tests** (microkernel-based tests): see `XTF `_ XTF has been designed to test interactions between your software and hardware. It is a very useful tool for testing low level functionality and is executed as part of the project's CI system. XTF can be easily executed locally on xen.git trees. * **osstest**: see `README `_ Osstest is the Xen Projects automated test system, which tests basic Xen use cases on a variety of different hardware. Before changes are committed, but **after** they have been reviewed. A contributor’s changes **cannot be applied to master** unless the tests pass this test suite. Note that XTF and other tests are also executed as part of osstest. Patch / Patch series information -------------------------------- * Informative one-line changelog * Full changelog * Motivation described * All important technical changes mentioned * Changes since previous revision listed * Reviewed-by’s and Acked-by’s dropped if appropriate More information related to these items can be found in our `Patch submission Guide `_. Code Review Workflow ==================== This section is important for code authors and reviewers. We recommend that in particular new code authors carefully read this section. Workflow from a Reviewer's Perspective -------------------------------------- Patch series typically contain multiple changes to the codebase, some transforming the same section of the codebase multiple times. It is quite common for patches in a patch series to rely on the previous ones. This means that code reviewers review patches and patch series **sequentially** and **the structure of a patch series guides the code review process**. Sometimes in a long series, patches {1,2}/10 will be clean-ups, {3-6}/10 will be general reorganizations which don't really seem to do anything and then {7-10}/10 will be the substance of the series, which helps the code reviewer understand what {3-6}/10 were about. Generally there are no hard rules on how to structure a series, as the structure of a series is very code specific and it is hard to give specific advice. There are some general tips which help and some general patterns. Tips ^^^^ * Outline the thinking behind the structure of the patch series. This can make a huge difference and helps ensure that the code reviewer understands what the series is trying to achieve and which portions are addressing which problems. * Try and keep changes that belong to a subsystem together * Expect that the structure of a patch series sometimes may need to change between different versions of a patch series * **Most importantly**: Start small. Don't submit a large and complex patch series as the first interaction with the community. Try and pick a smaller task first (e.g. a bug-fix, a clean-up task, etc.) such that you don't have to learn the tools, code and deal with a large patch series all together for the first time. General Patterns ^^^^^^^^^^^^^^^^ If there are multiple subsystems involved in your series, then these are best separated out into **sets of patches**, which roughly follow the following seven categories. In other words: you would end up with **7 categories x N subsystems**. In some cases, there is a **global set of patches** that affect all subsystems (e.g. headers, macros, documentation) impacting all changed subsystems which ideally comes **before** subsystem specific changes. The seven categories typically making up a logical set of patches 1. Cleanups and/or new Independent Helper Functions 2. Reorganizations 3. Headers, APIs, Documentation and anything which helps understand the substance of a series 4. The substance of the change 5. Cleanups of any infelicities introduced temporarily 6. Deleting old code 7. Test code Note that in many cases, some of the listed categories are not always present in each set, as they are not needed. Of course, sometimes there are several patches describing **changes of substance**, which could be ordered in different ways: in such cases it may be necessary to put reorganizations in between these patches. If a series is structured this way, it is often possible to agree early on, that a significant portion of the changes are fine and to check these in independently of the rest of the patch series. This means that there is * Less work for authors to rebase * Less cognitive overhead for reviewers to review successive versions of a series * The possibility for different code reviewers to review portions of such large changes independently Trade-Offs ^^^^^^^^^^ * In some cases, following the general pattern above may create extra patches and may make a series more complex and harder to understand. * Crafting a more extensive cover letter will be extra effort: in most cases, the extra time investment will be saving time during the code review process. Verbosity is not the goal, but clarity is. Before you send a larger series in particular: try and put yourself into the position of a code reviewer and try to identify information that helps a code reviewer follow the patch series. * In cases where changes need to be back-ported to older releases, moving general cleanups last is often preferable: in such cases the **substance of the change** is back-ported, whereas general cleanups and improvements are not. Example ^^^^^^^ * `[PATCH v3 00/18] VM forking `_ is a complex patch series with an exemplary cover letter. Notably, it contains the following elements * It provides a description of the design goals and detailed description of the steps required to fork a VM. * A description of changes to the user interface * It contains some information about the test status of the series including some performance information. * It maps the series onto the categories listed above. As expected, not all categories are used in this case. However, the series does contain elements of **1** (in this case preparation to enable the functionality), **2** reorganizations and other non-functional changes that enable the rest of the series and **4** the substance of the series with additional information to make it easier for the reviewer to parse the series. Workflow from an Author's Perspective ------------------------------------- When code authors receive feedback on their patches, they typically first try to clarify feedback they do not understand. For smaller patches or patch series it makes sense to wait until receiving feedback on the entire series before sending out a new version addressing the changes. For larger series, it may make sense to send out a new revision earlier. As a reviewer, you need some system that helps ensure that you address all review comments. This can be tedious when trying to map a hierarchical e-mail thread onto a code-base. Different people use different techniques from using * In-code TODO statements with comment snippets copied into the code * To keeping a separate TODO list * To printing out the review conversation tree and ticking off what has been addressed * A combination of the above .. _code-review-problematic-patch-review: .. _problems: Problematic Patch Reviews ------------------------- A typical waterfall software development process is sequential with the following steps: define requirements, analyze, design, code, test and deploy. Problems uncovered by code review or testing at such a late stage can cause costly redesign and delays. The principle of `Shift Left`_ is to take a task that is traditionally performed at a late stage in the process and perform that task at earlier stages. The goal is to save time by avoiding refactoring. Typically, problematic patch reviews uncover issues such as wrong or missed assumptions, a problematic architecture or design, or other bugs that require significant re-implementation of a patch series to fix the issue. The principle of **Shift Left** also applies in code reviews. Let's assume a series has a major flaw: ideally, this flaw would be picked up in the **first or second iteration** of the code review. As significant parts of the code may have to be re-written, it does not make sense for reviewers to highlight minor issues (such as style issues) until major flaws have been addressed of the affected part of a patch series. In such cases, providing feedback on minor issues reviewers cause the code author and themselves extra work by asking for changes to code, which ultimately may be changed later. To make it possible for code reviewers to identify major issues early, it is important for code-authors to highlight possible issues in a cover letter and to structure a patch series in such a way that makes it easy for reviewers to separate difficult and easy portions of a patch series. This will enable reviewers to progress uncontroversial portions of a patch independently from controversial ones. Reviewing for Patch Authors --------------------------- The following presentation by George Dunlap, provides an excellent overview on how we do code reviews, specifically targeting non-maintainers. As a community, we would love to have more help reviewing, including from **new community members**. But many people * do not know where to start, or * believe that their review would not contribute much, or * may feel intimidated reviewing the code of more established community members The presentation demonstrates that you do not need to worry about any of these concerns. In addition, reviewing other people's patches helps you * write better patches and experience the code review process from the other side * and build more influence within the community over time Thus, we recommend strongly that **patch authors** read the watch the recording or read the slides: * `Patch Review for Non-Maintainers slides `_ * `Patch Review for Non-Maintainers recording - 20" `_ .. _3: https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches .. _4: https://wiki.xenproject.org/wiki/Managing_Xen_Patches_with_Git .. _5: https://xenbits.xenproject.org/gitweb/?p=xen.git;a=blob;f=CODING_STYLE .. _6: https://xenbits.xenproject.org/gitweb/?p=xen.git;a=blob;f=tools/libxl/CODING_STYLE .. _7: https://xenbits.xenproject.org/gitweb/?p=xen.git;a=tree;f=docs .. _8: https://xenbits.xenproject.org/gitweb/?p=xen.git;a=blob;f=SUPPORT.md .. _9: https://xenbits.xenproject.org/gitweb/?p=xen.git;a=tree;f=tools/tests .. _A: https://xenbits.xenproject.org/gitweb/?p=xen.git;a=tree;f=xen/test .. _B: https://gitlab.com/xen-project/xen/pipelines .. _C: https://xenbits.xenproject.org/docs/xtf/ .. _D: https://xenbits.xenproject.org/gitweb/?p=osstest.git;a=blob;f=README .. _E: https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches .. _Shift Left: https://devopedia.org/shift-left .. _F: https://www.slideshare.net/xen_com_mgr/xpdds19-keynote-patch-review-for-nonmaintainers-george-dunlap-citrix-systems-uk-ltd .. _G: https://www.youtube.com/watch?v=ehZvBmrLRwg .. _H: https://lists.xenproject.org/archives/html/xen-devel/2019-12/threads.html#02097