[17/21] s390x: Fix latent query-cpu-model-FOO error handling bugs
diff mbox series

Message ID 20191130194240.10517-18-armbru@redhat.com
State New
Headers show
Series
  • Error handling fixes, may contain 4.2 material
Related show

Commit Message

Markus Armbruster Nov. 30, 2019, 7:42 p.m. UTC
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.

Cc: David Hildenbrand <david@redhat.com>
Cc: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 target/s390x/cpu_models.c | 43 ++++++++++++++++++++++++---------------
 1 file changed, 27 insertions(+), 16 deletions(-)

Comments

David Hildenbrand Nov. 30, 2019, 9:22 p.m. UTC | #1
> 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. Also, please change the terminology „messed up“ to „introduced in“ or similar.

(applies to all s390x patches)

Thanks.
Aleksandar Markovic Dec. 1, 2019, 1:46 p.m. UTC | #2
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.

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.
Aleksandar Markovic Dec. 1, 2019, 2:07 p.m. UTC | #3
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.
>
>
David Hildenbrand Dec. 1, 2019, 2:09 p.m. UTC | #4
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.
Aleksandar Markovic Dec. 1, 2019, 2:11 p.m. UTC | #5
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.
>>
>>
Markus Armbruster Dec. 2, 2019, 5:01 a.m. UTC | #6
David Hildenbrand <david@redhat.com> writes:

> 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.

First search hit.  Here's my second one:

    Q: What are latent bugs?

    A: These bugs do not cause problems today. However, they are lurking
    just waiting to reveal themselves later.  The Ariane 5 rocket
    failure was caused by a float->int conversion error that lay dormant
    when previous rockets were slower; but the faster Ariane 5 triggered
    the problem.  The Millennium bug is another example of a latent bug
    that came to light when circumstances changed.  Latent bugs are much
    harder to test using conventional testing techniques, and finding
    them requires someone with foresight to ask.

http://www.geekinterview.com/question_details/36689

My point is: common usage of the term is not as clear-cut as your quote
makes it seem.

> 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.

get_val() is silent on how it wants to be used.  error.h is anything
but: it spells out how it wantes to be used in quite some detail.  In
particular:

 * Receive an error and pass it on to the caller:
 *     Error *err = NULL;
 *     foo(arg, &err);
 *     if (err) {
 *         handle the error...
 *         error_propagate(errp, err);
 *     }
 * where Error **errp is a parameter, by convention the last one.
 *
 * Do *not* "optimize" this to
 *     foo(arg, errp);
 *     if (*errp) { // WRONG!
 *         handle the error...
 *     }
 * because errp may be NULL!

My patch fixes this exact misuse of the interface.

>> 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.

Yes, a bug that can bite deserves a reproducer and a formal Fixes: tag.

The thing we're discussing (however we may want to call it) does not
have a reproducer, and I think we're in agreement that it doesn't need a
Fixes: tag.

However, my patch is not cleaning up something that's dirty, it's fixing
something that's unequivocally wrong: a violation of a stated interface
contract.

The violation happens to have no ill effects at this time due to the way
the violating code is being used.

I call that a "latent bug".  git-log has quite a few occurences of
"latent bug", by Richard Henderson, Daniel Berrangé, Paolo, ...

Your point that the commit message should not confuse people looking for
real bugs is well taken.  I think "latent bug" is clear enough, and also
concise.  I'm of course open to better phrasings.

   s390x: Fix currently harmless query-cpu-model-FOO error API violations

feels no clearer to me than

   s390x: Fix latent query-cpu-model-FOO error handling bugs

It's also too long.

I tried.  Your turn :)
David Hildenbrand Dec. 2, 2019, 8:34 a.m. UTC | #7
[...]

> First search hit.  Here's my second one:
> 
>     Q: What are latent bugs?
> 
>     A: These bugs do not cause problems today. However, they are lurking
>     just waiting to reveal themselves later.  The Ariane 5 rocket
>     failure was caused by a float->int conversion error that lay dormant
>     when previous rockets were slower; but the faster Ariane 5 triggered
>     the problem.  The Millennium bug is another example of a latent bug
>     that came to light when circumstances changed.  Latent bugs are much
>     harder to test using conventional testing techniques, and finding
>     them requires someone with foresight to ask.
> 
> http://www.geekinterview.com/question_details/36689

Google search "latent software BUG"

Hit 1: What I posted


Hit 2:
https://www.quora.com/What-are-some-examples-for-a-latent-defect-in-software-testing

