Re: [Qemu-devel] [PULL 00/25] RISC-V Post-merge spec conformance and cleanup

[Eric Blake]

On 03/22/2018 02:10 PM, Peter Maydell wrote:

[snip lots of good advice]

> Code review isn't only about "does this behave as the
> secification requires". It can also catch:
>   * simple logic bugs
>   * places where the code is more complicated than it needs to be
>   * style issues
>   * places where a QEMU utility routine could have been used
>   * misunderstandings about QEMU internal APIs
>   * memory leaks or memory corruption bugs

    * grammar/typo fixes in comments/documentation/user-visible error 
strings

> Review by somebody who is familiar with QEMU is just as useful
> as review by somebody familiar with the target ISA.

Indeed.  Not everyone is an expert on every subject, but the more people 
that get involved, each in their own area of expertise, the wider the 
review coverage can be for a better end product.  And I'm very much okay 
with a reviewer stating up front "I'm only commenting on aspect XYZ, but 
will leave the rest of your patch to someone more familiar with ABC" - 
even that admission of a partial review is still helpful.

> If your queue of unsubmitted or unreviewed patches is steadily
> getting longer then something is going wrong; you should be
> aiming for it to be as short as possible.

More on this point - typically, the review process is OFTEN the long 
pole in open source projects, where patches are submitted faster than 
reviewers can keep up.  While the following advice is by no means a 
requirement that everyone should follow, my personal goal is that I 
review at least two patches for every one that I submit, to help 
alleviate the review bottleneck.  And doing so has made ME a better 
programmer, as I get to explore coding paradigms and portions of the 
tree that I was previously unfamiliar with.

Furthermore, providing reviews to other's patches in addition to writing 
your own patches has some nice benefits: you gain some name recognition 
in the community, and it is obvious that you care more about the project 
as a whole than just about getting your stuff merged.  That in turn 
tends to have an interesting effect that your own patches get reviewed 
faster.

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org