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

From: Markus Armbruster <armbru@redhat.com>
To: Cornelia Huck <cohuck@redhat.com>
Cc: vsementsov@virtuozzo.com, qemu-devel@nongnu.org,
	David Hildenbrand <david@redhat.com>
Subject: Re: [PATCH 17/21] s390x: Fix latent query-cpu-model-FOO error handling bugs
Date: Tue, 03 Dec 2019 08:49:58 +0100	[thread overview]
Message-ID: <87sgm17srd.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <20191202173147.3085b1a6.cohuck@redhat.com> (Cornelia Huck's message of "Mon, 2 Dec 2019 17:31:47 +0100")

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?