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

[Stanislav Laznicka]

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.
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-stlaz-0022-2-Cosmetic-changes-to-the-code.patch
Type: text/x-patch
Size: 4165 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160224/8ef95ca7/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-stlaz-0023-2-Fixes-minor-issues.patch
Type: text/x-patch
Size: 3784 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160224/8ef95ca7/attachment-0001.bin>