Submitting Xen Project Patches: Difference between revisions
Lars.kurth (talk | contribs) (→Sending the patches to the list: Added a section on how to use the workflow on other trees) |
Lars.kurth (talk | contribs) (→Using add_maintainers.pl (or get_maintainer.pl) from outside of xen.git: Correct template) |
||
| Line 342: | Line 342: | ||
L: xen-devel@lists.xenproject.org |
L: xen-devel@lists.xenproject.org |
||
S: Supported |
S: Supported |
||
F: * |
|||
F: */ |
F: */ |
||
</syntaxhighlight> |
</syntaxhighlight> |
||
Revision as of 07:49, 16 August 2019
This document assumes that you have cloned xen.git (or a similar repository) and that you have some development patches ready to submit. If you are not familiar with git and Xen Development branches, read the following documents
- Xen Project Repositories
- Managing Xen Patches with Git
- Managing Xen Patches with StGit
- Managing Xen Patches with_Git-series
This document outlines the code submission process primarily to the xen-devel@ mailing list. The process does also apply to other mailing list based subprojects: specific information related to these subprojects can be found at the bottom of this document.
What is in a patch and a patch series?
Changes to the Xen Project are submitted to a mailing list either as an individual patch or as a patch series. A patch series is a series of patches that are related and broken into a logical series of individual patches.
Patches, if accepted, will turn into commits in the git source tree. However, frequently multiple versions of patches will have to be posted, before the patch is accepted.
There are three groups of people to think about when writing the patch and the commit message:
- Reviewers on the mailing list, who are trying to understand what your patch does, if it's correct, and what side effects it will have.
- People skimming through changesets deciding if a patch is important for them to look at for backporting, review, implications on out-of-tree patches, &c.
- Someone in the perhaps distant future, who is trying to understand why the code is the way it is.
Try to make your patches with all of these people in mind.
What is in a patch?
A patch is an e-mail, which is generated from a single commit in your git tree and augmented by using a tool (e.g. git-format, add_maintainers.pl, git-send-email) which may prompt you to edit the final e-mail manually.
It is important to note, that your patch will inherit the following information from the git commit message on which it is based, assuming you recorded the information in the commit message at commit time
- Title and description
- Signed-off-by
- Other *-by tags such as Reported-by
- CC lists of reviewers - these can be added automatically later
- Change log and other information that is useful for the reviewer
Title and description of the patch
- The first line the top of the patch should contain a short description of what the patch does, and hints as to what code it touches. It typically contains a prefix, such as xen/x86:, xen/arm:, tools:, etc. Note that maintainers may propose a different prefix or change it at commit-time if it isn't correct
- The description should be useful for both the reviewers and people in the future trying to understand what the patch does. It can be as short as
Code cleanup -- no functional changes, or as long as it takes to accurately describe the bug you are trying to solve or the new functionality you are introducing.
The description should be wrapped to a maximum of 80 columns, unless it includes an error message which is longer than 80 characters.
Signed-off-by
All patches to the Xen Project code base must include the line Signed-off-by: your_name <your_email> at the end of the change description.
|
It is important that the e-mail address reflects the copyright holder of the code.
For example, if you work for a company called FOOBAR and developed the code on work-time, your employer owns the copyright for the change. Thus you must use an email address that includes @FOOBAR. If the code was developed on your own time, you can use a private email address. |
This is required and indicates that you certify the patch under the "Developer's Certificate of Origin" which states:
Developer's Certificate of Origin 1.1
By making a contribution to this project, I certify that:
(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or
(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source
license and I have the right under that license to submit that
work with modifications, whether created in whole or in part
by me, under the same open source license (unless I am
permitted to submit under a different license), as indicated
in the file; or
(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.
(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.Other *-by tags
You may add the following git tags before or after the signed-off-by tag:
- Reported-by: is used to credit someone who found the bug that the patch attempts to fix
- Tested-by: is used to indicate that the listed person applied the patch and found it to have the desired effect.
Later in the review process, your patch or patch series may accumulate tags, such as
- Acked-by: a maintainer of the patch reviewed the patch and found it is good to go in.
- Reviewed-by: another community member, a designated reviewer or a maintainer of another component reviewed the patch
Note that if there are several revisions of a patch, you ought to copy tags that have accumulated during the review. For example, if person A and person B added a Reviewed-by: tags to v1 of your patch, include it into v2 of your patch. If you make substantial changes after certain tags were already applied, you will want to consider which ones are no longer applicable (and may require re-providing).
CC lists of the maintainers or reviewers for the patch
- Always send a copy of the patch to (via CC) the maintainer of the code you are modifying. The maintainers and reviewers are listed in the MAINTAINERS file at the top of the Xen tree. If no maintainer is listed then the maintainers listed under THE REST in the MAINTAINERS file
- Note that if you follow the steps described in this document, the add_maintainers.pl/get_maintainer.pl scripts will add the correct people automatically
- If any people who were not on the CC list for an earlier review, but have reviewed your patch, you may want to manually add them (via CC) to the CC list.
Change log (for patch revisions >= 2)
Changes to previous versions of a patch are recorded in the patch commit message after the CC lists. These are required to be very detailed and will help the reviewer to check whether changes have been addressed. An example of a change log can be found below (the actual patch for this example is here).
Changes in v4:
- ARM -> Arm
- libxl__memory_policy_to_xc -> libxl__arch_memory_policy_to_xc
- keep Arm policies together
Changes in v3:
- s/nGRE/nGnRE/g
- s/LIBXL_MEMORY_POLICY_ARM_DEV_NGRE/LIBXL_MEMORY_POLICY_ARM_DEV_NGNRE/g
- s/arm_devmem/arm_dev_nGnRE/g
- s/arm_memory/arm_mem_WB/g
- improve commit message
- improve man page
- s/MEMORY_POLICY_X86_UC/MEMORY_POLICY_X86_UC_MINUS/g
- s/x86_uc/x86_UC_minus/g
- move security support clarification to a separate patch
Changes in v2:
- add #define LIBXL_HAVE_MEMORY_POLICY
- ability to part the memory policy parameter even if gfn is not passed
- rename cache_policy to memory policy
- rename MEMORY_POLICY_DEVMEM to MEMORY_POLICY_ARM_DEV_nGRE
- rename MEMORY_POLICY_MEMORY to MEMORY_POLICY_ARM_MEM_WB
- rename memory to arm_memory and devmem to arm_devmem
- expand the non-security support status to non device passthrough iomem
configurations
- rename iomem options
- add x86 specific iomem optionInformation that is useful for the reviewer, which you do not want in the final commit messages
You can manually add additional information that you do not want to appear in the commit message, after the maintainers list. An example can be found in this patch.
A version number and subject prefix
Each patch contains a revision number, or a subject prefix. For example, a patch email may contain [RFC PATCH v2] or [PATCH v3 4/11] indicating that the patch is part of a patch series.
Example patch
The example patch here that resulted in the following commit.
vif-common.sh: Have iptables wait for the xtables lock permalink
iptables has a system-wide lock on the xtables. Strangely though, in
the case of two concurrent invocations, the default is for the
instance not grabbing the lock to exit out rather than waiting for it.
This means that when starting a large number of guests in parallel,
many will fail out with messages like this:
2017-05-10 11:45:40 UTC libxl: error: libxl_exec.c:118: libxl_report_child_exitstatus: /etc/xen/scripts/vif-bridge remove [18767] exited with error status 4
2017-05-10 11:50:52 UTC libxl: error: libxl_exec.c:118: libxl_report_child_exitstatus: /etc/xen/scripts/vif-bridge offline [1554] exited with error status 4
In order to instruct iptables to wait for the lock, you have to
specify '-w'. Unfortunately, not all versions of iptables have the
'-w' option, so on first invocation check to see if it accepts the -w
command.
Reported-by: Antony Saba <aws...@gmail.com>
Signed-off-by: George Dunlap <geor...@citrix.com>
---
CC: Ian Jackson <ian....@citrix.com>
CC: Wei Liu <wei....@citrix.com>
---
tools/hotplug/Linux/vif-common.sh | 38 +++++++++++++++++++++++++++++++++++---
1 file changed, 35 insertions(+), 3 deletions(-)
diff --git a/tools/hotplug/Linux/vif-common.sh
b/tools/hotplug/Linux/vif-common.sh
index 6e8d584..29cd8dd 100644
--- a/tools/hotplug/Linux/vif-common.sh
...
- Lines 1 to 15: A good patch description answers the following questions
- What is the situation
- Why is the current situation a problem
- How does this patch fix the problem
- Anything else the reviewers need to know to understand the patch
- Lines 17 to 18: Contains the *by tags including the mandatory Signed-off-by tag
- Lines 20 to 21: Contains CC statements of the maintainers. Note that these can be added manually, but if you follow the steps below these will be added automatically
- Before Line 22: added in lines before the
--- - After Line 22: This is the actual code patch
What is in a patch series?
A patch series is a sequence of threaded emails, which are generated from multiple git commits (typically the last N ones) in your git tree and augmented by using a tool (e.g. git-format, add_maintainers.pl, git-send-email) which may prompt you to edit the final email manually.
Patch series can be anything from bug fixes or small improvements that are too big for an individual patch or encompass different components (e.g. hypervisor, toolstack change and a docs change) to new features with 10s of patches spanning multiple components.
In addition to the individual patches, which describe the patches making up the series, a patch series must contain a cover letter as the first email in the series. If you want any of the automated tooling which we use on xen-devel@ to work, the patch series must be threaded, with the cover letter the parent and each individual patch being a child of the cover letter.
Break down your patches appropriately
- Each patch in a patch series should preferably do one logical thing (or one related set of things). The goal is for reviewers to understand the change that each patch is making with a minimum of effort.
- No patch should ever break existing functionality.
- Never fix bugs in one of your patches by changing it in a later patch; go back and change the patch with the bug in it.
- Don't mix clean-up patches (which make things look prettier or refactor but don't change functionality) with code-change patches. However, e.g. style corrections on lines changed anyway are generally accepted (and often welcomed). Clean-up patches should be clearly marked as having no functional changes.
Cover letter
Cover letters are an important part of a patch series. The Xen Project has high standards when it comes to large and complex series. The cover letter should cover the following information
- It should contain an explanation of what you are trying to achieve
- It should explain as to why the series is needed (if it is not obvious)
- How the series achieves your goal
- It should list any pre-conditions and major assumptions you have made
- It should refer to any specifications, design documents, previous discussion on mailing lists and other relevant information
The amount of detail required depends on the complexity and size of the patch series. The larger and more complex the series, the more information is required. The goal of the information is to make it easier for the code reviewer to understand your series. The more time you invest in preparing the series’ cover letter, the easier it will be for a reviewer to understand your code.
However, cover letters do not get committed to the git tree: if the cover letter contains information that should be in the git tree, you may want to create a new document under docs/designs, docs/features or docs/specs instead as part of the series and refer to it from the series.
Change log (for patch series revisions >= 2)
The end of the cover letter is also the location where you would typically track changes between different versions of the patch series. The format is identical to change logs of individual patches (see here), but a series change log is much less detailed and more high-level than for a patch. The key purpose is to point reviewers to areas of the series which need to be revisited.
Providing a git branch
If you send a large patch series, it is recommended to include a branch in your public git repository in your cover letter, so that reviewers can pull directly from that branch.
If you do not have a public repository, you can fork the Xen Project’s GitHub or GitLab mirrors at https://github.com/xen-project/xen and https://gitlab.com/xen-project/xen into your own project space and push your branch to your own fork and provide a link to that branch. If you are a regular contributor to the Xen Project, you can request to host personal repositories at xenbits by sending an email to community dot manager at xenproject dot org.
Sending the patches to the list
The xen-devel mailing list is moderated for non-subscribers. It is not mandatory to subscribe but it can help avoid this delay. It is possible to subscribe and then disable delivery in the mailman options so as to avoid moderation but not receive list traffic if that is what you would prefer.
Setting up git send-email
The most robust way to send files is by using the git send-email command. (If you're using mercurial, please see our Submitting_Xen_Patches_-_mercurial).
To do this, first set configure your information:
git config --global sendemail.from "YOUR NAME <user@example.org>"
git config --global sendemail.smtpserver imap.example.org
git config --global sendemail.smtpuser USER
# depending on your config you may also set:
git config --global sendemail.smtpencryption tls
If you don't want to enter password for your SMTP server all the time:
git config --global sendemail.smtppass = PASS
Sending a Patch Series
Step 1: Create patches using git format-patch
Git format patch allows you to create and save formatted patches in a directory. By default it will create them in the root of your git directory, but you probably want to direct this into a ../patches/feature-version directory (in the examples below ../patches/feature-v2 would contain only v2 of that series). It is also possible to store several versions of a patch, e.g. v1, v2, etc in the same ../patches/feature directory). Let's say the last two commits of your head are part of your series you want to send out. In this case, the command line would look like
$ git format-patch --reroll-count=2 --thread --cover-letter -o ../patches/feature-v2 -2
This will create 3 files, such as
v2-0000-cover-letter.patch v2-0002-Patch-to-do-bar.patch v2-0001-Patch-to-do-foo.patch
Notes
- You will need to edit the subject and body of
v2-0000-cover-letter.patch - You must always use the --thread and --cover-letter options. If you omit --thread, any automatic tooling and patch checking that is triggered by sending a mail to one of our mailing lists may not work without using this option.
Other useful options include:
--rfc: generates subject lines such as[RFC PATCH ...]--subject-prefix=Subject-Prefix: e.g. example values arePATCH RESEND,RFC PATCH RESEND(ignores--rfc) ,PATCH for-4.11, etc.
Step 2: Use add_maintainers.pl (or get_maintainer.pl)
Option 1: use add_maintainers.pl
$ ./scripts/add_maintainers.pl -d ../patches/feature-v2
Then follow the instructions (which are essentially Step 3, with correct command line options based on what you pass to add_maintainers.pl). Note that the add_maintainers.pl script works around a limitation of git send-email ... --cc-cmd="./scripts/get_maintainer.pl" ..., which does not automatically update the CC list of your cover letter.
Other useful options include:
--reroll-count|-v(e.g.-v 2): If you store your patches in one directory with different versions in it generated by--reroll-count=2you want to use this option--patchcc|-p LOCATION: Inserts CC's into a specific location (see--help) to *.patch files--covercc|-c LOCATION: Inserts CC's into a specific location (see--help) to the cover letter--tags|-t: Adds people who have *-by tags to the mail header (git send-emaildoes not do this itself)--tagscc: Adds people who have *-by tags to the CC list--arg|-a(e.g.-a "<argument1 with space>" -a <argument2> ...for arguments you want to pass to./scripts/get_maintainer.pl)
Common LOCATION combinations include:
-p commit: copy CC blocks into the *.patch body (CC's will become part of the commit message)-p none -c header: copy CC blocks from *.patch files in the cover letter without modifying *.patch files. This is useful in rerolled patches, where you do not want to modify the CC blocks.-p ccbodybehaves similarly, only that any new CCs that may come from an updated MAINTAINERS file or from new files that have been added to *.patch files will be added to *.patch files.-p comment -c end: copy CC blocks after the *.patch body (CC's will not be committed) and into the body of the cover letter
Option 2: use get_maintainer.pl manually
Foreach <v2 patchfile> in ../patches/feature (with XXXX > 0000): - Run: ./scripts/get_maintainer.pl -f <v2 patchfile> - Review the e-mail list and prefix each line with Cc: - Copy the e-mail CC block into patchfile - Do anything else you want to do manually For v2-0000-cover-letter.patch take the superset of all the files generated previously and merge, then copy the e-mail CC block into 0000-cover-letter.patch
Step 3: Send patches using git send-email
Send the patches using
$ git send-email -to xen-devel@lists.xenproject.org ../patches/feature-v2/*.patch
Other useful options include:
--dry-run--ccif you need to CC additional reviewers (e.g. from within your team)
Sending an individual patch
You can use the same workflow as outlined in the previous section, without generating a cover letter. In this case, the git-format-patch command line in step 1 would look like
$ git format-patch --thread -o ../patches/bugfix-v2 -1
This is followed by steps 2 and 3. However, for single patches without a cover letter, using git-send-email alone, is quite a reasonable option. In this case, you can use the following command line, which will get the CC list from the ./scripts/get_maintainer.pl script, which allows you to fold all 3 steps into 1.
$ git send-email --to xen-devel@lists.xenproject.org --cc-cmd="./scripts/get_maintainer.pl" -1
Other useful options include:
--dry-runwill go through all the motions of preparing the patchbomb, but instead of sending a mail, will just output the mails it would have sent. Useful for testing.--ccif you need to CC additional reviewers (e.g. from within your team)--subject-prefixwill allow you to change the prefix from[PATCH]to something else. Examples may include--subject-prefx="PATCH RFC"if you just want to get feedback on a patch series (Request For Comments), or--subject-prefix="PATCH v3"for the 3rd version of your patch series (see also--reroll-count=N).
Using add_maintainers.pl (or get_maintainer.pl) from outside of xen.git
|
This capability outlined in this section is not yet available in xen.git. The relevant patches can be found here. |
You can use add_maintainers.pl or get_maintainer.pl on any Xen Project git repository with a compatible MAINTAINERS file in the root of its tree. An example is livepatch-build-tools.git. In this case simply replace ./scripts/add_maintainers.pl or ./scripts/get_maintainer.pl with the full path to the script. A minimum template for such a MAINTAINERS file can be found below.
This file follows the same conventions as outlined in
xen.git:MAINTAINERS. Please refer to the file in xen.git
for more information.
THE REST
M: MAINTAINER1 <maintainer1@email.com>
M: MAINTAINER2 <maintainer2@email.com>
L: xen-devel@lists.xenproject.org
S: Supported
F: *
F: */
Review, Rinse & Repeat
After posting your patches you will hopefully see some response in the form of comments, patch review and eventually commit.
Code Review
The form of the review may often be quite direct and to the point which may be daunting for some people. However bear in mind that detailed criticism of a patch usually means that the reviewer is interested in your patch and wants to see it go in!
Once you have addressed any review you should normally resend the patch. It is normally expected that you address all review comments before reposting. This often means code changes in your patch but could also mean simply responding to the review comments explaining you reasoning or giving reasons why something should be the way it is.
You should also rebase your change against the project's development tree before sending out a new version, such that when it is approved it applies cleanly. The only reason not to do that is if you don't expect it to be approved, and want feedback on what you have. In that case you can do the rebase later.
The relevant command line options in git-format-patch, ./scripts/add_maintainers.pl and git-send-email (if used standalone) for reposting new revisions is
--reroll-count|-v
Highlight changes in the new version
When resending a patch you should normally include a note of the changes between the current and last version of the patch. Common practice is to include these notes after your Signed-off-by separated by a triple-dash (---). This indicates patch commentary specific to the posting which need not be included in the final changelog (although you should also remember to update the changelog if necessary). You should also include a "V2" (V3, V4 etc) tag in the subject line (if you are using the git send-email command then the --reroll-count=N option is helpful here, or for older git versions --subject-prefix='PATCH vN').
Update tags
If someone replies to your patch with a tag of the form Acked-by: <Developer>, Reviewed-by:, Tested-by: etc then, assuming you have not significantly reworked the patch, you should include these tags in any reposting after your own Signed-off-by line. This lets people know that the patch has been seen and that someone approves of it and also serves to prevent reviewers wasting time re-reviewing a patch which is not significantly different to last time. The developers with commit access also like to see postings with such tags since it means they are likely to be much easier to deal with and commit.
An example of a new Patch Version
An example of a resend of the example patch from above might be:
Subject: [PATCH v2] foobar: Add a new trondle calls
Add a some new trondle calls to the foobar interface to support
the new zot feature.
Signed-off-by: Joe Smith <joe.smith@citrix.com>
Acked-by: Jane Doe <jane.doe@example.com>
---
Changed since v1:
* fix coding style
* be sure to treadle the trondle in the error case.
diff -r 63531e640828 tools/libxc/Makefile
--- a/tools/libxc/Makefile Mon Dec 07 17:01:11 2009 +0000
+++ b/tools/libxc/Makefile Mon Dec 21 11:45:00 2009 +0000
...
Resending Patches
If you do not get any response to your patch or you got lots of Acked-by's but the patch has not been committed (remember that reviewers and maintainers are busy people too and sometimes things may fall through the cracks) then after some time, perhaps 2-4 weeks (guidelines), you should resend the patch, perhaps including [RESEND] in the subject line to alert people that the last mail was dropped. Before resending you should:
- Check that the patch has not been applied to the staging branch, since committers do not always send a notification when they apply a patch.
- Consider if there is anything you can do to improve the commit message or subject line of your patch to better attract the attention of the relevant people.
Remember to include any Acked-by/Reviewed-by which you received in response to the previous post.
How to know when a patch has been committed
Changes committed to Xen Project by the committers are immediately available in the "staging" branch of the main xen.git tree. They are then automatically tested, and if the tests pass the changes are propagated to the "master" branch.
After your patch is committed
If your patch turns out to break something you will be expected to respond promptly to help diagnose and fix the problem. This assumes that you in particular keep an eye on the osstest flight reports sent to xen-devel. If it can't be fixed quickly, your change may be reverted.
How to Generate and Submit a Patch to Qemu-Xen
Xen Project uses QEMU as device model, that is the software component that takes care of emulating devices (like the network card) for HVM guests. Two QEMU trees, both using git for revision control, are currently in use by xen.git:
- qemu-xen-traditional: old and stable tree, only bug fixes are accepted in this tree at the moment;
- qemu-xen: new tree used for development, based on the latest stable branch of Upstream QEMU, that currently is available at qemu git repo. See QEMU Upstream on how to manually build it and use it to run VMs.
New features should be developed again the latest upstream QEMU first, see http://wiki.qemu.org/Contribute/SubmitAPatch, then upon request they can be backported to qemu-xen. Only patches that have been acked and committed to qemu.git are eligible to be backported to qemu-xen. In order to request a backport, send an email to xen-devel@lists.xen.org, specifying the original commit id in qemu.git and the reason why the patch should be backported to qemu-xen.
How to Generate and Submit a Xen Project Patch to the Linux Tree
Like Xen Project, Linux uses the git SCM. The Linux tree contains documentation on submitting patches, in Documentation/SubmittingPatches, as well as a script (scripts/checkpatch.pl) that ensures your patch is in Linux house style. Running git apply --check on your patch can reveal merge errors.
Patches should be emailed to xen-devel@lists.xenproject.org, linux-kernel@vger.kernel.org, and any relevant Linux maintainers (which can be found by running scripts/get_maintainer.pl on your patch).
The FreeBSD project uses svn for it's master source tree, although git clones are also available. Checkout the code from either the svn or git mirror and remember to always create your patch against the HEAD branch. It can be backported to stable versions afterwards, but it needs to be initially committed to HEAD.
The best way to get your patches reviewed is to use Phabricator and assign them to the relevant maintainer (check MAINTAINERS in the source tree), for Xen related code that would be royger.
How to Generate and Submit a Xen Project Patch to MiniOS and Unikraft
MiniOS uses the following git tree for development: mini-os.git. Patches are submitted to minios-devel@.
Unikraft uses several git trees under the unikraft namespace for development: unikraft/. Patches are submitted to minios-devel@ and are marked with a UNIKRAFT prefix (or with a library name prefix), such that they can be differentiated from MiniOS patches. A detailed description of the Unikraft convention can be found in the Unikraft base repository unikraft/unikraft.git: CONTRIBUTING.md.
