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

[Stanislav Laznicka]

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

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.
>>>
>>> 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.
>>>
>>> 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-stlaz-0022-Cosmetic-changes-to-the-code.patch
Type: text/x-patch
Size: 4137 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160202/fd20aa50/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-stlaz-0023-Fixes-minor-issues.patch
Type: text/x-patch
Size: 3747 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160202/fd20aa50/attachment-0001.bin>