Re: [PATCH v4 05/31] scripts: add script to fix error_append_hint/error_prepend usage

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

01.10.2019 19:22, Eric Blake wrote:
> On 10/1/19 10:52 AM, Vladimir Sementsov-Ogievskiy wrote:
>> error_append_hint and error_prepend will not work, if errp ==
>> &fatal_error, as program will exit before error_append_hint or
>> error_prepend call. Fix this by use of special macro
>> ERRP_AUTO_PROPAGATE.
> 
> This patch doesn't actually fix any code, but adds the script to enable automating the fixing of the code in subsequent patches.  Tweaking the commit message to make that point clear might be helpful.

Hmm, so, maybe, switch "Fix this by use ..." to

To fix code with help of this script do
spatch --sp-file scripts/coccinelle/fix-error-add-info.cocci FILE_TO_FIX


> 
> 
>>   scripts/coccinelle/fix-error-add-info.cocci | 28 +++++++++++++++++++++
>>   1 file changed, 28 insertions(+)
>>   create mode 100644 scripts/coccinelle/fix-error-add-info.cocci
>>
>> diff --git a/scripts/coccinelle/fix-error-add-info.cocci b/scripts/coccinelle/fix-error-add-info.cocci
>> new file mode 100644
>> index 0000000000..34fa3be720
>> --- /dev/null
>> +++ b/scripts/coccinelle/fix-error-add-info.cocci
>> @@ -0,0 +1,28 @@
>> +@rule0@
>> +// Add invocation to errp-functions
>> +identifier fn;
>> +@@
>> +
>> + fn(..., Error **errp, ...)
>> + {
>> ++   ERRP_AUTO_PROPAGATE();
>> +    <+...
>> +(
>> +    error_append_hint(errp, ...);
>> +|
>> +    error_prepend(errp, ...);
>> +)
> 
> So, for now, you aren't addressing any *errp usage, or any potential cleanups of error_propagate.  But that's okay; your cover letter did call out that you were only addressing 1 part out of 3 potential uses just to get some motion, based on the size of the effort.
> 
>> +    ...+>
>> + }
>> +
>> +@@
>> +// Drop doubled invocation
>> +identifier rule0.fn;
>> +@@
>> +
>> + fn(...)
>> +{
>> +    ERRP_AUTO_PROPAGATE();
>> +-   ERRP_AUTO_PROPAGATE();
>> +    ...
>> +}
>>
> 
> This looks idempotent once a file is patched, and safe to rerun as many times in the future as needed.  I'm still hoping we can find a way to make scripts/checkpatch.pl also do a sanity check, but as it's harder to parse C in perl than in Coccinelle, I can live with just the .cocci script in-tree as long as we remember to manually run it periodically.

scripts/checkpatch.pl is so unfriendly.. And to run coccinelle scripts we need
working directory, not just patch files.

I imagine the following:

move scripts/checkpatch.pl to scripts/checkpatch dir

Create scripts/checkpatch.py (or python/checkpatch.py, I don't
know what is the idea of python/ dir), which will work only on
commits or on patch files with specified base (master by default),
it will create temporary worktree, checkout corresponding commit
and then run scripts from scripts/checkpatch/, at least it would be
checkpatch.pl and coccinelle.sh (which will run some coccinelle
scripts from coccinelle dir)..

> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 


-- 
Best regards,
Vladimir