Re: [Freeipa-devel] [PATCH 0013-0021] Coverity patches
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
--
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 0013-0021] Coverity patches
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
--
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 0013-0021] Coverity patches
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.
Patch 0013:
1) I think this unreachable return is intentional, as indicated by the
comment:
-#we shouldn't get here
-return [UNKNOWN_ERROR]
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
--
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 0013-0021] Coverity patches
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 -- 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 0013-0021] Coverity patches
On 29.01.2016 14:49, Stanislav Laznicka wrote:
Hello,
I made some patches based on the Coverity report from 18.1.2016.
Cheers,
Standa
NACK
*) please describe issue in commit message instead of #coverity number
in all patches
PATCH: Removing dead code
LGTM
PATCH: Wrong assert
I'm not sure with this, please ask pspacek
PATCH: Searching for an attribute in a wrong object
Please file a ticket for this, if this is right, we might want to
backport patch
PATCH: Class attribute 'type' should be used instead
LGTM
PATCH: Removed identical branch
NACK, bytes and string are not the same type, this may cause issues in
python3
PATCH: completed_external might not have beed defined
LGTM
PATCH: Accessing attribute of a possible None value
LGTM
PATCH: Possible close/write into None or closed file
it looks like assertion overkill to me, would be better to rewrite the
logic IMO
PATCH: Fixed possible dereferencing of undefined objects
NACK
1)
-root_logger.error('Failed to get create new request.')
+else:
+raise RuntimeError('Failed to add a request to DBUS.'
raise original error, do not reraise new exception, it will lost traceback
2)
I would put return to else: block, and raise RuntimeError in try block,
then just add except (TypeError, RuntimeError)
Try to avoid broad exceptions.
--
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 0013-0021] Coverity patches
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
From 56bfba733321388190cf6df0ec0dfab5fff15996 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka
Date: Fri, 29 Jan 2016 08:57:06 +0100
Subject: [PATCH 1/9] Removing dead code
---
ipaclient/ipadiscovery.py | 3 ---
ipalib/plugins/dns.py | 3 ---
ipatests/i18n.py | 4 ++--
3 files changed, 2 insertions(+), 8 deletions(-)
diff --git a/ipaclient/ipadiscovery.py b/ipaclient/ipadiscovery.py
index 45a71e190e56d33d51d37f16ae61a7b4c28df521..39f3fcc98d06d7041a82ca7f5e6c240fe9a23983 100644
--- a/ipaclient/ipadiscovery.py
+++ b/ipaclient/ipadiscovery.py
@@ -401,9 +401,6 @@ class IPADiscovery(object):
else:
return [0, thost, lrealms[0]]
-#we shouldn't get here
-return [UNKNOWN_ERROR]
-
except errors.DatabaseTimeout:
root_logger.debug("LDAP Error: timeout")
return [NO_LDAP_SERVER]
diff --git a/ipalib/plugins/dns.py b/ipalib/plugins/dns.py
index 0a9fa181347514f0cc11c3ab4a3e19864df4eec0..1892d29ec9f87d3b3f955ff8df6b154961fbce36 100644
--- a/ipalib/plugins/dns.py
+++ b/ipalib/plugins/dns.py
@@ -800,9 +800,6 @@ class DNSRecord(Str):
if value is None:
return
-if value is None:
-return
-
if not self.supported:
return _('DNS RR type "%s" is not supported by bind-dyndb-ldap plugin') \
% self.rrtype
diff --git a/ipatests/i18n.py b/ipatests/i18n.py
index a83c5806ef8ceaf011ec54db18d0222769bad1b1..368712599063c3281b95bf46e48dea924fc7999e 100755
--- a/ipatests/i18n.py
+++ b/ipatests/i18n.py
@@ -755,7 +755,7 @@ def main():
print('ERROR: no mode specified', file=sys.stderr)
return 1
-if options.mode == 'validate_pot' or options.mode == 'validate_po':
+if options.mode in ('validate_pot', 'validate_po'):
if options.mode == 'validate_pot':
files = args
if not files:
@@ -795,7 +795,7 @@ def main():
else:
return 0
-elif options.mode == 'create_test' or 'test_gettext':
+elif options.mode in ('create_test', 'test_gettext'):
po_file = '%s.po' % options.test_lang
pot_file = options.pot_file
--
2.5.0
From 131fdd331d2283a7fc406e7c03e9f8e67f99794d Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka
Date: Fri, 29 Jan 2016 09:01:43 +0100
Subject: [PATCH 2/9] Wrong assert in ldapkeydb
Asserting 'ipk11id' attribute in different object than from which it
is later accessed
---
ipapython/dnssec/ldapkeydb.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ipapython/dnssec/ldapkeydb.py b/ipapython/dnssec/ldapkeydb.py
index 55c09c040a1790569924f506f520b8327775ee7c..aa04139348a42910ed20db5189db70e262c1d5c1 100644
--- a/ipapython/dnssec/ldapkeydb.py
+++ b/ipapython/dnssec/ldapkeydb.py
@@ -288,7 +288,7 @@ class LdapKeyDB(AbstractHSM):
for attr in default_attrs:
key.setdefault(attr, default_attrs[attr])
-assert 'ipk11id' in o, 'key is missing ipk11Id in %s' % key.entry.dn
+assert 'ipk11id' in key, 'key is missing ipk11Id in %s' % key.entry.dn
key_id = key['ipk11id']
assert key_id not in keys, 'duplicate ipk11Id=0x%s in "%s" and "%s"' % (hexlify(key_id), key.entry.dn, keys[key_id].entry.dn)
assert 'ipk11label' in key, 'key "%s" is missing ipk11Label' % key.entry.dn
--
2.5.0
From 528af83bee872b5722cdfef7c47e011f037db274 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka
Date: Fri, 29 Jan 2016 09:03:11 +0100
Subject: [PATCH 3/9] Searching for an attribute in a wrong object
Checking for an attribute in a different object than it's
accessed later
---
ipalib/plugins/permission.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ipalib/plugins/permission.py b/ipalib/plugins/permission.py
index 2312008fa6db646dd4e3e9598daf7ea48c867337..9c64fdf5f6a867a4eab1ccfcdba663d5e1214f08 100644
--- a/ipalib/plugins/permission.py
+++ b/ipalib/plugins/permission.py
@@ -473,7 +473,7 @@ class permission(baseldap.LDAPObject):
if old_client:
for old_name, new_name in _DEPRECATED_OPTION_ALIASES.items():
-if new_name in entry:
+if new_name in rights:
rights[old_name] = rights[new_name]
del rights[new_name]
--
2.5.0
From 61826966e57d2b070ce3de25a9f6fc9cf2588ab5 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka
Date: Fri, 29 Jan 2016 09:08:19 +0100
Subject: [PATCH 4/9] Builtin 'type' used wrong in Param class
The builting 'type' is used instead of the same-named class member
---
ipalib/parameters.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ipalib/parameters.py b/ipalib/
[Freeipa-devel] [PATCH 0013-0021] Coverity patches
Hello,
I made some patches based on the Coverity report from 18.1.2016.
Cheers,
Standa
From 89a945cb78b324757636dbcaddabc2616d57bde2 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka
Date: Fri, 29 Jan 2016 08:57:06 +0100
Subject: [PATCH 1/9] Removing dead code
Coverity #13532
Coverity #13537
Coverity #13552
---
ipaclient/ipadiscovery.py | 3 ---
ipalib/plugins/dns.py | 3 ---
ipatests/i18n.py | 4 ++--
3 files changed, 2 insertions(+), 8 deletions(-)
diff --git a/ipaclient/ipadiscovery.py b/ipaclient/ipadiscovery.py
index 45a71e190e56d33d51d37f16ae61a7b4c28df521..39f3fcc98d06d7041a82ca7f5e6c240fe9a23983 100644
--- a/ipaclient/ipadiscovery.py
+++ b/ipaclient/ipadiscovery.py
@@ -401,9 +401,6 @@ class IPADiscovery(object):
else:
return [0, thost, lrealms[0]]
-#we shouldn't get here
-return [UNKNOWN_ERROR]
-
except errors.DatabaseTimeout:
root_logger.debug("LDAP Error: timeout")
return [NO_LDAP_SERVER]
diff --git a/ipalib/plugins/dns.py b/ipalib/plugins/dns.py
index 0a9fa181347514f0cc11c3ab4a3e19864df4eec0..1892d29ec9f87d3b3f955ff8df6b154961fbce36 100644
--- a/ipalib/plugins/dns.py
+++ b/ipalib/plugins/dns.py
@@ -800,9 +800,6 @@ class DNSRecord(Str):
if value is None:
return
-if value is None:
-return
-
if not self.supported:
return _('DNS RR type "%s" is not supported by bind-dyndb-ldap plugin') \
% self.rrtype
diff --git a/ipatests/i18n.py b/ipatests/i18n.py
index a83c5806ef8ceaf011ec54db18d0222769bad1b1..368712599063c3281b95bf46e48dea924fc7999e 100755
--- a/ipatests/i18n.py
+++ b/ipatests/i18n.py
@@ -755,7 +755,7 @@ def main():
print('ERROR: no mode specified', file=sys.stderr)
return 1
-if options.mode == 'validate_pot' or options.mode == 'validate_po':
+if options.mode in ('validate_pot', 'validate_po'):
if options.mode == 'validate_pot':
files = args
if not files:
@@ -795,7 +795,7 @@ def main():
else:
return 0
-elif options.mode == 'create_test' or 'test_gettext':
+elif options.mode in ('create_test', 'test_gettext'):
po_file = '%s.po' % options.test_lang
pot_file = options.pot_file
--
2.5.0
From cdcf6dd18a7edd75e4dd4940aaaf55e3c961e3c0 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka
Date: Fri, 29 Jan 2016 09:01:43 +0100
Subject: [PATCH 2/9] Wrong assert
Coverity #13529
---
ipapython/dnssec/ldapkeydb.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ipapython/dnssec/ldapkeydb.py b/ipapython/dnssec/ldapkeydb.py
index 55c09c040a1790569924f506f520b8327775ee7c..aa04139348a42910ed20db5189db70e262c1d5c1 100644
--- a/ipapython/dnssec/ldapkeydb.py
+++ b/ipapython/dnssec/ldapkeydb.py
@@ -288,7 +288,7 @@ class LdapKeyDB(AbstractHSM):
for attr in default_attrs:
key.setdefault(attr, default_attrs[attr])
-assert 'ipk11id' in o, 'key is missing ipk11Id in %s' % key.entry.dn
+assert 'ipk11id' in key, 'key is missing ipk11Id in %s' % key.entry.dn
key_id = key['ipk11id']
assert key_id not in keys, 'duplicate ipk11Id=0x%s in "%s" and "%s"' % (hexlify(key_id), key.entry.dn, keys[key_id].entry.dn)
assert 'ipk11label' in key, 'key "%s" is missing ipk11Label' % key.entry.dn
--
2.5.0
From 575fddf988cfce082c310973fa46d98a9476587c Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka
Date: Fri, 29 Jan 2016 09:03:11 +0100
Subject: [PATCH 3/9] Searching for an attribute in a wrong object
Coverity #13530
---
ipalib/plugins/permission.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ipalib/plugins/permission.py b/ipalib/plugins/permission.py
index 2312008fa6db646dd4e3e9598daf7ea48c867337..9c64fdf5f6a867a4eab1ccfcdba663d5e1214f08 100644
--- a/ipalib/plugins/permission.py
+++ b/ipalib/plugins/permission.py
@@ -473,7 +473,7 @@ class permission(baseldap.LDAPObject):
if old_client:
for old_name, new_name in _DEPRECATED_OPTION_ALIASES.items():
-if new_name in entry:
+if new_name in rights:
rights[old_name] = rights[new_name]
del rights[new_name]
--
2.5.0
From 996de9aeb91d3ee53a0036ef35358290a2be30da Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka
Date: Fri, 29 Jan 2016 09:08:19 +0100
Subject: [PATCH 4/9] Class attribute 'type' should be used instead
Coverity #13531
---
ipalib/parameters.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ipalib/parameters.py b/ipalib/parameters.py
index e46068c96564d9fad61cbc75b8fe4f8f40cca98b..8d174ebec28bc0e3b2762c5d8dc1d99c041c54c9 100644
--- a/ipalib/parameters.py
+++ b/ipalib/parameters.py
@@ -482,7 +482,7 @@ class Param(ReadOnly):
elif type(value) is str:
