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

[Vladimir Sementsov-Ogievskiy]

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 <address@hidden>
> 


-- 
Best regards,
Vladimir