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

[Jan Cholasta]

On 24.2.2016 08:46, Stanislav Laznicka wrote:
> Reworded the commit messages so that they mention Coverity.
>
> On 02/22/2016 07:18 AM, Jan Cholasta wrote:
>> 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.
>>

Thanks, ACK.

Pushed to:
master: d7efd8a33ab14a561d3af445e62bceb6f2f13fd1
ipa-4-3: d78a759569020eba08b90e35707e86778523ec58

-- 
Jan Cholasta