Change Review and Management [ was: Re: Getting patches applied]
Hi all - Dipping my toes into this thread. Large projects need tools to help manage what's coming in. Matt brings this point home well below. Some history, for about 7 or 8 years, I was the ON CRT Chair for Solaris. The CRT, or Change Review Team, reviewed all changes for the ON (Operating Systems and Networking Consolidation). This is an elite team of experienced Solaris engineers and subject matter experts who determine if a fix is ready for the gate. They are basically acting as an extension of the technical lead (who could alway back out a change, or provide extra guidance when needed). We split things up like this, well before my time in ON, to distribute the load of reviewing requests to integrate into the source base. Requestors would specify things like: what bug they were fixing, why it needed to be fixed, what testing was performed, how risky the fix was, if doc changes were needed, diffs, and who had code reviewed the changeset. CRT members could ask for more/different reviewers, say this isn't needed or ready, or the tech lead could reject a change. New members can be added as new people become experienced and subject matter experts. Doing something like this would require some infrastructure and tools, but it does mean that you don't lose track of requests. Spreading the load also helps to process things faster. I believe a similar model is employed with other OSS projects. Yes, I realize that creating the tools and the process are not always obvious or straightforward. some old external documents on the subject: https://web.archive.org/web/20090828103904/http://www.opensolaris.org/os/community/on/crt/becoming-an-advocate/ https://web.archive.org/web/20090828103727/http://www.opensolaris.org/os/community/on/crt/charter/ Guidelines for people writing the code, also used by advocates to determine if the submitter has done their due diligence: https://web.archive.org/web/20090823010930/http://opensolaris.org/os/community/on/crt/rti-nits/ Hope that helps trigger some ideas Valerie [Please: no comments on why I had to use that form of URL, that's off topic for this community. ta.] On 04/11/14 02:26 AM, Matt Caswell wrote: On 11 April 2014 00:00, Steve Marquess marqu...@opensslfoundation.com wrote: With the very, very important caveat that I'm not one of the people who directly carry this burden: There is certainly room for improvement in the process by which patches are reviewed and merged into OpenSSL. For the more straightforward bug fixes and minor changes it might be useful to have a mechanism where a patch could be approved by multiple people and then committed to OpenSSL almost automatically. Obviously this wouldn't work for significant changes like whole new APIs and infrastructure mods. I have long been of the view that the current process for reviewing patches is broken. Through no individual's fault there just aren't enough people with commit rights to review the number of patches that get submitted. Many of these patches are really quite straight forward and I think we miss out on a lot. I see lots of patches go by which would have added value (e.g. documentation fixes, minor code fixes etc). If someone has gone to the effort of providing a patch, then they really deserve some kind of response (even if that response is thanks but no thanks). Many patches go by without anyone ever looking at them. I could envisage some kind of triage process where patches are classified into different levels or risk, and dependent on that risk a different level of scrutiny is required. E.g. so a patch providing a documentation tweak is treated differently to a patch providing a big piece of new functionality. I could also imagine a hierarchy of comitters with different levels of sign-off - low risk changes can be comitted by a larger group of people - only high risk changes need to go to the core team. The multiple people could be a sufficiently large and diverse group of serious and committed stakeholders, both OpenSSL team members and others. Volunteers? I see many of the same names appear on this list and on the users list from people who clearly know what they are talking about and have a high degree of skill. I am sure some of those could be persuaded to help out. I of course would be happy to volunteer :-) Of course, a process like that wouldn't necessarily prevent future vulnerabilities like the Debian PRNG issue or the heartbeat bug. Even gross bugs are only truly obvious in hindsight. True. And in fact it seems counter-intuitive when faced with a major problem like heartbleed to open up the project to *more* people capable of comitting. The temptation is to batton-down-the-hatches and keep new people out. But actually I think by involving more people in the review process we have the opportunity to improve quality through having many-eyes reviewing changes - instead of just a very small number who don't have enough time to spend on it. Matt
Re: Getting patches applied
On 11 April 2014 00:00, Steve Marquess marqu...@opensslfoundation.com wrote: With the very, very important caveat that I'm not one of the people who directly carry this burden: There is certainly room for improvement in the process by which patches are reviewed and merged into OpenSSL. For the more straightforward bug fixes and minor changes it might be useful to have a mechanism where a patch could be approved by multiple people and then committed to OpenSSL almost automatically. Obviously this wouldn't work for significant changes like whole new APIs and infrastructure mods. I have long been of the view that the current process for reviewing patches is broken. Through no individual's fault there just aren't enough people with commit rights to review the number of patches that get submitted. Many of these patches are really quite straight forward and I think we miss out on a lot. I see lots of patches go by which would have added value (e.g. documentation fixes, minor code fixes etc). If someone has gone to the effort of providing a patch, then they really deserve some kind of response (even if that response is thanks but no thanks). Many patches go by without anyone ever looking at them. I could envisage some kind of triage process where patches are classified into different levels or risk, and dependent on that risk a different level of scrutiny is required. E.g. so a patch providing a documentation tweak is treated differently to a patch providing a big piece of new functionality. I could also imagine a hierarchy of comitters with different levels of sign-off - low risk changes can be comitted by a larger group of people - only high risk changes need to go to the core team. The multiple people could be a sufficiently large and diverse group of serious and committed stakeholders, both OpenSSL team members and others. Volunteers? I see many of the same names appear on this list and on the users list from people who clearly know what they are talking about and have a high degree of skill. I am sure some of those could be persuaded to help out. I of course would be happy to volunteer :-) Of course, a process like that wouldn't necessarily prevent future vulnerabilities like the Debian PRNG issue or the heartbeat bug. Even gross bugs are only truly obvious in hindsight. True. And in fact it seems counter-intuitive when faced with a major problem like heartbleed to open up the project to *more* people capable of comitting. The temptation is to batton-down-the-hatches and keep new people out. But actually I think by involving more people in the review process we have the opportunity to improve quality through having many-eyes reviewing changes - instead of just a very small number who don't have enough time to spend on it. Matt __ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager majord...@openssl.org
Re: Getting patches applied
On Fri, Apr 11, 2014 at 10:26:08AM +0100, Matt Caswell wrote: On 11 April 2014 00:00, Steve Marquess marqu...@opensslfoundation.com wrote: With the very, very important caveat that I'm not one of the people who directly carry this burden: There is certainly room for improvement in the process by which patches are reviewed and merged into OpenSSL. For the more straightforward bug fixes and minor changes it might be useful to have a mechanism where a patch could be approved by multiple people and then committed to OpenSSL almost automatically. Obviously this wouldn't work for significant changes like whole new APIs and infrastructure mods. I have long been of the view that the current process for reviewing patches is broken. Through no individual's fault there just aren't enough people with commit rights to review the number of patches that get submitted. Many of these patches are really quite straight forward and I think we miss out on a lot. I see lots of patches go by which would have added value (e.g. documentation fixes, minor code fixes etc). If someone has gone to the effort of providing a patch, then they really deserve some kind of response (even if that response is thanks but no thanks). Many patches go by without anyone ever looking at them. Yes, and this is my main motivation for starting this thread. It's unrelated to heartbleed. Kurt __ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager majord...@openssl.org
Getting patches applied
Hi, I've seen many examples of patches being submitted but never getting applied. For some problems there are actually multiple people submitting a patch for the same issue, and none of them getting applied. What is the problem getting them applied? Not enough people to do the reviewing? People who have commit access don't have enough time, or different priorities? Kurt __ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager majord...@openssl.org
Re: Getting patches applied
On 04/10/2014 03:22 PM, Kurt Roeckx wrote: Hi, I've seen many examples of patches being submitted but never getting applied. For some problems there are actually multiple people submitting a patch for the same issue, and none of them getting applied. What is the problem getting them applied? Not enough people to do the reviewing? People who have commit access don't have enough time, or different priorities? Yes, yes, and not necessarily. While I'm not one of the OpenSSL committers, I've had the honor and privilege of being close enough to some of the action to have an appreciation for the heavy burden of responsibility they carry. IMHO user community contributions, and the care and effort that went into them and the desire and need for them, are not being callously disregarded. It's just that after all the urgently necessary activities are covered there isn't a lot of discretionary time left over. OpenSSL hangs by a thinner thread than most people realize. Since contributions are as likely to introduce problems or vulnerabilities as code authored directly by the OpenSSL team, I think you can expect even more caution for awhile. -Steve M. -- Steve Marquess OpenSSL Software Foundation, Inc. 1829 Mount Ephraim Road Adamstown, MD 21710 USA +1 877 673 6775 s/b +1 301 874 2571 direct marqu...@opensslfoundation.com marqu...@openssl.com gpg/pgp key: http://openssl.com/docs/0xCE69424E.asc __ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager majord...@openssl.org
Re: Getting patches applied
On Thu, Apr 10, 2014 at 04:40:13PM -0400, Steve Marquess wrote: On 04/10/2014 03:22 PM, Kurt Roeckx wrote: Hi, I've seen many examples of patches being submitted but never getting applied. For some problems there are actually multiple people submitting a patch for the same issue, and none of them getting applied. What is the problem getting them applied? Not enough people to do the reviewing? People who have commit access don't have enough time, or different priorities? Yes, yes, and not necessarily. While I'm not one of the OpenSSL committers, I've had the honor and privilege of being close enough to some of the action to have an appreciation for the heavy burden of responsibility they carry. IMHO user community contributions, and the care and effort that went into them and the desire and need for them, are not being callously disregarded. It's just that after all the urgently necessary activities are covered there isn't a lot of discretionary time left over. OpenSSL hangs by a thinner thread than most people realize. So my the question is basically what we can do so that more of the patches get applied in a reasonable time? For instance, would it help to use Signed-off-by or Reviewed-by patches in some git tree? If I'm going to put time in this, will someone take the time to get them applied? Kurt __ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager majord...@openssl.org
Re: Getting patches applied
On 04/10/2014 05:36 PM, Kurt Roeckx wrote: ... While I'm not one of the OpenSSL committers, I've had the honor and privilege of being close enough to some of the action to have an appreciation for the heavy burden of responsibility they carry. IMHO user community contributions, and the care and effort that went into them and the desire and need for them, are not being callously disregarded. It's just that after all the urgently necessary activities are covered there isn't a lot of discretionary time left over. OpenSSL hangs by a thinner thread than most people realize. So my the question is basically what we can do so that more of the patches get applied in a reasonable time? For instance, would it help to use Signed-off-by or Reviewed-by patches in some git tree? If I'm going to put time in this, will someone take the time to get them applied? With the very, very important caveat that I'm not one of the people who directly carry this burden: There is certainly room for improvement in the process by which patches are reviewed and merged into OpenSSL. For the more straightforward bug fixes and minor changes it might be useful to have a mechanism where a patch could be approved by multiple people and then committed to OpenSSL almost automatically. Obviously this wouldn't work for significant changes like whole new APIs and infrastructure mods. The multiple people could be a sufficiently large and diverse group of serious and committed stakeholders, both OpenSSL team members and others. Volunteers? Of course, a process like that wouldn't necessarily prevent future vulnerabilities like the Debian PRNG issue or the heartbeat bug. Even gross bugs are only truly obvious in hindsight. -Steve M. -- Steve Marquess OpenSSL Software Foundation, Inc. 1829 Mount Ephraim Road Adamstown, MD 21710 USA +1 877 673 6775 s/b +1 301 874 2571 direct marqu...@opensslfoundation.com marqu...@openssl.com gpg/pgp key: http://openssl.com/docs/0xCE69424E.asc __ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager majord...@openssl.org
