Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix unassigned variable exception in base #946

Conversation

dpindur
Copy link
Contributor

@dpindur dpindur commented Oct 7, 2017

If install_pkgs is empty pkg would never be assigned to and cause an exception when it is passed to _verification_of_packages. A DNF exception would then be raised indicating that some packages have an incorrect checksum.

I noticed this while using the DNF Ansible module. The bug prevented the module from removing packages.

Let me know if you need more details, or can suggest a nicer place to implement the fix.

dnf/base.py Outdated
@@ -2289,7 +2290,7 @@ def _verification_of_packages(pkg_list, logger_msg):

try:
msg = _('Package "{}" from local repository "{}" has incorrect checksum')
if not _verification_of_packages(local_repository_pkgs, msg.format(pkg, pkg.reponame)):
if pkg is not None and not _verification_of_packages(local_repository_pkgs, msg.format(pkg, pkg.reponame)):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[flake8]

  • [E501] line too long (119 > 100 characters)

@ignatenkobrain
Copy link
Contributor

indeed looks good to me!

Could you please wrap long line? After you fix this, I will merge PR.

@ignatenkobrain ignatenkobrain self-assigned this Oct 7, 2017
@dpindur
Copy link
Contributor Author

dpindur commented Oct 8, 2017

Line wrapped and I think we're all good.

@ignatenkobrain
Copy link
Contributor

@rh-atomic-bot r+

@rh-atomic-bot
Copy link

📌 Commit 404503b has been approved by ignatenkobrain

@rh-atomic-bot
Copy link

⌛ Testing commit 404503b with merge f82ce2e...

rh-atomic-bot pushed a commit that referenced this pull request Oct 9, 2017
rh-atomic-bot pushed a commit that referenced this pull request Oct 9, 2017
Closes: #946
Approved by: ignatenkobrain
rh-atomic-bot pushed a commit that referenced this pull request Oct 9, 2017
Closes: #946
Approved by: ignatenkobrain
@rh-atomic-bot
Copy link

💥 Test timed out

@j-mracek
Copy link
Member

@rh-atomic-bot r-

@j-mracek
Copy link
Member

This is incorrect fix.

Copy link
Member

@j-mracek j-mracek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please the original code was little bit more problematic, that is visible on the first look, therefore it has to be little bit more improved.

@@ -2289,7 +2290,8 @@ def _verification_of_packages(pkg_list, logger_msg):

try:
msg = _('Package "{}" from local repository "{}" has incorrect checksum')
if not _verification_of_packages(local_repository_pkgs, msg.format(pkg, pkg.reponame)):
if pkg is not None and not _verification_of_packages(local_repository_pkgs,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct line would be:

if not _verification_of_packages(local_repository_pkgs, msg):

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I try to little bit improve the code but I didn't test it. Please can you take a look and if you will like it, just use it.

diff --git a/dnf/base.py b/dnf/base.py
index 970f3003..515e2b9c 100644
--- a/dnf/base.py
+++ b/dnf/base.py
@@ -2285,11 +2285,18 @@ class Base(object):
         :return: list of remote pkgs
         """
         def _verification_of_packages(pkg_list, logger_msg):
+            all_packages_verified = True
             for pkg in pkg_list:
-                if not pkg.verifyLocalPkg():
-                    logger.critical(logger_msg)
-                    return False
-            return True
+                pkg_successfully_verified = False
+                try:
+                    pkg_successfully_verified = pkg.verifyLocalPkg()
+                except Exception as e:
+                    logger.critical(str(e))
+                if not pkg_successfully_verified:
+                    logger.critical(logger_msg.format(pkg, pkg.reponame))
+                    all_packages_verified = False
+
+            return all_packages_verified
 
         remote_pkgs = []
         local_repository_pkgs = []
@@ -2299,17 +2306,9 @@ class Base(object):
                     local_repository_pkgs.append(pkg)
             else:
                 remote_pkgs.append(pkg)
-        error = False
-
-        try:
-            msg = _('Package "{}" from local repository "{}" has incorrect checksum')
-            if not _verification_of_packages(local_repository_pkgs, msg.format(pkg, pkg.reponame)):
-                error = True
-        except Exception as e:
-            logger.critical(str(e))
-            error = True
 
-        if error:
+        msg = _('Package "{}" from local repository "{}" has incorrect checksum')
+        if not _verification_of_packages(local_repository_pkgs, msg):
             raise dnf.exceptions.Error(
                 _("Some packages from local repository have incorrect checksum"))

@j-mracek j-mracek self-assigned this Oct 13, 2017
@ignatenkobrain ignatenkobrain removed their assignment Oct 16, 2017
@j-mracek
Copy link
Member

@dpindur I added my requested changes in additional commit to yours and create a new pull-request #956. Unfortunately it looks like that others hit the same issue, therefore I just want to speed up the review. Thank you very much for your contribution.

I am closing PR.

@j-mracek j-mracek closed this Oct 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants