| Message ID | 20191130194240.10517-18-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Error handling fixes, may contain 4.2 material | expand |
> 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.
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.
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. > >
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.
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. >> >>
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 :)
[...] > 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.
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?
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. [...]
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?
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?
>> 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>
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; }
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(-)