Re: [PATCH 17/21] s390x: Fix latent query-cpu-model-FOO error handling bugs

[Cornelia Huck <cohuck@redhat.com>]

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?