From: Stefano Stabellini <sstabellini@kernel.org>
To: Lars Kurth <lars.kurth@xenproject.org>
Cc: Lars Kurth <lars.kurth@citrix.com>,
xen-api@lists.xenproject.org, minios-devel@lists.xenproject.org,
committers@xenproject.org, mirageos-devel@lists.xenproject.org,
xen-devel@lists.xenproject.org,
win-pv-devel@lists.xenproject.org
Subject: Re: [Xen-devel] [PATCH v2 4/6] Add Code Review Guide
Date: Wed, 27 Nov 2019 16:54:05 -0800 (PST) [thread overview]
Message-ID: <alpine.DEB.2.21.1911271608510.27669@sstabellini-ThinkPad-T480s> (raw)
In-Reply-To: <97e3adf75cf71ba39e702d4cab23236ada8d5a6c.1569525222.git.lars.kurth@citrix.com>
[-- Attachment #1: Type: text/plain, Size: 7965 bytes --]
On Thu, 26 Sep 2019, 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.
I think the document is missing a couple of things:
- a simple one line statement that possibly the most important thing in
a code review is to indentify any bugs in the code
- an explanation that requests for major changes to the series should be
made early on (i.e. let's not change the architecture of a feature at
v9 if possible) I also made this comment in reply to patch #5. I'll
let you decide where is the best place for it.
> 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 | 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)<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](https://gitlab.com/xen-project/xen/pipelines)<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](https://xenbits.xenproject.org/docs/xtf/)<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](https://xenbits.xenproject.org/gitweb/?p=osstest.git;a=blob;f=README)<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](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)
> --
> 2.13.0
>
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
next prev parent reply other threads:[~2019-11-28 0:54 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-26 19:39 [Xen-devel] [PATCH v2 0/6] Code of Conduct + Extra Guides and Best Practices Lars Kurth
2019-09-26 19:39 ` [Xen-devel] [PATCH v2 1/6] Import v1.4 of Contributor Covenant CoC Lars Kurth
2019-10-07 11:06 ` George Dunlap
2019-10-07 11:27 ` Lars Kurth
2019-09-26 19:39 ` [Xen-devel] [PATCH v2 2/6] Xen Project Code of Conduct Lars Kurth
2019-09-26 19:39 ` [Xen-devel] [PATCH v2 3/6] Add Communication Guide Lars Kurth
2019-09-26 19:39 ` [Xen-devel] [PATCH v2 4/6] Add Code Review Guide Lars Kurth
2019-11-28 0:54 ` Stefano Stabellini [this message]
2019-11-28 10:09 ` Jan Beulich
2019-11-28 13:06 ` Lars Kurth
2019-11-28 13:37 ` Jan Beulich
2019-11-28 14:02 ` Lars Kurth
2019-11-28 18:20 ` [Xen-devel] [MirageOS-devel] " Rich Persaud
2019-11-29 1:39 ` Lars Kurth
2019-12-05 23:41 ` Lars Kurth
2019-12-06 9:51 ` [Xen-devel] " Jan Beulich
2019-12-09 11:02 ` Lars Kurth
2019-12-09 15:58 ` Lars Kurth
2019-11-28 18:12 ` Rich Persaud
2019-11-29 1:50 ` Lars Kurth
2019-11-28 18:19 ` Stefano Stabellini
2019-09-26 19:39 ` [Xen-devel] [PATCH v2 5/6] Add guide on Communication Best Practice Lars Kurth
2019-09-27 8:59 ` Jan Beulich
2019-09-27 9:53 ` Lars Kurth
2019-09-27 9:14 ` Jan Beulich
2019-09-27 10:17 ` Lars Kurth
2019-09-27 10:22 ` Lars Kurth
2019-09-27 14:19 ` Jan Beulich
2019-10-07 16:13 ` George Dunlap
2019-10-08 7:29 ` Jan Beulich
2019-11-28 1:06 ` Stefano Stabellini
2019-11-29 0:02 ` Lars Kurth
2019-10-07 15:29 ` George Dunlap
2019-11-28 0:57 ` Stefano Stabellini
2019-11-29 0:00 ` Lars Kurth
2019-09-26 19:39 ` [Xen-devel] [PATCH v2 6/6] Added Resolving Disagreement Lars Kurth
2019-11-28 0:56 ` Stefano Stabellini
2019-11-28 10:18 ` Jan Beulich
2019-11-28 18:50 ` Stefano Stabellini
2019-11-29 2:10 ` Lars Kurth
2019-11-29 1:42 ` Lars Kurth
2019-10-24 7:51 ` [Xen-devel] [PATCH v2 0/6] Code of Conduct + Extra Guides and Best Practices Felipe Huici
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=alpine.DEB.2.21.1911271608510.27669@sstabellini-ThinkPad-T480s \
--to=sstabellini@kernel.org \
--cc=committers@xenproject.org \
--cc=lars.kurth@citrix.com \
--cc=lars.kurth@xenproject.org \
--cc=minios-devel@lists.xenproject.org \
--cc=mirageos-devel@lists.xenproject.org \
--cc=win-pv-devel@lists.xenproject.org \
--cc=xen-api@lists.xenproject.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).