"The problems will not cause the damage currently, but wait to reveal
themselves at a later time. ... E.g. February has 28 days. ... These
defects do not cause damage to the system immediately but wait for a
particular event sometime to cause damage and show their presence."

"Mostly, these types of bugs are unexpected outcome of any corner/edge
case scenarios which was executed with some specific set of test data."


Hit 3: https://sqa.stackexchange.com/questions/9170/what-is-a-latent-bug

"Latent bugs are bugs which exist, but have not yet been discovered.

They are bugs waiting to be found."

"In Software Quality Assurance:

Latent defects are the those which arises in the field, and unknown
until they reported by the field staff."


Hit 4:
https://sqa.stackexchange.com/questions/13980/what-is-the-difference-between-gold-bug-and-latent-bug

"A latent bug is a bug which is present in the system from previous
iterations or release (in your scenario Sprint 1). They are either low
priority bugs, which either went undetected or were not reported."


Same at least for Hit 5, 6, 7 (then I got tired ;) )

https://blog.qatestlab.com/2011/10/21/latent-and-masked-software-bugs-what-is-the-difference/
https://www.testing-whiz.com/blog/3-types-of-unusual-software-defects-you-should-not-miss
https://www.360logica.com/blog/latent-defect-hide-n-seek-defect-software-testing/

Which contain perfect examples and descriptions for latent bugs :)

e.g.,

"Let’s imagine that an application is able to print a document either by
laser printer or by dot matrix printer. To reach this, the application
first searches for the laser printer. In this case if it finds a laser
printer (used by default) it uses this one and prints. In case if it
does not find a laser printer, the application searches for dot matrix
printer. And if the application finds a dot matrix printer, it  gives an
error message. This unleashes a latent defect. Therefore this
application will never search for the dot matrix printer. And the
application never got tested for the dot matrix printer. That means the
accurate conditions were never met for the dot matrix printer. This is
what we call a latent software bug."

[...]

> 
>>> 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.
> 
> Yes, a bug that can bite deserves a reproducer and a formal Fixes: tag.

A "BUG that cannot be triggered" is an oxymoron. :)

The code might be "error prone", it might "violate API guidelines", it
might "not obey coding practices". If it can't be triggered, it's - by
definition - not a (latent) BUG.

> 
> The thing we're discussing (however we may want to call it) does not
> have a reproducer, and I think we're in agreement that it doesn't need a
> Fixes: tag.
> 
> However, my patch is not cleaning up something that's dirty, it's fixing
> something that's unequivocally wrong: a violation of a stated interface
> contract.

In general, I don't care about these minor details, but if somebody
literally tells the world for all eternity in a handful of patch
descriptions "David messed up the code by introducing a (latent) BUG
(that cannot be triggered)" I get suspicious. Something nicer (IMHO)
would be "Commit X introduced error prone code, let's rework that to
make it more stable in the future and obey the "error API" guidelines. I
hope you see the difference ;)

> 
> Your point that the commit message should not confuse people looking for
> real bugs is well taken.  I think "latent bug" is clear enough, and also
> concise.  I'm of course open to better phrasings.
> 
>    s390x: Fix currently harmless query-cpu-model-FOO error API violations
> 
> feels no clearer to me than
> 
>    s390x: Fix latent query-cpu-model-FOO error handling bugs
> 
> It's also too long.
> 
> I tried.  Your turn :)


s390x: Fix query-cpu-model-FOO error API violations

s390x: Make query-cpu-model-FOO error handling less error prone

s390x: Cleanup error handling in query-cpu-model-FOO

s390x: Rework error handling in query-cpu-model-FOO

...

but, enough time spent on this, feel free to continue with this however
you want.
Cornelia Huck Dec. 2, 2019, 4:31 p.m. UTC | #8
On Sat, 30 Nov 2019 20:42:36 +0100
Markus Armbruster <armbru@redhat.com> wrote:

I don't really want to restart the discussion :), but what about:

> 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. 

"It would crash when the visitor or the QOM setter fails if its @errp
argument were NULL." ?

(Hope I got my conditionals right...)

> Messed up in commit 137974cea3 's390x/cpumodel:

I agree that "Introduced" is a bit nicer than "Messed up".

> 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"'.

If we agree, I can tweak the various commit messages for the s390x
patches and apply them.

> The bugs can't bite as no caller actually passes null.  Fix them
> anyway.
> 
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Cornelia Huck <cohuck@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  target/s390x/cpu_models.c | 43 ++++++++++++++++++++++++---------------
>  1 file changed, 27 insertions(+), 16 deletions(-)

