Re: [PATCH v7 03/11] scripts: add coccinelle script to use auto propagated errp

[Markus Armbruster <armbru@redhat.com>]

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> 25.02.2020 15:52, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>
>>> 23.02.2020 11:55, Markus Armbruster wrote:
>>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>>>
>>>>> Script adds ERRP_AUTO_PROPAGATE macro invocation where appropriate and
>>>>> does corresponding changes in code (look for details in
>>>>> include/qapi/error.h)
>>>>>
>>>>> Usage example:
>>>>> spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
>>>>>    --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
>>>>>    blockdev-nbd.c qemu-nbd.c {block/nbd*,nbd/*,include/block/nbd*}.[hc]
>>>>>
>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>> ---
>>>>>
>>>>> CC: Eric Blake <eblake@redhat.com>
>>>>> CC: Kevin Wolf <kwolf@redhat.com>
>>>>> CC: Max Reitz <mreitz@redhat.com>
>>>>> CC: Greg Kurz <groug@kaod.org>
>>>>> CC: Stefano Stabellini <sstabellini@kernel.org>
>>>>> CC: Anthony Perard <anthony.perard@citrix.com>
>>>>> CC: Paul Durrant <paul@xen.org>
>>>>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>>>>> CC: "Philippe Mathieu-Daudé" <philmd@redhat.com>
>>>>> CC: Laszlo Ersek <lersek@redhat.com>
>>>>> CC: Gerd Hoffmann <kraxel@redhat.com>
>>>>> CC: Stefan Berger <stefanb@linux.ibm.com>
>>>>> CC: Markus Armbruster <armbru@redhat.com>
>>>>> CC: Michael Roth <mdroth@linux.vnet.ibm.com>
>>>>> CC: qemu-block@nongnu.org
>>>>> CC: xen-devel@lists.xenproject.org
>>>>>
>>>>>    include/qapi/error.h                          |   3 +
>>>>>    scripts/coccinelle/auto-propagated-errp.cocci | 158 ++++++++++++++++++
>>>>>    2 files changed, 161 insertions(+)
>>>>>    create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci
>>>>>
>>>>> diff --git a/include/qapi/error.h b/include/qapi/error.h
>>>>> index b9452d4806..79f8e95214 100644
>>>>> --- a/include/qapi/error.h
>>>>> +++ b/include/qapi/error.h
>>>>> @@ -141,6 +141,9 @@
>>>>>     *         ...
>>>>>     *     }
>>>>>     *
>>>>> + * For mass conversion use script
>>>>> + *   scripts/coccinelle/auto-propagated-errp.cocci
>>>>> + *
>>>>>     *
>>>>>     * Receive and accumulate multiple errors (first one wins):
>>>>>     *     Error *err = NULL, *local_err = NULL;
>>>>
>>>> Extra blank line.
>>>>
>>>>> diff --git a/scripts/coccinelle/auto-propagated-errp.cocci b/scripts/coccinelle/auto-propagated-errp.cocci
>>>>> new file mode 100644
>>>>> index 0000000000..fb03c871cb
>>>>> --- /dev/null
>>>>> +++ b/scripts/coccinelle/auto-propagated-errp.cocci
>>>>> @@ -0,0 +1,158 @@
>>>>> +// Use ERRP_AUTO_PROPAGATE (see include/qapi/error.h)
>>>>> +//
>>>>> +// Copyright (c) 2020 Virtuozzo International GmbH.
>>>>> +//
>>>>> +// This program is free software; you can redistribute it and/or modify
>>>>> +// it under the terms of the GNU General Public License as published by
>>>>> +// the Free Software Foundation; either version 2 of the License, or
>>>>> +// (at your option) any later version.
>>>>> +//
>>>>> +// This program is distributed in the hope that it will be useful,
>>>>> +// but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>>> +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>>>> +// GNU General Public License for more details.
>>>>> +//
>>>>> +// You should have received a copy of the GNU General Public License
>>>>> +// along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>>>> +//
>>>>> +// Usage example:
>>>>> +// spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
>>>>> +//  --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
>>>>> +//  blockdev-nbd.c qemu-nbd.c {block/nbd*,nbd/*,include/block/nbd*}.[hc]
>>>>> +
>>>>> +@rule0@
>>>>> +// Add invocation to errp-functions where necessary
>>>>> +// We should skip functions with "Error *const *errp"
>>>>> +// parameter, but how to do it with coccinelle?
>>>>> +// I don't know, so, I skip them by function name regex.
>>>>> +// It's safe: if we did not skip some functions with
>>>>> +// "Error *const *errp", ERRP_AUTO_PROPAGATE invocation
>>>>> +// will fail to compile, because of const violation.
>>>>
>>>> Not skipping a function we should skip fails to compile.
>>>>
>>>> What about skipping a function we should not skip?
>>>
>>> Then it will not be updated.. Not good but I don't have better solution.
>>> Still, I hope, function called *error_append_*_hint will not return error
>>> through errp pointer.
>>
>> Seems likely.  I just dislike inferring behavior from name patterns.
>>
>> Ideally, we recognize the true exceptional pattern instead, i.e. the
>> presence of const.  But figuring out how to make Coccinelle do that for
>> us may be more trouble than it's worth.
>>
>> Hmm...  Coccinelle matches the parameter even with const due to what it
>> calls "isomorphism".  Can I disable it?  *Tinker* *tinker*
>>
>> diff --git a/scripts/coccinelle/auto-propagated-errp.cocci b/scripts/coccinelle/auto-propagated-errp.cocci
>> index fb03c871cb..0c4414bff3 100644
>> --- a/scripts/coccinelle/auto-propagated-errp.cocci
>> +++ b/scripts/coccinelle/auto-propagated-errp.cocci
>> @@ -20,15 +20,11 @@
>>   //  --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
>>   //  blockdev-nbd.c qemu-nbd.c {block/nbd*,nbd/*,include/block/nbd*}.[hc]
>>   -@rule0@
>> +@rule0 disable optional_qualifier@
>>   // Add invocation to errp-functions where necessary
>> -// We should skip functions with "Error *const *errp"
>> -// parameter, but how to do it with coccinelle?
>> -// I don't know, so, I skip them by function name regex.
>> -// It's safe: if we did not skip some functions with
>> -// "Error *const *errp", ERRP_AUTO_PROPAGATE invocation
>> -// will fail to compile, because of const violation.
>> -identifier fn !~ "error_append_.*_hint";
>> +// Disable optional_qualifier to skip functions with "Error *const *errp"
>> +// parameter,
>> +identifier fn;
>>   identifier local_err, ERRP;
>>   @@
>>
>> Could you verify this results in the same tree-wide change as your
>> version?
>
> Yes, no difference. Thanks!

Excellent!

[...]
>> Let's see whether I got it:
>>
>> * The first rule (rule0) adds ERRP_AUTO_PROPAGATE() to all functions
>>    that take an Error ** parameter, and either pass it error_prepend() or
>>    error_append_hint(), or use local_err, and don't have
>>    ERRP_AUTO_PROPAGATE() already, except it skips the ones named
>>    error_append_FOO_hint().  Uff.
>>
>>    The "use local_err" part is an approximation of "use local_err +
>>    error_propagate()".
>>
>>    The "except for the ones named error_append_FOO_hint()" part is an
>>    approximation of "except for the ones taking an Error *const *
>>    parameter".
>>
>>    ERRP_AUTO_PROPAGATE() requires the Error ** parameter to be named
>>    @errp, which need not be the case.  The next rule fixes it up:
>>
>> * The second rule ensures the parameter is named @errp wherever the
>>    first rule applied, renaming if necessary.
>>
>>    Correct?
>>
>>    Incorrect transformation followed by fixup is not ideal, because it
>>    can trip up reviewers.  But ideal is too expensive; this is good
>>    enough.
>>
>> * The third rule (rule1) ensures functions that take an Error **errp
>>    parameter don't declare local_err, except it skips the ones named
>>    error_append_FOO_hint().
>>
>>    In isolation, this rule makes no sense.  To make sense of it, we need
>>    context:
>>
>>    * Subsequent rules remove all uses of @errp from any function where
>
> of local_err
>
>>      rule1 matches.
>>
>>    * Preceding rules ensure any function where rule1 matches has
>>      ERRP_AUTO_PROPAGATE().
>>
>>    Correct?
>
> Oh, yes, everything is correct.

Thank you!

>>
>>    The need for this much context is hard on reviewers.  Good enough for
>>    transforming the tree now, but I'd hate having to make sense of this
>>    again in six months.
>
> Ohh, yes. Far from good design. I can try to reorder chunks a bit.

Please don't spend too much effort on it.  The script is primarily for
helping us convert the whole tree within a short time span.  We may also
use it later to convert instances of the old pattern that have crept
back.  We hopefully won't have to change the script then.  Readability
is not as important as it is for code we expect to be read again and
again over a long time.

[...]