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

[Petr Spacek]

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.


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

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