David, I don't think you gave a R-b for that one yet?
Markus Armbruster Dec. 3, 2019, 7:27 a.m. UTC | #9
David Hildenbrand <david@redhat.com> writes:

> [...]
>
>> First search hit.  Here's my second one:
>> 
>>     Q: What are latent bugs?
>> 
>>     A: These bugs do not cause problems today. However, they are lurking
>>     just waiting to reveal themselves later.  The Ariane 5 rocket
>>     failure was caused by a float->int conversion error that lay dormant
>>     when previous rockets were slower; but the faster Ariane 5 triggered
>>     the problem.  The Millennium bug is another example of a latent bug
>>     that came to light when circumstances changed.  Latent bugs are much
>>     harder to test using conventional testing techniques, and finding
>>     them requires someone with foresight to ask.
>> 
>> http://www.geekinterview.com/question_details/36689
>
> Google search "latent software BUG"

I think this argument has run its course.  Let's agree to differ on the
finer meaning and possible uses of "latent", and craft a mutually
agreeable commit message instead.  I'll reply to Cornelia's message.

[...]
Markus Armbruster Dec. 3, 2019, 7:49 a.m. UTC | #10
Cornelia Huck <cohuck@redhat.com> writes:

> On Sat, 30 Nov 2019 20:42:36 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
> I don't really want to restart the discussion :), but what about:
>
>> 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. 
>
> "It would crash when the visitor or the QOM setter fails if its @errp
> argument were NULL." ?
>
> (Hope I got my conditionals right...)

I don't think this is an improvement.

The commit message matches a pattern "what's wrong, since when, impact,
how is it fixed".  The pattern has become habit for me.  Its "what's
wrong" part is strictly local.  The non-local argument comes in only
when we assess impact.

