On Sunday, December 1, 2019, Aleksandar Markovic <address@hidden> wrote:
On Saturday, November 30, 2019, David Hildenbrand <address@hidden> wrote:
> Am 30.11.2019 um 20:42 schrieb Markus Armbruster <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
„ 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.