Re: [Xen-devel] [PATCH v8 02/10] scripts: add coccinelle script to use auto propagated errp

[Markus Armbruster <armbru@xxxxxxxxxx>]

Vladimir Sementsov-Ogievskiy <vsementsov@xxxxxxxxxxxxx> writes:

> 11.03.2020 12:38, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@xxxxxxxxxxxxx> writes:
>>
>>> 09.03.2020 12:56, Markus Armbruster wrote:
>>>> Suggest
>>>>
>>>>       scripts: Coccinelle script to use auto-propagated errp
>>>>
>>>> or
>>>>
>>>>       scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE()
>>>>
>>>> Vladimir Sementsov-Ogievskiy <vsementsov@xxxxxxxxxxxxx> writes:
>> [...]
>>>>> +// Note, that we update everything related to matched by rule1 function 
>>>>> name
>>>>> +// and local_err name. We may match something not related to the pattern
>>>>> +// matched by rule1. For example, local_err may be defined with the same 
>>>>> name
>>>>> +// in different blocks inside one function, and in one block follow the
>>>>> +// propagation pattern and in other block doesn't. Or we may have several
>>>>> +// functions with the same name (for different configurations).
>>>>
>>>> Context: rule1 matches functions that have all three of
>>>>
>>>> * an Error **errp parameter
>>>>
>>>> * an Error *local_err = NULL variable declaration
>>>>
>>>> * an error_propagate(errp, local_err) or error_propagate_prepend(errp,
>>>>     local_err, ...) expression, where @errp is the parameter and
>>>>     @local_err is the variable.
>>>>
>>>> If I understand you correctly, you're pointing out two potential issues:
>>>>
>>>> 1. This rule can match functions rule1 does not match if there is
>>>> another function with the same name that rule1 does match.
>>>>
>>>> 2. This rule matches in the entire function matched by rule1, even when
>>>> parts of that function use a different @errp or @local_err.
>>>>
>>>> I figure these apply to all rules with identifier rule1.fn, not just
>>>> this one.  Correct?
>>>>
>>>> Regarding 1.  There must be a better way to chain rules together, but I
>>>> don't know it.
>>>
>>> Hmm, what about something like this:
>>>
>>> @rule1 disable optional_qualifier exists@
>>> identifier fn, local_err;
>>> symbol errp;
>>> @@
>>>
>>>   fn(..., Error **
>>> - errp
>>> + ___errp_coccinelle_updating___
>>>      , ...)
>>>   {
>>>       ...
>>>       Error *local_err = NULL;
>>>       ...
>>> (
>>>      error_propagate_prepend(errp, local_err, ...);
>>> |
>>>      error_propagate(errp, local_err);
>>> )
>>>       ...
>>>   }
>>>
>>>
>>> [..]
>>>
>>> match symbol ___errp_coccinelle_updating___ in following rules in function 
>>> header
>>>
>>> [..]
>>>
>>>
>>> @ disable optional_qualifier@
>>> identifier fn, local_err;
>>> symbol errp;
>>> @@
>>>
>>>   fn(..., Error **
>>> - ___errp_coccinelle_updating___
>>> + errp
>>>      , ...)
>>>   {
>>>       ...
>>>   }
>>>
>>>
>>> - hacky, but seems not more hacky than python detection, and should work 
>>> better
>>
>> As simple, forceful and unsubtle as a sledgehammer.  I like it :)
>>
>
>
> Hmm, not so simple.
>
> It leads to reindenting of function header, which is bad.

Because ___errp_coccinelle_updating___ is longer than errp, I guess.
Try ____?

> Possible solution is instead
>
> fn(...)
> {
> +   ___errp_coccinelle_updating___();
>
>
> but this slow down coccinelle. For example, on block.c from ~3s to 1m16s.
>
> .
>
> So, I'm returning to just a warning.
>
> I think something simple like
>
> @@
> identifier rule1.fn;
> position p != rule1.p;
> @@
>
> fn@p(...) {...}
>
> @ script:python@
>
> <print warning>
>
> should work.

Up to you.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel