1Submitting a Patch 2================== 3 4QEMU welcomes contributions of code (either fixing bugs or adding new 5functionality). However, we get a lot of patches, and so we have some 6guidelines about submitting patches. If you follow these, you'll help 7make our task of code review easier and your patch is likely to be 8committed faster. 9 10This page seems very long, so if you are only trying to post a quick 11one-shot fix, the bare minimum we ask is that: 12 13- You **must** provide a Signed-off-by: line (this is a hard 14 requirement because it's how you say "I'm legally okay to contribute 15 this and happy for it to go into QEMU", modeled after the `Linux kernel 16 <http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297>`__ 17 policy.) ``git commit -s`` or ``git format-patch -s`` will add one. 18- All contributions to QEMU must be **sent as patches** to the 19 qemu-devel `mailing list <MailingLists>`__. Patch contributions 20 should not be posted on the bug tracker, posted on forums, or 21 externally hosted and linked to. (We have other mailing lists too, 22 but all patches must go to qemu-devel, possibly with a Cc: to another 23 list.) ``git send-email`` works best for delivering the patch without 24 mangling it (`hints for setting it 25 up <http://lxr.free-electrons.com/source/Documentation/process/email-clients.rst>`__), 26 but attachments can be used as a last resort on a first-time 27 submission. 28- You must read replies to your message, and be willing to act on them. 29 Note, however, that maintainers are often willing to manually fix up 30 first-time contributions, since there is a learning curve involved in 31 making an ideal patch submission. 32 33You do not have to subscribe to post (list policy is to reply-to-all to 34preserve CCs and keep non-subscribers in the loop on the threads they 35start), although you may find it easier as a subscriber to pick up good 36ideas from other posts. If you do subscribe, be prepared for a high 37volume of email, often over one thousand messages in a week. The list is 38moderated; first-time posts from an email address (whether or not you 39subscribed) may be subject to some delay while waiting for a moderator 40to whitelist your address. 41 42The larger your contribution is, or if you plan on becoming a long-term 43contributor, then the more important the rest of this page becomes. 44Reading the table of contents below should already give you an idea of 45the basic requirements. Use the table of contents as a reference, and 46read the parts that you have doubts about. 47 48.. _writing_your_patches: 49 50Writing your Patches 51-------------------- 52 53.. _use_the_qemu_coding_style: 54 55Use the QEMU coding style 56~~~~~~~~~~~~~~~~~~~~~~~~~ 57 58You can run run *scripts/checkpatch.pl <patchfile>* before submitting to 59check that you are in compliance with our coding standards. Be aware 60that ``checkpatch.pl`` is not infallible, though, especially where C 61preprocessor macros are involved; use some common sense too. See also: 62 63- `QEMU Coding Style 64 <https://qemu-project.gitlab.io/qemu/devel/style.html>`__ 65 66- `Automate a checkpatch run on 67 commit <http://blog.vmsplice.net/2011/03/how-to-automatically-run-checkpatchpl.html>`__ 68 69.. _base_patches_against_current_git_master: 70 71Base patches against current git master 72~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 73 74There's no point submitting a patch which is based on a released version 75of QEMU because development will have moved on from then and it probably 76won't even apply to master. We only apply selected bugfixes to release 77branches and then only as backports once the code has gone into master. 78 79.. _split_up_long_patches: 80 81Split up long patches 82~~~~~~~~~~~~~~~~~~~~~ 83 84Split up longer patches into a patch series of logical code changes. 85Each change should compile and execute successfully. For instance, don't 86add a file to the makefile in patch one and then add the file itself in 87patch two. (This rule is here so that people can later use tools like 88`git bisect <http://git-scm.com/docs/git-bisect>`__ without hitting 89points in the commit history where QEMU doesn't work for reasons 90unrelated to the bug they're chasing.) Put documentation first, not 91last, so that someone reading the series can do a clean-room evaluation 92of the documentation, then validate that the code matched the 93documentation. A commit message that mentions "Also, ..." is often a 94good candidate for splitting into multiple patches. For more thoughts on 95properly splitting patches and writing good commit messages, see `this 96advice from 97OpenStack <https://wiki.openstack.org/wiki/GitCommitMessages>`__. 98 99.. _make_code_motion_patches_easy_to_review: 100 101Make code motion patches easy to review 102~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 103 104If a series requires large blocks of code motion, there are tricks for 105making the refactoring easier to review. Split up the series so that 106semantic changes (or even function renames) are done in a separate patch 107from the raw code motion. Use a one-time setup of 108``git config diff.renames true; git config diff.algorithm patience`` 109(Refer to `git-config <http://git-scm.com/docs/git-config>`__.) The 110``diff.renames`` property ensures file rename patches will be given in a 111more compact representation that focuses only on the differences across 112the file rename, instead of showing the entire old file as a deletion 113and the new file as an insertion. Meanwhile, the 'diff.algorithm' 114property ensures that extracting a non-contiguous subset of one file 115into a new file, but where all extracted parts occur in the same order 116both before and after the patch, will reduce churn in trying to treat 117unrelated ``}`` lines in the original file as separating hunks of 118changes. 119 120Ideally, a code motion patch can be reviewed by doing:: 121 122 git format-patch --stdout -1 > patch; 123 diff -u <(sed -n 's/^-//p' patch) <(sed -n 's/^\+//p' patch) 124 125to focus on the few changes that weren't wholesale code motion. 126 127.. _dont_include_irrelevant_changes: 128 129Don't include irrelevant changes 130~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 131 132In particular, don't include formatting, coding style or whitespace 133changes to bits of code that would otherwise not be touched by the 134patch. (It's OK to fix coding style issues in the immediate area (few 135lines) of the lines you're changing.) If you think a section of code 136really does need a reindent or other large-scale style fix, submit this 137as a separate patch which makes no semantic changes; don't put it in the 138same patch as your bug fix. 139 140For smaller patches in less frequently changed areas of QEMU, consider 141using the `trivial patches process 142<https://qemu-project.gitlab.io/qemu/devel/style.html>`__. 143 144.. _write_a_meaningful_commit_message: 145 146Write a meaningful commit message 147~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 148 149Commit messages should be meaningful and should stand on their own as a 150historical record of why the changes you applied were necessary or 151useful. 152 153QEMU follows the usual standard for git commit messages: the first line 154(which becomes the email subject line) is "subsystem: single line 155summary of change". Whether the "single line summary of change" starts 156with a capital is a matter of taste, but we prefer that the summary does 157not end in ".". Look at ``git shortlog -30`` for an idea of sample 158subject lines. Then there is a blank line and a more detailed 159description of the patch, another blank and your Signed-off-by: line. 160Please do not use lines that are longer than 76 characters in your 161commit message (so that the text still shows up nicely with "git show" 162in a 80-columns terminal window). 163 164The body of the commit message is a good place to document why your 165change is important. Don't include comments like "This is a suggestion 166for fixing this bug" (they can go below the ``---`` line in the email so 167they don't go into the final commit message). Make sure the body of the 168commit message can be read in isolation even if the reader's mailer 169displays the subject line some distance apart (that is, a body that 170starts with "... so that" as a continuation of the subject line is 171harder to follow). 172 173.. _submitting_your_patches: 174 175Submitting your Patches 176----------------------- 177 178.. _cc_the_relevant_maintainer: 179 180CC the relevant maintainer 181~~~~~~~~~~~~~~~~~~~~~~~~~~ 182 183Send patches both to the mailing list and CC the maintainer(s) of the 184files you are modifying. look in the MAINTAINERS file to find out who 185that is. Also try using scripts/get_maintainer.pl from the repository 186for learning the most common committers for the files you touched. 187 188Example:: 189 190 ~/src/qemu/scripts/get_maintainer.pl -f hw/ide/core.c 191 192In fact, you can automate this, via a one-time setup of ``git config 193sendemail.cccmd 'scripts/get_maintainer.pl --nogit-fallback'`` (Refer to 194`git-config <http://git-scm.com/docs/git-config>`__.) 195 196.. _do_not_send_as_an_attachment: 197 198Do not send as an attachment 199~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 200 201Send patches inline so they are easy to reply to with review comments. 202Do not put patches in attachments. 203 204.. _use_git_format_patch: 205 206Use ``git format-patch`` 207~~~~~~~~~~~~~~~~~~~~~~~~ 208 209Use the right diff format. 210`git format-patch <http://git-scm.com/docs/git-format-patch>`__ will 211produce patch emails in the right format (check the documentation to 212find out how to drive it). You can then edit the cover letter before 213using ``git send-email`` to mail the files to the mailing list. (We 214recommend `git send-email <http://git-scm.com/docs/git-send-email>`__ 215because mail clients often mangle patches by wrapping long lines or 216messing up whitespace. Some distributions do not include send-email in a 217default install of git; you may need to download additional packages, 218such as 'git-email' on Fedora-based systems.) Patch series need a cover 219letter, with shallow threading (all patches in the series are 220in-reply-to the cover letter, but not to each other); single unrelated 221patches do not need a cover letter (but if you do send a cover letter, 222use --numbered so the cover and the patch have distinct subject lines). 223Patches are easier to find if they start a new top-level thread, rather 224than being buried in-reply-to another existing thread. 225 226.. _patch_emails_must_include_a_signed_off_by_line: 227 228Patch emails must include a ``Signed-off-by:`` line 229~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 230 231For more information see `1.12) Sign your work 232<http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n296>`__. 233This is vital or we will not be able to apply your patch! Please use 234your real name to sign a patch (not an alias or acronym). 235 236If you wrote the patch, make sure your "From:" and "Signed-off-by:" 237lines use the same spelling. It's okay if you subscribe or contribute to 238the list via more than one address, but using multiple addresses in one 239commit just confuses things. If someone else wrote the patch, git will 240include a "From:" line in the body of the email (different from your 241envelope From:) that will give credit to the correct author; but again, 242that author's Signed-off-by: line is mandatory, with the same spelling. 243 244.. _include_a_meaningful_cover_letter: 245 246Include a meaningful cover letter 247~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 248 249This usually applies only to a series that includes multiple patches; 250the cover letter explains the overall goal of such a series. 251 252When reviewers don't know your goal at the start of their review, they 253may object to early changes that don't make sense until the end of the 254series, because they do not have enough context yet at that point of 255their review. A series where the goal is unclear also risks a higher 256number of review-fix cycles because the reviewers haven't bought into 257the idea yet. If the cover letter can explain these points to the 258reviewer, the process will be smoother patches will get merged faster. 259Make sure your cover letter includes a diffstat of changes made over the 260entire series; potential reviewers know what files they are interested 261in, and they need an easy way determine if your series touches them. 262 263.. _use_the_rfc_tag_if_needed: 264 265Use the RFC tag if needed 266~~~~~~~~~~~~~~~~~~~~~~~~~ 267 268For example, "[PATCH RFC v2]". ``git format-patch --subject-prefix=RFC`` 269can help. 270 271"RFC" means "Request For Comments" and is a statement that you don't 272intend for your patchset to be applied to master, but would like some 273review on it anyway. Reasons for doing this include: 274 275- the patch depends on some pending kernel changes which haven't yet 276 been accepted, so the QEMU patch series is blocked until that 277 dependency has been dealt with, but is worth reviewing anyway 278- the patch set is not finished yet (perhaps it doesn't cover all use 279 cases or work with all targets) but you want early review of a major 280 API change or design structure before continuing 281 282In general, since it's asking other people to do review work on a 283patchset that the submitter themselves is saying shouldn't be applied, 284it's best to: 285 286- use it sparingly 287- in the cover letter, be clear about why a patch is an RFC, what areas 288 of the patchset you're looking for review on, and why reviewers 289 should care 290 291.. _participating_in_code_review: 292 293Participating in Code Review 294---------------------------- 295 296All patches submitted to the QEMU project go through a code review 297process before they are accepted. Some areas of code that are well 298maintained may review patches quickly, lesser-loved areas of code may 299have a longer delay. 300 301.. _stay_around_to_fix_problems_raised_in_code_review: 302 303Stay around to fix problems raised in code review 304~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 305 306Not many patches get into QEMU straight away -- it is quite common that 307developers will identify bugs, or suggest a cleaner approach, or even 308just point out code style issues or commit message typos. You'll need to 309respond to these, and then send a second version of your patches with 310the issues fixed. This takes a little time and effort on your part, but 311if you don't do it then your changes will never get into QEMU. It's also 312just polite -- it is quite disheartening for a developer to spend time 313reviewing your code and suggesting improvements, only to find that 314you're not going to do anything further and it was all wasted effort. 315 316When replying to comments on your patches **reply to all and not just 317the sender** -- keeping discussion on the mailing list means everybody 318can follow it. 319 320.. _pay_attention_to_review_comments: 321 322Pay attention to review comments 323~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 324 325Someone took their time to review your work, and it pays to respect that 326effort; repeatedly submitting a series without addressing all comments 327from the previous round tends to alienate reviewers and stall your 328patch. Reviewers aren't always perfect, so it is okay if you want to 329argue that your code was correct in the first place instead of blindly 330doing everything the reviewer asked. On the other hand, if someone 331pointed out a potential issue during review, then even if your code 332turns out to be correct, it's probably a sign that you should improve 333your commit message and/or comments in the code explaining why the code 334is correct. 335 336If you fix issues that are raised during review **resend the entire 337patch series** not just the one patch that was changed. This allows 338maintainers to easily apply the fixed series without having to manually 339identify which patches are relevant. Send the new version as a complete 340fresh email or series of emails -- don't try to make it a followup to 341version 1. (This helps automatic patch email handling tools distinguish 342between v1 and v2 emails.) 343 344.. _when_resending_patches_add_a_version_tag: 345 346When resending patches add a version tag 347~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 348 349All patches beyond the first version should include a version tag -- for 350example, "[PATCH v2]". This means people can easily identify whether 351they're looking at the most recent version. (The first version of a 352patch need not say "v1", just [PATCH] is sufficient.) For patch series, 353the version applies to the whole series -- even if you only change one 354patch, you resend the entire series and mark it as "v2". Don't try to 355track versions of different patches in the series separately. `git 356format-patch <http://git-scm.com/docs/git-format-patch>`__ and `git 357send-email <http://git-scm.com/docs/git-send-email>`__ both understand 358the ``-v2`` option to make this easier. Send each new revision as a new 359top-level thread, rather than burying it in-reply-to an earlier 360revision, as many reviewers are not looking inside deep threads for new 361patches. 362 363.. _include_version_history_in_patchset_revisions: 364 365Include version history in patchset revisions 366~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 367 368For later versions of patches, include a summary of changes from 369previous versions, but not in the commit message itself. In an email 370formatted as a git patch, the commit message is the part above the "---" 371line, and this will go into the git changelog when the patch is 372committed. This part should be a self-contained description of what this 373version of the patch does, written to make sense to anybody who comes 374back to look at this commit in git in six months' time. The part below 375the "---" line and above the patch proper (git format-patch puts the 376diffstat here) is a good place to put remarks for people reading the 377patch email, and this is where the "changes since previous version" 378summary belongs. The 379`git-publish <https://github.com/stefanha/git-publish>`__ script can 380help with tracking a good summary across versions. Also, the 381`git-backport-diff <https://github.com/codyprime/git-scripts>`__ script 382can help focus reviewers on what changed between revisions. 383 384.. _tips_and_tricks: 385 386Tips and Tricks 387--------------- 388 389.. _proper_use_of_reviewed_by_tags_can_aid_review: 390 391Proper use of Reviewed-by: tags can aid review 392~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 393 394When reviewing a large series, a reviewer can reply to some of the 395patches with a Reviewed-by tag, stating that they are happy with that 396patch in isolation (sometimes conditional on minor cleanup, like fixing 397whitespace, that doesn't affect code content). You should then update 398those commit messages by hand to include the Reviewed-by tag, so that in 399the next revision, reviewers can spot which patches were already clean 400from the previous round. Conversely, if you significantly modify a patch 401that was previously reviewed, remove the reviewed-by tag out of the 402commit message, as well as listing the changes from the previous 403version, to make it easier to focus a reviewer's attention to your 404changes. 405 406.. _if_your_patch_seems_to_have_been_ignored: 407 408If your patch seems to have been ignored 409~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 410 411If your patchset has received no replies you should "ping" it after a 412week or two, by sending an email as a reply-to-all to the patch mail, 413including the word "ping" and ideally also a link to the page for the 414patch on 415`patchwork <http://patchwork.ozlabs.org/project/qemu-devel/list/>`__ or 416GMANE. It's worth double-checking for reasons why your patch might have 417been ignored (forgot to CC the maintainer? annoyed people by failing to 418respond to review comments on an earlier version?), but often for 419less-maintained areas of QEMU patches do just slip through the cracks. 420If your ping is also ignored, ping again after another week or so. As 421the submitter, you are the person with the most motivation to get your 422patch applied, so you have to be persistent. 423 424.. _is_my_patch_in: 425 426Is my patch in? 427~~~~~~~~~~~~~~~ 428 429Once your patch has had enough review on list, the maintainer for that 430area of code will send notification to the list that they are including 431your patch in a particular staging branch. Periodically, the maintainer 432then sends a `pull request 433<https://qemu-project.gitlab.io/qemu/devel/submitting-a-pull-request.html>`__ 434for aggregating topic branches into mainline qemu. Generally, you do not 435need to send a pull request unless you have contributed enough patches 436to become a maintainer over a particular section of code. Maintainers 437may further modify your commit, by resolving simple merge conflicts or 438fixing minor typos pointed out during review, but will always add a 439Signed-off-by line in addition to yours, indicating that it went through 440their tree. Occasionally, the maintainer's pull request may hit more 441difficult merge conflicts, where you may be requested to help rebase and 442resolve the problems. It may take a couple of weeks between when your 443patch first had a positive review to when it finally lands in qemu.git; 444release cycle freezes may extend that time even longer. 445 446.. _return_the_favor: 447 448Return the favor 449~~~~~~~~~~~~~~~~ 450 451Peer review only works if everyone chips in a bit of review time. If 452everyone submitted more patches than they reviewed, we would have a 453patch backlog. A good goal is to try to review at least as many patches 454from others as what you submit. Don't worry if you don't know the code 455base as well as a maintainer; it's perfectly fine to admit when your 456review is weak because you are unfamiliar with the code. 457