[Freeipa-devel] [PATCH 0013-0021] Coverity patches

[Jan Cholasta]

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.

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

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

>>
>>
>> Patch 0016: The original code is in fact correct.
>>
>>
>> Patch 0017: This will break Python 3. The two branches are performing the same
>> action, but with different data types.
>>
>>
>> Patch 0018: LGTM
>>
>>
>> Patch 0019: IMO the original code is better (None has a __class__ too, you know).
>>
>>
>> Patch 0020: LGTM
>>
>>
>> Patch 0021: Please use the original error messages (there are no requests
>> being added to D-Bus, but to certmonger).
>>
>>
>> Honza
>


-- 
Jan Cholasta