|
|
| (38 intermediate revisions by 9 users not shown) |
| Line 1: |
Line 1: |
| QEMU welcomes contributions of code (either fixing bugs or adding new functionality). However, we get a lot of patches, and so we have some guidelines about submitting patches. If you follow these, you'll help make our task of code review easier and your patch is likely to be committed faster.
| | Moved at https://www.qemu.org/docs/master/devel/submitting-a-patch.html |
| | |
| | |
| All contributions to QEMU must be '''sent as patches''' to the qemu-devel [[MailingLists|mailing list]]. Patch contributions should not be posted on the bug tracker, posted on forums, or externally hosted and linked to.
| |
| | |
| You do not have to subscribe to post (list policy is to reply-to-all to preserve CCs and keep non-subscribers in the loop on the threads they start), although you may find it easier as a subscriber to pick up good ideas from other posts. If you do subscribe, be prepared for a high volume of email, often over one thousand messages in a week. The list is moderated; first-time posts from an email address (whether or not you subscribed) may be subject to some delay while waiting for a moderator to whitelist your address.
| |
| | |
| Send patches to the mailing list and '''CC the relevant maintainer''' -- look in the MAINTAINERS file to find out who that is. Also try using scripts/get_maintainer.pl from the repository for learning the most common committers for the files you touched.
| |
| | |
| '''Send patches inline''' so they are easy to reply to with review comments. Do not put patches in attachments.
| |
| | |
| '''Use the right patch format'''. [http://git-scm.com/docs/git-format-patch <code>git format-patch</code>] will produce patch emails in the right format (check the documentation to find out how to drive it). You can then edit the cover letter before using <code>git send-email</code> to mail the files to the mailing list. (We recommend [http://git-scm.com/docs/git-send-email <code>git send-email</code>] because mail clients often mangle patches by wrapping long lines or messing up whitespace. Some distributions do not include send-email in a default install of git; you may need to download additional packages, such as 'git-email' on Fedora-based systems.) Patch series need a cover letter, with shallow threading (all patches in the series are in-reply-to the cover letter, but not to each other); single unrelated patches do not need a cover letter (but if you do send a cover letter, use --numbered so the cover and the patch have distinct subject lines). Patches are easier to find if they start a new top-level thread, rather than being buried in-reply-to another existing thread.
| |
| | |
| '''Patch emails must include a Signed-off-by: line'''. For more information see [http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=Documentation/SubmittingPatches;h=689e2371095cc5dfea9927120009341f369159aa;hb=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#l297 SubmittingPatches 1.12]. This is vital or we will not be able to apply your patch! Please use your real name to sign a patch (not an alias or acronym).
| |
| | |
| '''Make the cover letter meaningful'''. When reviewers don't know your goal at the start of their review, they may object to early changes that don't make sense until the end of the series, because they do not have enough context yet at that point of their review. A series where the goal is unclear also risks a higher number of review-fix cycles because the reviewers haven't bought into the idea yet. It is in everyone's interest to explain the goal in the cover letter. Then the patches get reviewed more smoothly and merged faster. Make sure your cover letter includes a diffstat of changes made over the entire series, as it is easier for a potential reviewer to check your cover letter than it is to check every letter in the series when determining if the series touches files they are interested in.
| |
| | |
| '''Follow the coding style''' and run ''scripts/checkpatch.pl <patchfile>'' before submitting. (Be aware that checkpatch.pl is not infallible, though, especially where C preprocessor macros are involved; use some common sense too.) See also:
| |
| * {{src|path=CODING_STYLE|description=QEMU Coding Style}}
| |
| * {{src|path=HACKING|description=QEMU Coding Guidelines}}
| |
| * [http://blog.vmsplice.net/2011/03/how-to-automatically-run-checkpatchpl.html Automate a checkpatch run on commit]
| |
| | |
| '''Correct English''' is appreciated. If you are not sure, [[Contribute/SpellCheck|codespell]] or other programs help finding the most common spelling mistakes in code and documentation.
| |
| | |
| '''Patches should be against current git master'''. There's no point submitting a patch which is based on a released version of QEMU because development will have moved on from then and it probably won't even apply to master. We only apply selected bugfixes to release branches and then only as backports once the code has gone into master.
| |
| | |
| '''Split up longer patches''' into a patch series of logical code changes. Each change should compile and execute successfully. For instance, don't add a file to the makefile in patch one and then add the file itself in patch two. (This rule is here so that people can later use tools like [http://git-scm.com/docs/git-bisect <code>git bisect</code>] without hitting points in the commit history where QEMU doesn't work for reasons unrelated to the bug they're chasing.) Put documentation first, not last, so that someone reading the series can do a clean-room evaluation of the documentation, then validate that the code matched the documentation. A commit message that mentions "Also, ..." is often a good candidate for splitting into multiple patches. For more thoughts on properly splitting patches and writing good commit messages, see [https://wiki.openstack.org/wiki/GitCommitMessages this advice from OpenStack].
| |
| | |
| '''Make code motion patches easy to review'''. If a series requires large blocks of code motion, there are tricks for making the refactoring easier to review. Split up the series so that semantic changes (or even function renames) are done in a separate patch from the raw code motion. Use a one-time setup of [http://git-scm.com/docs/git-config <code>git config diff.renames true;</code> <code>git config diff.algorithm patience</code>]. The 'diff.renames' property ensures file rename patches will be given in a more compact representation that focuses only on the differences across the file rename, instead of showing the entire old file as a deletion and the new file as an insertion. Meanwhile, the 'diff.algorithm' property ensures that extracting a non-contiguous subset of one file into a new file, but where all extracted parts occur in the same order both before and after the patch, will reduce churn in trying to treat unrelated <code>}</code> lines in the original file as separating hunks of changes. Ideally, a code motion patch can be reviewed by doing <code>git format-patch --stdout -1 > patch;</code> <code>diff -u <(sed -n 's/^-//p' patch) <(sed -n 's/^\+//p' patch)</code>, to focus on the few changes that weren't wholesale code motion.
| |
| | |
| '''Don't include irrelevant changes'''. In particular, don't include formatting, coding style or whitespace changes to bits of code that would otherwise not be touched by the patch. (It's OK to fix coding style issues in the immediate area (few lines) of the lines you're changing.) If you think a section of code really does need a reindent or other large-scale style fix, submit this as a separate patch which makes no semantic changes; don't put it in the same patch as your bug fix.
| |
| | |
| For smaller patches in less frequently changed areas of QEMU, '''consider using the [[Contribute/TrivialPatches|trivial patches]] process'''.
| |
| | |
| '''Write a good commit message'''. QEMU follows the usual standard for git commit messages: the first line (which becomes the email subject line) is "subsystem: single line summary of change". Whether the "single line summary of change" starts with a capital is a matter of taste, but we prefer that the summary does not end in ".". Look at <code>git short-log 30</code> for an idea of sample subject lines. Then there is a blank line and a more detailed description of the patch, another blank and your Signed-off-by: line. The body of the commit message is a good place to document why your change is important. Don't include comments like "This is a suggestion for fixing this bug" (they can go below the "---" line in the email so they don't go into the final commit message). Make sure the body of the commit message can be read in isolation even if the reader's mailer displays the subject line some distance apart (that is, a body that starts with "... so that" as a continuation of the subject line is harder to follow).
| |
| | |
| '''Stay around to fix problems raised in code review'''. Not many patches get into QEMU straight away -- it is quite common that developers will identify bugs, or suggest a cleaner approach, or even just point out code style issues or commit message typos. You'll need to respond to these, and then send a second version of your patches with the issues fixed. This takes a little time and effort on your part, but if you don't do it then your changes will never get into QEMU. It's also just polite -- it is quite disheartening for a developer to spend time reviewing your code and suggesting improvements, only to find that you're not going to do anything further and it was all wasted effort.
| |
| | |
| When replying to comments on your patches '''reply to all and not just the sender''' -- keeping discussion on the mailing list means everybody can follow it.
| |
| | |
| '''Pay attention to review comments'''. Someone took their time to review your work, and it pays to respect that effort; repeatedly submitting a series without addressing all comments from the previous round tends to alienate reviewers and stall your patch. Reviewers aren't always perfect, so it is okay if you want to argue that your code was correct in the first place instead of blindly doing everything the reviewer asked. On the other hand, if someone pointed out a potential issue during review, then even if your code turns out to be correct, it's probably a sign that you should improve your commit message and/or comments in the code explaining why the code is correct.
| |
| | |
| If you fix issues that are raised during review '''resend the entire patch series''' not just the one patch that was changed. This allows maintainers to easily apply the fixed series without having to manually identify which patches are relevant. Send the new version as a complete fresh email or series of emails -- don't try to make it a followup to version 1. (This helps automatic patch email handling tools distinguish between v1 and v2 emails.)
| |
| | |
| '''When resending patches add a v2/v3 suffix''' (eg [PATCH v2]). This means people can easily identify whether they're looking at the most recent version. (The first version of a patch need not say "v1", just [PATCH] is sufficient.) For patch series, the version applies to the whole series -- even if you only change one patch, you resend the entire series and mark it as "v2". Don't try to track versions of different patches in the series separately. [http://git-scm.com/docs/git-format-patch <code>git format-patch</code>] and [http://git-scm.com/docs/git-send-email<code>git send-email</code>] both understand the <code>-v2</code> option to make this easier. Send each new revision as a new top-level thread, rather than burying it in-reply-to an earlier revision, as many reviewers are not looking inside deep threads for new patches.
| |
| | |
| For later versions of patches '''include a summary of changes from previous versions, but not in the commit message itself'''. In an email formatted as a git patch, the commit message is the part above the "---" line, and this will go into the git changelog when the patch is committed. This part should be a self-contained description of what this version of the patch does, written to make sense to anybody who comes back to look at this commit in git in six months' time. The part below the "---" line and above the patch proper (git format-patch puts the diffstat here) is a good place to put remarks for people reading the patch email, and this is where the "changes since previous version" summary belongs. The [https://github.com/stefanha/git-publish git-publish] script can help with tracking a good summary across versions. Also, the [https://github.com/codyprime/git-scripts git-backport-diff] script can help focus reviewers on what changed between revisions.
| |
| | |
| '''Use RFC suffix if needed''' (eg [PATCH RFC v2]). <code>git format-patch --subject-prefix=RFC</code> can help.
| |
| | |
| "RFC" means "Request For Comments" and is a statement that you don't
| |
| intend for your patchset to be applied to master, but would like some
| |
| review on it anyway. Reasons for doing this include:
| |
| * the patch depends on some pending kernel changes which haven't yet been accepted, so the QEMU patch series is blocked until that dependency has been dealt with, but is worth reviewing anyway
| |
| * the patch set is not finished yet (perhaps it doesn't cover all use cases or work with all targets) but you want early review of a major API change or design structure before continuing
| |
| | |
| In general, since it's asking other people to do review work
| |
| on a patchset that the submitter themselves is saying shouldn't
| |
| be applied, it's best to:
| |
| * use it sparingly
| |
| * in the cover letter, be clear about why a patch is an RFC, what areas of the patchset you're looking for review on, and why reviewers should care
| |
| | |
| '''Proper use of Reviewed-by: tags can aid review.''' When reviewing a large series, a reviewer can reply to some of the patches with a Reviewed-by tag, stating that they are happy with that patch in isolation (sometimes conditional on minor cleanup, like fixing whitespace, that doesn't affect code content). You should then update those commit messages by hand to include the Reviewed-by tag, so that in the next revision, reviewers can spot which patches were already clean from the previous round. Conversely, if you significantly modify a patch that was previously reviewed, remove the reviewed-by tag out of the commit message, as well as listing the changes from the previous version, to make it easier to focus a reviewer's attention to your changes.
| |
| | |
| '''If your patch seems to have been ignored''' you should "ping" it after a week or two, by sending an email as a reply-to-all to the patch mail, including the word "ping" and ideally also a link to the page for the patch on [http://patchwork.ozlabs.org/project/qemu-devel/list/ patchwork] or GMANE. It's worth double-checking for reasons why your patch might have been ignored (forgot to CC the maintainer? annoyed people by failing to respond to review comments on an earlier version?), but often for less-maintained areas of QEMU patches do just slip through the cracks. If your ping is also ignored, ping again after another week or so. As the submitter, you are the person with the most motivation to get your patch applied, so you have to be persistent.
| |
| | |
| '''Is my patch in?''' Once your patch has had enough review on list, the maintainer for that area of code will send notification to the list that they are including your patch in a particular staging branch. Periodically, the maintainer then sends a [[Contribute/SubmitAPullRequest|pull request]] for aggregating topic branches into mainline qemu. Generally, you do not need to send a pull request unless you have contributed enough patches to become a maintainer over a particular section of code. Maintainers may further modify your commit, by resolving simple merge conflicts or fixing minor typos pointed out during review, but will always add a Signed-off-by line in addition to yours, indicating that it went through their tree. Occasionally, the maintainer's pull request may hit more difficult merge conflicts, where you may be requested to help rebase and resolve the problems. It may take a couple of weeks between when your patch first had a positive review to when it finally lands in qemu.git; release cycle freezes may extend that time even longer.
| |
| | |
| '''Return the favor'''. Peer review only works if everyone chips in a bit of review time. If everyone submitted more patches than they reviewed, we would have a patch backlog. A good goal is to try to review at least as many patches from others as what you submit. Don't worry if you don't know the code base as well as a maintainer; it's perfectly fine to admit when your review is weak because you are unfamiliar with the code.
| |
