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

2016-02-24 Thread 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

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code


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

2016-02-23 Thread 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.




From 7e062d48fca49374eac27890c3cb6489764540b1 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka 
Date: Tue, 2 Feb 2016 12:50:26 +0100
Subject: [PATCH 1/2] Cosmetic changes to the code

Fixes some Coverity issues

ipadiscovery.py:  added assert should universe break
plugins/dns.py:   removed dead code
dnssec/ldapkeydb.py:  attribute assert in the proper object
test_automount_plugin.py: fixed possible close() on None
xmlrpc_test.py:   Coverity does not like accessing None.__class__

https://fedorahosted.org/freeipa/ticket/5661
---
 ipaclient/ipadiscovery.py | 2 +-
 ipalib/plugins/dns.py | 3 ---
 ipapython/dnssec/ldapkeydb.py | 2 +-
 ipatests/test_xmlrpc/test_automount_plugin.py | 2 ++
 ipatests/test_xmlrpc/xmlrpc_test.py   | 2 +-
 5 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/ipaclient/ipadiscovery.py b/ipaclient/ipadiscovery.py
index 45a71e190e56d33d51d37f16ae61a7b4c28df521..772add43a282838539b9b18dc60d3bba041bed1b 100644
--- a/ipaclient/ipadiscovery.py
+++ b/ipaclient/ipadiscovery.py
@@ -402,7 +402,7 @@ class IPADiscovery(object):
 return [0, thost, lrealms[0]]
 
 #we shouldn't get here
-

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

2016-02-21 Thread Jan Cholasta

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.


--
Jan Cholasta

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code


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

2016-02-02 Thread 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.
From f7a4ba996960aa53af432a64489a8caf6baa1ee7 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka 
Date: Tue, 2 Feb 2016 12:50:26 +0100
Subject: [PATCH 1/2] Cosmetic changes to the code

ipadiscovery.py:  added assert should universe break
plugins/dns.py:   removed dead code
dnssec/ldapkeydb.py:  attribute assert in the proper object
test_automount_plugin.py: fixed possible close() on None
xmlrpc_test.py:   Coverity does not like accessing None.__class__

https://fedorahosted.org/freeipa/ticket/5661
---
 ipaclient/ipadiscovery.py | 2 +-
 ipalib/plugins/dns.py | 3 ---
 ipapython/dnssec/ldapkeydb.py | 2 +-
 ipatests/test_xmlrpc/test_automount_plugin.py | 2 ++
 ipatests/test_xmlrpc/xmlrpc_test.py   | 2 +-
 5 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/ipaclient/ipadiscovery.py b/ipaclient/ipadiscovery.py
index 45a71e190e56d33d51d37f16ae61a7b4c28df521..772add43a282838539b9b18dc60d3bba041bed1b 100644
--- a/ipaclient/ipadiscovery.py
+++ b/ipaclient/ipadiscovery.py
@@ -402,7 +402,7 @@ class IPADiscovery(object):
 return [0, thost, lrealms[0]]
 
 #we shouldn't get here
-return [UNKNOWN_ERROR]
+assert False, "Unknown error in ipadiscovery"
 
 except errors.DatabaseTimeout:
 root_logger.debug("LDAP Error: timeout")
diff --git a/ipalib/plugins/dns.py b/ipalib/plugins/dns.py
index 0a9fa181347514f0cc11c3ab4a3e19864df4eec0..1892d29ec9f87d3b3f955ff8df6b154961fbce36 100644
--- a/ipalib/plugins/dns.py
+++