Difference between revisions of "Submitting Xen Project Patches"
(→Using add_maintainers.pl (or get_maintainer.pl) from outside of xen.git)
m (→Using add_maintainers.pl (or get_maintainer.pl) from outside of xen.git)
|Line 356:||Line 356:|
Revision as of 10:06, 4 September 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.
- 1 What is in a patch and a patch series?
- 1.1 What is in a patch?
- 1.1.1 Title and description of the patch
- 1.1.2 Signed-off-by
- 1.1.3 Other *-by tags
- 1.1.4 CC lists of the maintainers or reviewers for the patch
- 1.1.5 Change log (for patch revisions >= 2)
- 1.1.6 Information that is useful for the reviewer, which you do not want in the final commit messages
- 1.1.7 A version number and subject prefix
- 1.1.8 Example patch
- 1.2 What is in a patch series?
- 1.1 What is in a patch?
- 2 Sending the patches to the list
- 2.1 Setting up git send-email
- 2.2 Sending a Patch Series
- 2.3 Sending an individual patch
- 2.4 Using add_maintainers.pl (or get_maintainer.pl) from outside of xen.git
- 3 Review, Rinse & Repeat
- 4 Workflow for related repositories and projects
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
- 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.
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.
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.
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 option
Information 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.
1 vif-common.sh: Have iptables wait for the xtables lock permalink 2 3 iptables has a system-wide lock on the xtables. Strangely though, in 4 the case of two concurrent invocations, the default is for the 5 instance not grabbing the lock to exit out rather than waiting for it. 6 This means that when starting a large number of guests in parallel, 7 many will fail out with messages like this: 8 9 2017-05-10 11:45:40 UTC libxl: error: libxl_exec.c:118: libxl_report_child_exitstatus: /etc/xen/scripts/vif-bridge remove  exited with error status 4 10 2017-05-10 11:50:52 UTC libxl: error: libxl_exec.c:118: libxl_report_child_exitstatus: /etc/xen/scripts/vif-bridge offline  exited with error status 4 11 12 In order to instruct iptables to wait for the lock, you have to 13 specify '-w'. Unfortunately, not all versions of iptables have the 14 '-w' option, so on first invocation check to see if it accepts the -w 15 command. 16 17 Reported-by: Antony Saba <aws...@gmail.com> 18 Signed-off-by: George Dunlap <geor...@citrix.com> 19 --- 20 CC: Ian Jackson <ian....@citrix.com> 21 CC: Wei Liu <wei....@citrix.com> 22 --- 23 tools/hotplug/Linux/vif-common.sh | 38 +++++++++++++++++++++++++++++++++++--- 24 1 file changed, 35 insertions(+), 3 deletions(-) 25 26 diff --git a/tools/hotplug/Linux/vif-common.sh 27 b/tools/hotplug/Linux/vif-common.sh 28 index 6e8d584..29cd8dd 100644 29 --- a/tools/hotplug/Linux/vif-common.sh 30 ...
- 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-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 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
To do this, first set configure your information:
git config --global sendemail.from "YOUR NAME <email@example.com>" 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
- You will need to edit the subject and body of
- 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 are
RFC PATCH RESEND(ignores
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:
-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
-a "<argument1 with space>" -a <argument2> ...for arguments you want to pass to
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 <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 firstname.lastname@example.org ../patches/feature-v2/*.patch
Other useful options include:
--ccif you need to CC additional reviewers (e.g. from within your team)
Submitting patches against point releases, such as Xen 4.10
When you develop against older Xen versions you are likely going to use an outdated version of the MAINTAINERS file. Typically only the files on staging or master are up-to-date. To work around this, use the following slightly modified workflow
$ git format-patch ... $ checkout master $ ./scripts/add_maintainers.pl -d ... $ checkout <original branch> $ git send-email ...
This ensures that you use the latest set of tools and the latest MAINTAINERS file
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 email@example.com --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
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
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/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 <firstname.lastname@example.org> M: MAINTAINER2 <email@example.com> L: firstname.lastname@example.org S: Supported F: * F: */ V: xen-maintainers-1
Review, Rinse & Repeat
After posting your patches you will hopefully see some response in the form of comments, patch review and eventually commit.
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-send-email (if used standalone) for reposting new revisions is
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
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 <email@example.com> Acked-by: Jane Doe <firstname.lastname@example.org> --- 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 ...
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 email@example.com, 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 firstname.lastname@example.org, email@example.com, 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.
How to Generate and Submit a Xen Project Patch to MiniOS and Unikraft
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.