[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 17/21] s390x: Fix latent query-cpu-model-FOO error handling b
From: |
Cornelia Huck |
Subject: |
Re: [PATCH 17/21] s390x: Fix latent query-cpu-model-FOO error handling bugs |
Date: |
Tue, 3 Dec 2019 09:01:07 +0100 |
On Tue, 03 Dec 2019 08:49:58 +0100
Markus Armbruster <address@hidden> wrote:
> Cornelia Huck <address@hidden> writes:
>
> > On Sat, 30 Nov 2019 20:42:36 +0100
> > Markus Armbruster <address@hidden> 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 <address@hidden>
> Cc: Cornelia Huck <address@hidden>
> Signed-off-by: Markus Armbruster <address@hidden>
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 <address@hidden>
> >> Cc: Cornelia Huck <address@hidden>
> >> Signed-off-by: Markus Armbruster <address@hidden>
> >> ---
> >> 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?