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:
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 and tools/libxl/CODING_STYLE
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 folder.
When adding new features that have an impact on the end-user, a contributor should include an update to the SUPPORT.md 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 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
Cleanups and/or new Independent Helper Functions
Reorganizations
Headers, APIs, Documentation and anything which helps understand the substance of a series
The substance of the change
Cleanups of any infelicities introduced temporarily
Deleting old code
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.
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.