svn commit: r335402 - head/sbin/veriexecctl

[Conrad Meyer]

Hi Jon,

On Wed, Jun 20, 2018 at 11:58 AM, Jonathan Anderson
<jonathan at freebsd.org> wrote:
> On 20 Jun 2018, at 15:32, Jonathan T. Looney wrote:
>> My reasoning
>> was fairly simple: when a review has been open for over a year with no
>> action, it seems like the submitter should be able to commit it without
>> waiting for more review, if they are confident in their change. I stand by
>> that (and, in fact, would substitute something shorter for "over a year").

For this as a question of general process, I think "it depends."  For
minor fixes?  Of course!  Less than one year.  For minor improvements
or enhancements?  Also yes, also probably less than 1yr.  For large
features like this, I think the answer is more nuanced.

Sometimes being in a review for a year means the reviewers are
overloaded and don't have time.  In that case, maybe they shouldn't
block progress.  Sometimes being in review for a year just means the
right reviewers haven't been found.  I think that was more the case
here.

I would suggest anyone submitting a large feature and not getting
feedback in a reasonable timeframe to solicit feedback from a wider
audience much sooner than one year, and also indicate intention to
merge the unreviewed change with some time window (1-2 weeks?) on
giving feedback or at least asking for more time to review.

> So: is there a strong reason why a backout-and-re-commit approach is
> superior to iterating on the code in-tree?

Yeah:

> Any disputed change must be backed out pending resolution of the dispute
> if requested by a maintainer. Security related changes may override a
> maintainer's wishes at the Security Officer's discretion.
>
> This may be hard to swallow in times of conflict (when each side is
> convinced that they are in the right, of course) but a version control
> system makes it unnecessary to have an ongoing dispute raging when it is
> far easier to simply reverse the disputed change, get everyone calmed
> down again and then try to figure out what is the best way to proceed.
> If the change turns out to be the best thing after all, it can be easily
> brought back. If it turns out not to be, then the users did not have to
> live with the bogus change in the tree while everyone was busily
> debating its merits. People very rarely call for back-outs in the
> repository since discussion generally exposes bad or controversial
> changes before the commit even happens, but on such rare occasions the
> back-out should be done without argument so that we can get immediately
> on to the topic of figuring out whether it was bogus or not.

In particular, SHA1 was never the primary reason the backout was
requested, just one design smell in the aggregate.  This change
introduces a security feature that doesn't provide the security
guarantees it claims to.  That should be a major red flag.

All the best,
Conrad