On Tue, 03 Dec 2019 08:49:58 +0100 Markus Armbruster <arm...@redhat.com> wrote:
> Cornelia Huck <coh...@redhat.com> writes: > > > On Sat, 30 Nov 2019 20:42:36 +0100 > > Markus Armbruster <arm...@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 <da...@redhat.com> > Cc: Cornelia Huck <coh...@redhat.com> > Signed-off-by: Markus Armbruster <arm...@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 <da...@redhat.com> > >> Cc: Cornelia Huck <coh...@redhat.com> > >> Signed-off-by: Markus Armbruster <arm...@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?
