Re: [PATCH 17/21] s390x: Fix latent query-cpu-model-FOO error handling bugs

From: David Hildenbrand <david@redhat.com>
To: Aleksandar Markovic <aleksandar.m.mail@gmail.com>,
	David Hildenbrand <dhildenb@redhat.com>
Cc: Cornelia Huck <cohuck@redhat.com>,
	"vsementsov@virtuozzo.com" <vsementsov@virtuozzo.com>,
	Markus Armbruster <armbru@redhat.com>,
	"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>
Subject: Re: [PATCH 17/21] s390x: Fix latent query-cpu-model-FOO error handling bugs
Date: Sun, 1 Dec 2019 15:09:04 +0100	[thread overview]
Message-ID: <d9eb5b70-8396-7537-aac5-24c4d872d553@redhat.com> (raw)
In-Reply-To: <CAL1e-=jst9hGBXy0zm-975QDvW0F0xBNJAypqM4KooWEUvJfjQ@mail.gmail.com>

On 01.12.19 14:46, Aleksandar Markovic wrote:
> 
> 
> On Saturday, November 30, 2019, David Hildenbrand <dhildenb@redhat.com
> <mailto:dhildenb@redhat.com>> wrote:
> 
> 
> 
>     > Am 30.11.2019 um 20:42 schrieb Markus Armbruster
>     <armbru@redhat.com <mailto:armbru@redhat.com>>:
>     >
>     > cpu_model_from_info() is a helper for
>     qmp_query_cpu_model_expansion(),
>     > qmp_query_cpu_model_comparison(), qmp_query_cpu_model_baseline().  It
>     > crashes when the visitor or the QOM setter fails, and its @errp
>     > argument is null.  Messed up in commit 137974cea3 's390x/cpumodel:
>     > implement QMP interface "query-cpu-model-expansion"'.
>     >
>     > Its three callers have the same bug.  Messed up in commit 4e82ef0502
>     > 's390x/cpumodel: implement QMP interface "query-cpu-model-comparison"'
>     > and commit f1a47d08ef 's390x/cpumodel: implement QMP interface
>     > "query-cpu-model-baseline"'.
>     >
>     > The bugs can't bite as no caller actually passes null.  Fix them
>     > anyway.
> 
>     https://en.m.wikipedia.org/wiki/Software_bug
>     <https://en.m.wikipedia.org/wiki/Software_bug>
> 
>       „ A software bug is an error, flaw or fault in a computer program
>     or system that causes it to produce an incorrect or unexpected
>     result, or to behave in unintended ways. „
> 
>     Please make it clear in the descriptions that these are cleanups and
>     not bugfixes. It might be very confusing for people looking out for
>     real bugs.
> 
> 
> 
> Disclaimer: I am not entirely familiar with the code in question, so
> take my opinion with reasonablereservation.
> 
> It looks that we here deal with latent bugs. As you probably know from
> experience, a latent bugs, when they are activated with some ostensibly
> unrelated code change, can be much more difficult to diagnose and fix
> than regular bugs.

"https://economictimes.indiatimes.com/definition/latent-bug

"Definition: An uncovered or unidentified bug which exists in the system
over a period of time is known as the Latent Bug. The bug may persist in
the system in one or more versions of the software."

AFAIK, a latent BUG can be triggered, it simply was never triggered.

Do you think the following code is buggy?

static int get_val(int *ptr)
{
	return *ptr;
}

int main()
{
	int a = 0;

	return get_val(&a);
}

I claim, no, although we could access a NULL pointer if ever reworked.
There is no invalid system state possible.

> 
> In that light, this change is not a clean up. It is a fix of a latent
> bugs, and Markus' aproach to treat it as a bug fix looks right to me. I
> would just add a word "latent" or similar, which would even more
> distance the patch from "cleanup" meaning.

I agree iff there is some way to trigger it. Otherwise, to me it is a
cleanup.If it's a BUG, it deserves proper Fixes tags and some
description how it can be triggered.

-- 
Thanks,

David / dhildenb