[Freeipa-devel] [PATCH 0022-23] Coverity patches

[Jan Cholasta]

On 2.2.2016 13:36, Stanislav Laznicka wrote:
> On 02/01/2016 02:24 PM, Jan Cholasta wrote:
>> On 1.2.2016 12:11, Petr Spacek wrote:
>>> On 1.2.2016 09:03, Jan Cholasta wrote:
>>>> Hi,
>>>>
>>>> On 29.1.2016 15:49, Martin Basti wrote:
>>>>>
>>>>>
>>>>> On 29.01.2016 15:49, Stanislav Laznicka wrote:
>>>>>> Reworded the commits so that they better reflect what's going on
>>>>>> in those.
>>>>>>
>>>>>> On 01/29/2016 02:49 PM, Stanislav Laznicka wrote:
>>>>>>> Hello,
>>>>>>>
>>>>>>> I made some patches based on the Coverity report from 18.1.2016.
>>>>>>>
>>>>>>> Cheers,
>>>>>>> Standa
>>>>>>>
>>>>>>
>>>>> NACK, see my previous email
>>>>
>>>> I don't think this deserves 9 patches, 1 would be sufficient enough.
>>>
>>> I would rather have it is separate patches as these fixes are largely
>>> not
>>> related. It will make bisecting easier.
>>
>> Most of the fixes are cosmetic, which makes them related, and the rest
>> are small isolated changes, so in reality it would hardly make
>> bisecting easier and only increase the overhead. In the past we
>> usually had put such fixes into a single patch and AFAIK nobody
>> complained so far.
>>
> Squeezed the changes into two new patches, then. One for the very
> cosmetic changes, one for possible bugs.

OK.

>>>
>>>
>>>> Patch 0013:
>>>>
>>>> 1) I think this unreachable return is intentional, as indicated by
>>>> the comment:
>>>>
>>>> -            #we shouldn't get here
>>>> -            return [UNKNOWN_ERROR]
>>>
>>> I would use
>>> assert False, "we shouldn't get here"
>>> neither we nor Coverity are confused when we hit the code path one day.
>>>
>>> UNKNOWN_ERROR would pop up somewhere else and it will be harder to
>>> find out
>>> why the hell the code behaves as mad. Traceback will clearly indicate
>>> that
>>> there is a problem with the 'switch'.
>>
>> Sure, my point is that returning None is no better than returning
>> UNKNOWN_ERROR.
>>
> Added assert as suggested. There should still be no way of getting to
> it, though.
>>>
>>> Petr^2 Spacek
>>>
>>>> 2) How is this dead code?
>>>>
>>>> -    if options.mode == 'validate_pot' or options.mode ==
>>>> 'validate_po':
>>>> +    if options.mode in ('validate_pot', 'validate_po'):
>>>>
>>>> -    elif options.mode == 'create_test' or 'test_gettext':
>>>> +    elif options.mode in ('create_test', 'test_gettext'):
>>>>
>>>>
>>>>
>>>> Patch 0014-0015: LGTM
>>
>> Actually scratch that, patch 0015 breaks correct code.
>>
> The dead code appears in the 'else' branch as the latter of these two
> conditions always evaluates to True. The first condition change is just
> a cosmetic one so that both of the conditions look the same.

OK.

>
> Also removed the changes made in patch 0015.
>>>>
>>>>
>>>> Patch 0016: The original code is in fact correct.
>>>>
> Point taken, removed the change.
>>>>
>>>> Patch 0017: This will break Python 3. The two branches are
>>>> performing the same
>>>> action, but with different data types.
>>>>
> This might undergo further investigation in the future as there is no
> way how "bytes" instance could become an argument of this function (as
> suggested). Not even the newest Python 3 patches from pviktori mention
> this code.

OK. (This is not what Coverity was complaining about, though.)

>>>>
>>>> Patch 0018: LGTM
>>>>
>>>>
>>>> Patch 0019: IMO the original code is better (None has a __class__
>>>> too, you know).
>>>>
> Made it more "Coverity friendly" yet nice enough modification.
>>>>
>>>> Patch 0020: LGTM
>>>>
> It seems that there actually is a check that checks whether the input is
> correct. It is called ad-hoc but that might be the test feature.
> Therefore just added an assert so that Coverity does not complain.

OK.

>>>>
>>>> Patch 0021: Please use the original error messages (there are no
>>>> requests
>>>> being added to D-Bus, but to certmonger).
>>>>
>>>>
>>>> Honza
>>>
>>
> Added error messages that reflect the situation better, then.

Could you please mention Coverity in the commit messages, so that it's 
clear why are we doing these changes? Otherwise LGTM.

-- 
Jan Cholasta