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 <slazn...@redhat.com>
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
+++ 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/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
diff --git a/ipatests/test_xmlrpc/test_automount_plugin.py b/ipatests/test_xmlrpc/test_automount_plugin.py
index 974a0f1f7ff227cfb97b877f1e889b454b47dc38..6694bf07cbedb6c5ece1ec2386398476e6fa53dd 100644
--- a/ipatests/test_xmlrpc/test_automount_plugin.py
+++ b/ipatests/test_xmlrpc/test_automount_plugin.py
@@ -87,6 +87,8 @@ class AutomountTest(XMLRPC_test):
                 break
             else:
                 current_file.write(line + '\n')
+        assert current_file is not None, ('The input file does not contain any'
+                                          'records of files to be opened.')
         current_file.close()
 
         self.failsafe_add(api.Object.automountlocation, self.locname)
diff --git a/ipatests/test_xmlrpc/xmlrpc_test.py b/ipatests/test_xmlrpc/xmlrpc_test.py
index 8140364a53c20513fa6265997b8cd48e6140e62b..687958d690fa0833a50d80e4e82571f4900c1682 100644
--- a/ipatests/test_xmlrpc/xmlrpc_test.py
+++ b/ipatests/test_xmlrpc/xmlrpc_test.py
@@ -357,7 +357,7 @@ class Declarative(XMLRPC_test):
         if not expected(e, output):
             raise AssertionError(
                 UNEXPECTED % (cmd, name, args, options,
-                              e.__class__.__name__, e)
+                              type(e).__name__, e)
             )
 
     def check_output(self, nice, cmd, args, options, expected, extra_check):
-- 
2.5.0

From 42481e5203b7d11c963ebafc9e8c21c5e0f31dd6 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Tue, 2 Feb 2016 12:57:11 +0100
Subject: [PATCH 2/2] Fixes minor issues

plugins/baseldap.py:      possible undefined value in return
certmonger.py:            possible dereference of None value
i18n.py:                  fixed always True bug (+ cosmetic change)

https://fedorahosted.org/freeipa/ticket/5661
---
 ipalib/plugins/baseldap.py |  2 +-
 ipapython/certmonger.py    | 14 ++++++++++----
 ipatests/i18n.py           |  4 ++--
 3 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/ipalib/plugins/baseldap.py b/ipalib/plugins/baseldap.py
index 03a255ca1303091f8a9f08f6ae1d0f76281937a0..ffc0008a65d3ea52a084870591b867648c8dea36 100644
--- a/ipalib/plugins/baseldap.py
+++ b/ipalib/plugins/baseldap.py
@@ -451,12 +451,12 @@ def remove_external_post_callback(ldap, dn, entry_attrs, failed, completed,
 
     # Run through the failures and gracefully remove any member defined
     # as an external member.
+    completed_external = 0
     if memberattr in failed and membertype in failed[memberattr]:
         entry_attrs_ = ldap.get_entry(dn, [externalattr])
         dn = entry_attrs_.dn
         external_entries = entry_attrs_.get(externalattr, [])
         failed_entries = []
-        completed_external = 0
 
         for entry in failed[memberattr][membertype]:
             membername = entry[0].lower()
diff --git a/ipapython/certmonger.py b/ipapython/certmonger.py
index f89ca0b7a1cbb9d34b0c044e30e213e7aa1c74fd..effb7bb15e88e4dbc1c9b26d1f220fe7e8153718 100644
--- a/ipapython/certmonger.py
+++ b/ipapython/certmonger.py
@@ -318,8 +318,11 @@ def request_cert(nssdb, nickname, subject, principal, passwd_fname=None):
         if result[0]:
             request = _cm_dbus_object(cm.bus, cm, result[1], DBUS_CM_REQUEST_IF,
                                       DBUS_CM_IF, True)
-    except TypeError:
-        root_logger.error('Failed to get create new request.')
+        else:
+            raise RuntimeError('add_request() returned False')
+    except Exception as e:
+        root_logger.error('Failed to create a new request: {error}'
+                          .format(error=e))
         raise
     return request.obj_if.get_nickname()
 
@@ -356,8 +359,11 @@ def start_tracking(nickname, secdir, password_file=None, command=None):
         if result[0]:
             request = _cm_dbus_object(cm.bus, cm, result[1], DBUS_CM_REQUEST_IF,
                                       DBUS_CM_IF, True)
-    except TypeError as e:
-        root_logger.error('Failed to add new request.')
+        else:
+            raise RuntimeError('add_request() returned False')
+    except Exception as e:
+        root_logger.error('Failed to add new request: {error}'
+                          .format(error=e))
         raise
     return request.prop_if.Get(DBUS_CM_REQUEST_IF, 'nickname')
 
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

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

Reply via email to