linux-kernel - Re: [PATCH 3/3] selftests: KVM: Add test case for MMIO during event delivery

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <ZxFCG7pxWXs1D0p5@google.com>
Date: Thu, 17 Oct 2024 09:58:03 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: David Woodhouse <dwmw2@...radead.org>
Cc: Ivan Orlov <iorlov@...zon.com>, bp@...en8.de, dave.hansen@...ux.intel.com, 
	mingo@...hat.com, pbonzini@...hat.com, shuah@...nel.org, tglx@...utronix.de, 
	hpa@...or.com, kvm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-kselftest@...r.kernel.org, x86@...nel.org, jalliste@...zon.com, 
	nh-open-source@...zon.com, pdurrant@...zon.co.uk
Subject: Re: [PATCH 3/3] selftests: KVM: Add test case for MMIO during event delivery

On Thu, Oct 17, 2024, David Woodhouse wrote:
> On Fri, 2024-10-11 at 17:21 -0700, Sean Christopherson wrote:
> > 
> > > +
> > > +       /* We should never reach this point */
> > 
> > No pronouns.  Yes, it's nitpicky, but "we" gets _very_ ambiguous when "we" could
> > mean the admin, the user, the VMM, KVM, the guest, etc.
> > 
> > > +       GUEST_ASSERT(0);
> 
> 
> Is there really *any* way that can be interpreted as anything other
> than "the CPU executing this code will never get to this point and
> that's why there's an ASSERT(0) right after this comment"?
> 
> I don't believe there's *any* way that particular pronoun can be
> ambiguous, and now we've got to the point of fetishising the bizarre
> "no pronouns" rule just for the sake of it.

No, it's not just for the sake of it.  In this case, "we" isn't all that ambiguous,
(though my interpretation of it is "the test", not "the CPU"), but only because the
comment is utterly useless.  The GUEST_ASSERT(0) communicates very clearly that it's
supposed to be unreachable.

And if the comment were rewritten to explain _why_ the code is unreachable, then
"we" is all bug guaranateed to become ambiguous, because explaining "why" likely
means preciesly describing the behavior the userspace side, the guest side, and/or
KVM.  In other words, using "we" or "us" is often a hint that either the statement
is likely ambiguous or doesn't add value.

And irrespective of whether or not you agree with the above, having a hard rule of
"no we, no us" eliminates all subjectivity, and for me that is sufficient reason
to enforce the rule.

> I get it, especially for some individuals it *can* be difficult to take
> context into account, and the wilful use of pronouns instead of
> spelling things out explicitly *every* *single* *time* can sometimes
> help. But at a cost of conciseness and brevity.

In this particular case, I am more than willing to sacrifice brevity.  I 100%
agree that there is value in having to-the-point comments and changelogs, but I
can't recall a single time where avoiding a "we" or "us" made a statement
meaningfully harder to read and understand.  On ther hand, I can recall many, many
changelogs I had to re-read multiple times because I struggled to figure out how
the author _intended_ "we" or "us" to be interpreted.