'Re: [win-pv-devel] [Xen-devel] [PATCH v4 5/7] Add Code Review Guide'

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       win-pv-devel
Subject:    Re: [win-pv-devel] [Xen-devel] [PATCH v4 5/7] Add Code Review Guide
From:       Jürgen Groß <jgross () suse ! com>
Date:       2020-01-06 7:03:45
Message-ID: a6b3d3d7-5c56-0a0c-dc8c-39c8ed4d1197 () suse ! com
[Download RAW message or body]

On 30.12.19 20:32, Lars Kurth wrote:
> From: Lars Kurth <lars.kurth@citrix.com>
> 
> This document highlights what reviewers such as maintainers and committers look
> for when reviewing code. It sets expectations for code authors and provides
> a framework for code reviewers.
> 
> Changes since v3
> * Added example under *Workflow from a Reviewer's Perspective* section
> * Fixed typos in text introduced in v2
> 
> Changes since v2 (introduced in v2)
> * Extend introduction
> * Add "Code Review Workflow" covering
> - "Workflow from a Reviewer's Perspective"
> - "Workflow from an Author's Perspective"
> - "Problematic Patch Reviews"
> * Wrap to 80 characters
> * Replace inline links with reference links to make
> wrapping easier
> 
> Signed-off-by: Lars Kurth <lars.kurth@citrix.com>
> ---
> Cc: minios-devel@lists.xenproject.org
> Cc: xen-api@lists.xenproject.org
> Cc: win-pv-devel@lists.xenproject.org
> Cc: mirageos-devel@lists.xenproject.org
> Cc: committers@xenproject.org
> ---
> code-review-guide.md | 313 +++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 313 insertions(+)
> create mode 100644 code-review-guide.md
> 
> diff --git a/code-review-guide.md b/code-review-guide.md
> new file mode 100644
> index 0000000..b2c08d2
> --- /dev/null
> +++ b/code-review-guide.md
> @@ -0,0 +1,313 @@
> +# 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

duplicate "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](#problems)
> +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:
> +* [Communication Best Practice][1]
> +* [Resolving Disagreement][2]
> +* [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 efﬁcient 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][A]<br>
> +  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][B]<br>
> +  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][C]<br>
> +  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][D]<br>
> +  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][E].
> +
> +## 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 reorganisations
> +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 subsytems (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. Reorganisations
> +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 reorganisations 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][H] 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** reorganisations 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
> +
> +### <a name="problems"></a>Problematic Patch Reviews
> +
> +A typical waterfall software development process is sequential with the
> +following steps: define requirements, analyse, 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][D]** 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

s/read the//

> +or read the slides:
> +* [Patch Review for Non-Maintainers slides][F]
> +* [Patch Review for Non-Maintainers recording - 20"][G]
> +
> +[1]: communication-practice.md
> +[2]: resolving-disagreement.md
> +[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
> +[D]: 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
> 

Juergen

_______________________________________________
win-pv-devel mailing list
win-pv-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/win-pv-devel

[prev in list] [next in list] [prev in thread] [next in thread] 