Re: [RFC PATCH v2] blog post: how to get your new feature up-streamed

[Peter Maydell]

On Mon, 6 Dec 2021 at 14:14, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Experience has shown that getting new functionality up-streamed can be
> a somewhat painful process. Lets see if we can collect some of our
> community knowledge into a blog post describing some best practices
> for getting code accepted.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

(I shan't bother with a scan for typos etc for the moment.)

> +From time to time I hear of frustrations from potential new
> +contributors who have tried to get new features up-streamed into the
> +QEMU repository. After having read [our patch
> +guidelines](https://qemu.readthedocs.io/en/latest/devel/submitting-a-patch.html)
> +they post them to [qemu-devel](https://lore.kernel.org/qemu-devel/).
> +Often the patches sit there seemingly unread and unloved. The
> +developer is left wandering if they missed out the secret hand shake
> +required to move the process forward. My hope is that this blog post
> +will help.
> +
> +New features != Fixing a bug
> +----------------------------
> +
> +Adding a new feature is not the same as fixing a bug. For an area of
> +code that is supported for Odd Fixes or above there will be a
> +someone listed in the
> +[MAINTAINERS](https://gitlab.com/qemu-project/qemu/-/blob/master/MAINTAINERS)
> +file. A properly configured `git-send-email` will even automatically
> +add them to the patches as they are sent out. The maintainer will
> +review the code and if no changes are requested they ensure the
> +patch flows through the appropriate trees and eventually makes it into
> +the master branch.
> +
> +This doesn't usually happen for new code unless your patches happen to
> +touch a directory that is marked as maintained. Without a maintainer
> +to look at and apply your patches how will it ever get merged?

I think there is a distinction here between "new feature added
to something that we already have" and "new feature that isn't
part of something we already have". In the former category for
instance you have things like "support virtio-mem-pci on the
virt board". That's definitely not a bug fix, but it ties into
existing and hopefully maintained in-tree things (the virt board,
the x86 virtio-mem-pci support) whose maintainers are probably[*]
interested and informed enough to help with review. On the other
hand "add support for new target architecture foo" is more
distinct from what we have already.

[*] There is of course the elephant-trap the project sets for
new submitters of having parts of the codebase that according
to MAINTAINERS have a maintainer but in practice that maintainer
is AWOL or overstretched.

There is also the spectrum from "clearly fits neatly into QEMU's
current design" through to "proposing something very new".
"New board" and "new target architecture" may not have a current
maintainer, but they do fit very obviously into our existing
structure and design. Nobody is likely to object on principle,
and the submitter's problem here is merely to attract the attention
of somebody to get it reviewed. On the other hand, a new feature
like "support plugging QEMU into a SystemC emulation" is massively
impactful and poses serious structural questions. A submitter would
have a massive uphill job to gain consensus about supporting the
feature at all, even before getting into the specifics of a patchset.

> +
> +Adding new code to a project is not a free activity. Code that isn't
> +actively maintained has a tendency to [bit
> +rot](http://www.catb.org/jargon/html/B/bit-rot.html) and become a drag
> +on the rest of the code base. The QEMU code base is quite large and
> +none of the developers are knowledgeable about the all of it. If
> +features aren't
> +[documented](https://qemu.readthedocs.io/en/latest/devel/submitting-a-patch.html)
> +they tend to remain unused as users struggle to enable them. If an
> +unused feature becomes a drag on the rest of the code base by preventing
> +re-factoring and other clean ups it is likely to be deprecated.
> +Eventually deprecated code gets removed from the code base never to be
> +seen again.
> +
> +Fortunately there is a way to avoid the ignominy of ignored new features
> +and that is to become a maintainer of your own code!

I think this is oversimplifying to the point of being misleading.
Some kind of commitment to staying around (ie that this is not
a "drop the code and run away" manoeuvre) is definitely helpful,
but it doesn't by itself do anything to address the primary
problem, which is "you need to persuade somebody to care enough
about your new feature to put in the work to review it".


> +A practically perfect set of patches
> +------------------------------------
> +
> +I don't want to repeat all the valuable information from the
> +submitting patches document but

For a new feature, I think the thing I would most emphasise
is the importance of a really good cover letter. It gets more critical
as you move along the spectrum from "new feature in existing
subsystem" to "new feature but which sits cleanly within QEMU"
to "new and unusual feature". The cover letter is your opportunity
to explain what you're doing and why somebody else should care about
it enough to code review it. It needs to explain the work
to an audience who hasn't spent the last six weeks developing
it, and why it would be useful to the project to accept it.
If the feature looks on the surface like it's "odd thing that
we haven't done before" but is really "quite similar to existing
thing if you look at it closer", the cover letter is a good
place to explain that. (This all applies still for v2 etc;
the v2 cover letter shouldn't assume that the reader was
paying attention and looked at the v1 cover letter or patches.)

> +there is still the
> +problem of getting reviews for your brand new code. Fortunately there
> +is no approved reviewer list for QEMU.

Something of a side-tangent, but I'm not sure about "fortunately".
There's an analogy to be drawn here with some of the points made
in Jo Freeman's _Tyranny of Structurelessness_ essay
(https://www.jofreeman.com/joreen/tyranny.htm) about how if
you don't have official structures and hierarchy you get a
de-facto unofficial one anyway -- we do not have
a formal approved reviewer list, and therefore instead we
have an informal and unacknowledged set of "trusted" reviewers.
The latter situation is not self-evidently better than the former.

-- PMM