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

[Aleksandar Markovic <aleksandar.m.mail@gmail.com>]

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

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

>
>
> 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.
>
>
Actually, the message title already does use "latent .... bugs". So it is
fine - in my opinion.



> 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: 5017 bytes --]