Use of "would" in the what part's conditional signals the condition is
unlikely.  True (it's actually impossible), but distracting (because it
involves the non-local argument I'm not yet ready to make).

Let me try a different phrasing below.

>> Messed up in commit 137974cea3 's390x/cpumodel:
>
> I agree that "Introduced" is a bit nicer than "Messed up".

Works fine for me.  I didn't mean any disrespect --- I'd have to
disrespect myself: the mess corrected by PATCH 10 is mine.

>> implement QMP interface "query-cpu-model-expansion"'.
>> 
>> Its three callers have the same bug.  Messed up in commit 4e82ef0502

Feel free to call it "issue" rather than "bug".  I don't care, but David
might.

>> 's390x/cpumodel: implement QMP interface "query-cpu-model-comparison"'
>> and commit f1a47d08ef 's390x/cpumodel: implement QMP interface
>> "query-cpu-model-baseline"'.
>
> If we agree, I can tweak the various commit messages for the s390x
> patches and apply them.

Tweaking the non-s390x commit messages as well would be nicer, but
requires a respin.

Let's try to craft a mutually agreeable commit message for this patch.
Here's my attempt:

    s390x: Fix query-cpu-model-FOO error API violations

    cpu_model_from_info() is a helper for qmp_query_cpu_model_expansion(),
    qmp_query_cpu_model_comparison(), qmp_query_cpu_model_baseline().  It
    dereferences @errp when the visitor or the QOM setter fails.  That's
    wrong; see the big comment in error.h.  Introduced in commit
    137974cea3 's390x/cpumodel: implement QMP interface
    "query-cpu-model-expansion"'.

    Its three callers have the same issue.  Introduced in commit
    4e82ef0502 's390x/cpumodel: implement QMP interface
    "query-cpu-model-comparison"' and commit f1a47d08ef 's390x/cpumodel:
    implement QMP interface "query-cpu-model-baseline"'.

    No caller actually passes null.  To fix, splice in a local Error *err,
    and error_propagate().

    Cc: David Hildenbrand <david@redhat.com>
    Cc: Cornelia Huck <cohuck@redhat.com>
    Signed-off-by: Markus Armbruster <armbru@redhat.com>

Adapting it to other patches should be straightforward.

>> The bugs can't bite as no caller actually passes null.  Fix them
>> anyway.
>> 
>> Cc: David Hildenbrand <david@redhat.com>
>> Cc: Cornelia Huck <cohuck@redhat.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  target/s390x/cpu_models.c | 43 ++++++++++++++++++++++++---------------
>>  1 file changed, 27 insertions(+), 16 deletions(-)
>
> David, I don't think you gave a R-b for that one yet?
Cornelia Huck Dec. 3, 2019, 8:01 a.m. UTC | #11
On Tue, 03 Dec 2019 08:49:58 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Cornelia Huck <cohuck@redhat.com> writes:
> 
> > On Sat, 30 Nov 2019 20:42:36 +0100
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> > I don't really want to restart the discussion :), but what about:
> >  
> >> 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.   
> >
> > "It would crash when the visitor or the QOM setter fails if its @errp
> > argument were NULL." ?
> >
> > (Hope I got my conditionals right...)  
> 
> I don't think this is an improvement.
> 
> The commit message matches a pattern "what's wrong, since when, impact,
> how is it fixed".  The pattern has become habit for me.  Its "what's
> wrong" part is strictly local.  The non-local argument comes in only
> when we assess impact.
> 
> Use of "would" in the what part's conditional signals the condition is
> unlikely.  True (it's actually impossible), but distracting (because it
> involves the non-local argument I'm not yet ready to make).

I think we'll have to agree to disagree here...

> 
> Let me try a different phrasing below.

...but also see below :)

> 
> >> Messed up in commit 137974cea3 's390x/cpumodel:  
> >
> > I agree that "Introduced" is a bit nicer than "Messed up".  
> 
> Works fine for me.  I didn't mean any disrespect --- I'd have to
> disrespect myself: the mess corrected by PATCH 10 is mine.
> 
> >> implement QMP interface "query-cpu-model-expansion"'.
> >> 
> >> Its three callers have the same bug.  Messed up in commit 4e82ef0502  
> 
> Feel free to call it "issue" rather than "bug".  I don't care, but David
> might.
> 
> >> 's390x/cpumodel: implement QMP interface "query-cpu-model-comparison"'
> >> and commit f1a47d08ef 's390x/cpumodel: implement QMP interface
> >> "query-cpu-model-baseline"'.  
> >
> > If we agree, I can tweak the various commit messages for the s390x
> > patches and apply them.  
> 
> Tweaking the non-s390x commit messages as well would be nicer, but
> requires a respin.
> 
> Let's try to craft a mutually agreeable commit message for this patch.
> Here's my attempt:
> 
>     s390x: Fix query-cpu-model-FOO error API violations
> 
>     cpu_model_from_info() is a helper for qmp_query_cpu_model_expansion(),
>     qmp_query_cpu_model_comparison(), qmp_query_cpu_model_baseline().  It
>     dereferences @errp when the visitor or the QOM setter fails.  That's
>     wrong; see the big comment in error.h.  Introduced in commit
>     137974cea3 's390x/cpumodel: implement QMP interface
>     "query-cpu-model-expansion"'.
> 
>     Its three callers have the same issue.  Introduced in commit
>     4e82ef0502 's390x/cpumodel: implement QMP interface
>     "query-cpu-model-comparison"' and commit f1a47d08ef 's390x/cpumodel:
>     implement QMP interface "query-cpu-model-baseline"'.
> 
>     No caller actually passes null.  To fix, splice in a local Error *err,
>     and error_propagate().
> 
>     Cc: David Hildenbrand <david@redhat.com>
>     Cc: Cornelia Huck <cohuck@redhat.com>
>     Signed-off-by: Markus Armbruster <armbru@redhat.com>

That sounds good to me.

> 
> Adapting it to other patches should be straightforward.

Ok, so how to proceed? I'm happy to tweak the commit messages for
s390x, but that is bound to get messy.

> 
> >> The bugs can't bite as no caller actually passes null.  Fix them
> >> anyway.
> >> 
> >> Cc: David Hildenbrand <david@redhat.com>
> >> Cc: Cornelia Huck <cohuck@redhat.com>
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> ---
> >>  target/s390x/cpu_models.c | 43 ++++++++++++++++++++++++---------------
> >>  1 file changed, 27 insertions(+), 16 deletions(-)  
> >
> > David, I don't think you gave a R-b for that one yet?
David Hildenbrand Dec. 3, 2019, 9:51 a.m. UTC | #12
>>     s390x: Fix query-cpu-model-FOO error API violations
>>
>>     cpu_model_from_info() is a helper for qmp_query_cpu_model_expansion(),
>>     qmp_query_cpu_model_comparison(), qmp_query_cpu_model_baseline().  It
>>     dereferences @errp when the visitor or the QOM setter fails.  That's
>>     wrong; see the big comment in error.h.  Introduced in commit
>>     137974cea3 's390x/cpumodel: implement QMP interface
>>     "query-cpu-model-expansion"'.
>>
>>     Its three callers have the same issue.  Introduced in commit
>>     4e82ef0502 's390x/cpumodel: implement QMP interface
>>     "query-cpu-model-comparison"' and commit f1a47d08ef 's390x/cpumodel:
>>     implement QMP interface "query-cpu-model-baseline"'.
>>
>>     No caller actually passes null.  To fix, splice in a local Error *err,
>>     and error_propagate().
>>
>>     Cc: David Hildenbrand <david@redhat.com>
>>     Cc: Cornelia Huck <cohuck@redhat.com>
>>     Signed-off-by: Markus Armbruster <armbru@redhat.com>
> 
> That sounds good to me.

Yes, something I would have enjoyed reading from the first sentence.

Reviewed-by: David Hildenbrand <david@redhat.com>

Patch
diff mbox series

diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index c702e34a26..3ed301b5e5 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -477,6 +477,7 @@  CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp)
 static void cpu_model_from_info(S390CPUModel *model, const CpuModelInfo *info,
                                 Error **errp)
 {
+    Error *err = NULL;
     const QDict *qdict = NULL;
     const QDictEntry *e;
     Visitor *visitor;
@@ -513,24 +514,26 @@  static void cpu_model_from_info(S390CPUModel *model, const CpuModelInfo *info,
 
     if (qdict) {
         visitor = qobject_input_visitor_new(info->props);
-        visit_start_struct(visitor, NULL, NULL, 0, errp);
-        if (*errp) {
+        visit_start_struct(visitor, NULL, NULL, 0, &err);
+        if (err) {
+            error_propagate(errp, err);
             visit_free(visitor);
             object_unref(obj);
             return;
         }
         for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) {
-            object_property_set(obj, visitor, e->key, errp);
-            if (*errp) {
+            object_property_set(obj, visitor, e->key, &err);
+            if (err) {
                 break;
             }
         }
-        if (!*errp) {
+        if (!err) {
             visit_check_struct(visitor, errp);
         }
         visit_end_struct(visitor, NULL);
         visit_free(visitor);
-        if (*errp) {
+        if (err) {
+            error_propagate(errp, err);
             object_unref(obj);
             return;
         }
@@ -595,13 +598,15 @@  CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
                                                       CpuModelInfo *model,
                                                       Error **errp)
 {
+    Error *err = NULL;
     CpuModelExpansionInfo *expansion_info = NULL;
     S390CPUModel s390_model;
     bool delta_changes = false;
 
     /* convert it to our internal representation */
-    cpu_model_from_info(&s390_model, model, errp);
-    if (*errp) {
+    cpu_model_from_info(&s390_model, model, &err);
+    if (err) {
+        error_propagate(errp, err);
         return NULL;
     }
 
@@ -634,18 +639,21 @@  CpuModelCompareInfo *qmp_query_cpu_model_comparison(CpuModelInfo *infoa,
                                                      CpuModelInfo *infob,
                                                      Error **errp)
 {
+    Error *err = NULL;
     CpuModelCompareResult feat_result, gen_result;
     CpuModelCompareInfo *compare_info;
     S390FeatBitmap missing, added;
     S390CPUModel modela, modelb;
 
     /* convert both models to our internal representation */
-    cpu_model_from_info(&modela, infoa, errp);
-    if (*errp) {
+    cpu_model_from_info(&modela, infoa, &err);
+    if (err) {
+        error_propagate(errp, err);
         return NULL;
     }
-    cpu_model_from_info(&modelb, infob, errp);
-    if (*errp) {
+    cpu_model_from_info(&modelb, infob, &err);
+    if (err) {
+        error_propagate(errp, err);
         return NULL;
     }
     compare_info = g_new0(CpuModelCompareInfo, 1);
@@ -707,6 +715,7 @@  CpuModelBaselineInfo *qmp_query_cpu_model_baseline(CpuModelInfo *infoa,
                                                     CpuModelInfo *infob,
                                                     Error **errp)
 {
+    Error *err = NULL;
     CpuModelBaselineInfo *baseline_info;
     S390CPUModel modela, modelb, model;
     uint16_t cpu_type;
@@ -714,13 +723,15 @@  CpuModelBaselineInfo *qmp_query_cpu_model_baseline(CpuModelInfo *infoa,
     uint8_t max_gen;
 
     /* convert both models to our internal representation */
-    cpu_model_from_info(&modela, infoa, errp);
-    if (*errp) {
+    cpu_model_from_info(&modela, infoa, &err);
+    if (err) {
+        error_propagate(errp, err);
         return NULL;
     }
 
-    cpu_model_from_info(&modelb, infob, errp);
-    if (*errp) {
+    cpu_model_from_info(&modelb, infob, &err);
+    if (err) {
+        error_propagate(errp, err);
         return NULL;
     }