Re: [PATCH v6 07/12] KVM: arm64: Export kvm_are_all_memslots_empty()

[Sean Christopherson <seanjc@google.com>]

On Tue, Mar 14, 2023, Marc Zyngier wrote:
> On Mon, 13 Mar 2023 15:18:55 +0000,
> Sean Christopherson <seanjc@google.com> wrote:
> > 
> > On Sun, Mar 12, 2023, Marc Zyngier wrote:
> > > On Tue, 07 Mar 2023 03:45:50 +0000,
> > > Ricardo Koller <ricarkol@google.com> wrote:
> > > > No functional change intended.
> > > 
> > > I wish people stopped adding this pointless sentence to commit
> > > messages. All changes have a functional change one way or another,
> > > unless you are only changing a comment.
> > 
> > The implied context is that there is no change in runtime functionality, which
> > does hold true for many changes.  I personally find the annotation helpful, both
> > for code review and when doing git archaeology.  If a changelog states that the
> > author doesn't/didn't intend a functional change, then _any_ change in (runtime)
> > functionality becomes a red flag, and for me, prompts a closer look regardless of
> > whether or not I have other concerns with the patch/commit.
> 
> And I think it lures the reviewer into a false sense of security. No
> intended change, must be fine. Except when it is not. More often than
> not, we end-up with seemingly innocent changes that break things.
> 
> It is even worse when things get (for good or bad reasons) backported
> to -stable or an internal tree of some description. "No functional
> change" can become something very different in another context. How do
> you communicate this?

For KVM x86, we opt out of AUTOSEL, so barring errors elsewhere in the process,
a maintainer needs to review such patches at some point.  And again, for me,
sending a patch to stable that was intended to be a nop is a flag that the backport
warrants a closer look, e.g. extra justification for why a patch that's (allegedly)
a nop needs to be backported to stable kernels.

I agree it's imperfect, e.g could lead downstream maintainers astray if they view
the disclaimer as a statement of truth as opposed to be a statement of intent.
But IMO the good things that come from being able to know the author's intent far
outweigh the probability of bad things happening because a reviewer and/or downstream
consumer put too much weight on the statement.

My opinion is certainly influenced by having spent far too much time digging through
historical KVM x86 commits, where it's all too often unclear if a buggy/flawed
commit was simply a coding goof, versus the commit doing exactly what the author
intended and being broken because of bad assumptions, incorrect interpretation of
a spec, etc.  But even with that bias in mind, I still think explicitly declaring
an author's intent for these types of changes is overall a net positive.

Anywho, I don't mean to step on toes and force my preferences down everyone's
throats, just wanted to provide my reasoning for including the disclaimer and
encouraging other KVM x86 contributors to do the same.