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

From: Aleksandar Markovic <aleksandar.m.mail@gmail.com>
To: David Hildenbrand <dhildenb@redhat.com>
Cc: David Hildenbrand <david@redhat.com>,
	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:07:16 +0100	[thread overview]
Message-ID: <CAL1e-=jP3kYhxSFsGg2=w2rAK8mfMBFg5MvvFZd_4z_t3LSmcA@mail.gmail.com> (raw)
In-Reply-To: <CAL1e-=jst9hGBXy0zm-975QDvW0F0xBNJAypqM4KooWEUvJfjQ@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 2914 bytes --]

On Sunday, December 1, 2019, Aleksandar Markovic <
aleksandar.m.mail@gmail.com> wrote:

>
>
> On Saturday, November 30, 2019, David Hildenbrand <dhildenb@redhat.com>
> wrote:
>
>>
>>
>> > Am 30.11.2019 um 20:42 schrieb Markus Armbruster <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
>>
>>   „ 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.
>
>
Oops, I didn't even realize that the patch title contains the word
"latent". (I wrote the previous message without that knowledge. For some
strange reason, my email client doesn't display email subject while
replying.)

In this case, I would suggest usage of phrase "latent bug" instead of
"latent error" or so in the message title, to strenghten the point that
this is not a cleanup.

Yours, Aleksandar

> 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.
>
> David, if I understand well, this patch fixes the commit done by you. I
> definitely understand this is not a pleasant position, but we all
> (definitelly including myself too) should learn to handle such situations
> as gracefully as we can.
>
> Yours,
> Aleksandar
>
>
>
>>
>>
>>
>> Also, please change the terminology „messed up“ to „introduced in“ or
>> similar.
>>
>> (applies to all s390x patches)
>>
>> Thanks.
>
>

[-- Attachment #2: Type: text/html, Size: 4458 bytes --]