Re: GPU-DRM-TILCDC: Less function calls in tilcdc_convert_slave_node() after error detection

[Rob Clark]

On Thu, Sep 22, 2016 at 2:38 PM, SF Markus Elfring
<elfring@xxxxxxxxxxxxxxxxxxxxx> wrote:
>>> The of_node_put() function was called in some cases
>>> by the tilcdc_convert_slave_node() function during error handling
>>> even if the passed variable contained a null pointer.
>>>
>>> * Adjust jump targets according to the Linux coding style convention.
>>>
>>> * Split a condition check for resource detection failures so that
>>>   each pointer from these function calls will be checked immediately.
>>>
>>>   See also background information:
>>>   Topic "CWE-754: Improper check for unusual or exceptional conditions"
>>>   Link: https://cwe.mitre.org/data/definitions/754.html
>>>
>>
>> I don't really agree with this patch.
>
> This kind of feedback can be fine at first glance.
>
>
>> There is no harm in calling of_node_put() with NULL as an argument
>
> The cost of additional function calls will be eventually not noticed
> just because they belong to an exception handling implementation so far.

iirc, there are Coccinelle rules that find code with unnecessary null
checks and removes them..

Although you probably made this complex enough that cocinelle would
not find it.  That is not a complement.  One should not make error
handling/cleanup more complex than needed.

BR,
-R

>
>> and because of that there is no point in making the function more complex
>
> There is inherent software complexity involved.
>
>
>> and harder to maintain.
>
> How do you think about to discuss this aspect a bit more?
>
>
> I suggest to reconsider this design detail if it is really acceptable
> for the safe implementation of such a software module.
>
> * How much will it matter in general that one function call was performed
>   in this use case without checking its return value immediately?
>
> * Should it usually be determined quicker if a required resource
>   could be acquired before trying the next allocation?
>
> Regards,
> Markus
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel