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

[David Hildenbrand]

On 01.12.19 14:46, Aleksandar Markovic wrote:
> 
> 
> On Saturday, November 30, 2019, David Hildenbrand <address@hidden
> <mailto:address@hidden>> wrote:
> 
> 
> 
>     > Am 30.11.2019 um 20:42 schrieb Markus Armbruster
>     <address@hidden <mailto:address@hidden>>:
>     >
>     > 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