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