From patchwork Thu Sep 26 19:39:22 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Lars Kurth X-Patchwork-Id: 11163727 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 128371747 for ; Fri, 27 Sep 2019 04:20:16 +0000 (UTC) Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id EC75120863 for ; Fri, 27 Sep 2019 04:20:15 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org EC75120863 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=xenproject.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=xen-devel-bounces@lists.xenproject.org Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.89) (envelope-from ) id 1iDhiP-0003F3-Jn; Fri, 27 Sep 2019 04:18:45 +0000 Received: from us1-rack-iad1.inumbo.com ([172.99.69.81]) by lists.xenproject.org with esmtp (Exim 4.89) (envelope-from ) id 1iDZdE-0001QW-8Y for xen-devel@lists.xenproject.org; Thu, 26 Sep 2019 19:40:52 +0000 X-Inumbo-ID: 606c9456-e095-11e9-97fb-bc764e2007e4 Received: from mail.xenproject.org (unknown [104.130.215.37]) by localhost (Halon) with ESMTPS id 606c9456-e095-11e9-97fb-bc764e2007e4; Thu, 26 Sep 2019 19:39:38 +0000 (UTC) Received: from xenbits.xenproject.org ([104.239.192.120]) by mail.xenproject.org with esmtp (Exim 4.89) (envelope-from ) id 1iDZbx-0002Gj-GG; Thu, 26 Sep 2019 19:39:33 +0000 Received: from localhost ([127.0.0.1] helo=MacBook-Pro-2.Home) by xenbits.xenproject.org with esmtp (Exim 4.89) (envelope-from ) id 1iDZbx-0007uS-7e; Thu, 26 Sep 2019 19:39:33 +0000 From: Lars Kurth To: xen-devel@lists.xenproject.org Date: Thu, 26 Sep 2019 20:39:22 +0100 Message-Id: <97e3adf75cf71ba39e702d4cab23236ada8d5a6c.1569525222.git.lars.kurth@citrix.com> X-Mailer: git-send-email 2.13.0 In-Reply-To: References: MIME-Version: 1.0 In-Reply-To: References: X-Mailman-Approved-At: Fri, 27 Sep 2019 04:18:43 +0000 Subject: [Xen-devel] [PATCH v2 4/6] Add Code Review Guide X-BeenThere: xen-devel@lists.xenproject.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Cc: Lars Kurth , xen-api@lists.xenproject.org, minios-devel@lists.xenproject.org, committers@xenproject.org, mirageos-devel@lists.xenproject.org, win-pv-devel@lists.xenproject.org Errors-To: xen-devel-bounces@lists.xenproject.org Sender: "Xen-devel" From: Lars Kurth 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. Signed-off-by: Lars Kurth --- 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 | 125 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 125 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..8639431 --- /dev/null +++ b/code-review-guide.md @@ -0,0 +1,125 @@ +# 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. + +This document does **not cover** the following topics: +* [Communication Best Practice](communication-practice.md) +* [Resolving Disagreement](resolving-disagreement.md) +* [Patch Submission Workflow](https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches) +* [Managing Patch Submission with Git](https://wiki.xenproject.org/wiki/Managing_Xen_Patches_with_Git) + +## 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](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=blob;f=CODING_STYLE) + and [tools/libxl/CODING_STYLE](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=blob;f=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](https://xenbits.xen.org/gitweb/?p=xen.git;a=tree;f=docs) folder. +* When adding new features that have an impact on the end-user, + a contributor should include an update to the + [SUPPORT.md](https://xenbits.xen.org/gitweb/?p=xen.git;a=tree;f=docs) 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](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=tree;f=tools/tests) +or [xen/test](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=tree;f=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](https://gitlab.com/xen-project/xen/pipelines)
+ 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](https://xenbits.xenproject.org/docs/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](https://xenbits.xenproject.org/gitweb/?p=osstest.git;a=blob;f=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](https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches). + +## 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](https://www.slideshare.net/xen_com_mgr/xpdds19-keynote-patch-review-for-nonmaintainers-george-dunlap-citrix-systems-uk-ltd) +* [Patch Review for Non-Maintainers recording - 20"](https://www.youtube.com/watch?v=ehZvBmrLRwg)