On 04/24/2015 03:19 PM, Martin Basti wrote:
On 20/04/15 10:58, Martin Babinsky wrote:
On 04/20/2015 10:32 AM, Martin Basti wrote:
On 17/04/15 14:11, Martin Babinsky wrote:
On 04/17/2015 12:41 PM, Martin Babinsky wrote:
On 04/17/2015 12:36 PM, Martin Basti wrote:
On 17/04/15 12:33, Martin Babinsky wrote:
On 04/17/2015 12:04 PM, Martin Basti wrote:
On 15/04/15 15:53, Martin Babinsky wrote:
On 04/14/2015 04:24 PM, Martin Basti wrote:
On 14/04/15 16:12, Martin Basti wrote:
On 14/04/15 14:25, Martin Babinsky wrote:
This patch addresses
https://fedorahosted.org/freeipa/ticket/4966

The noise during rollback/uninstall is caused mainly by
unsuccessful
attempts to remove files that do not exist anymore. These
errors
are
now logged at debug level and do not pop-up to stdout/stderr.



Hello, thank you for the patch.

1)
The option add_warning is quite unclear to me. It does not show
warning but error. I suggest something like, show_hint,
show_user_action, or something show_additional_..., or
promt_manual_removal

Martin^2


Continue...

2)

             if file_exists(preferences_fname):
                 try:
                     os.remove(preferences_fname)
                 except OSError as e:
                     log_file_removal_error(e, preferences_fname,
True)

In this case file not found error should never happen.

Could you remove the 'if file_exists' part and handle just
exception?

I just reverted this bit to original form in order to not fix
something that isn't broken. Is that ok?
3)
this is inconsistent with change above, choose one style please:

             if os.path.exists(ca_file):
                 try:
                     os.unlink(ca_file)
                 except OSError, e:
                     root_logger.error(
                         "Failed to remove '%s': %s", ca_file, e)

--
Martin Basti


Attaching updated patch.

thanks,

just one nitpick, can you move the new function into
installutils, it
can be used in different scripts not just in ipaclient.


I'm not sure if it is a good idea as installutils is a part for
freeipa-server package.

Placing it there would create an unnecessary dependency of
freeipa-client on freeipa-server because of a single function.

you are right, I do not why I thought that ipa-client-install uses
installutils.

ACK

self-NACK, I will try to rewrite the patch in a slightly less dumb
way.

Sorry for the confusion.


Attaching updated patch which does the same but using a wrapper around
os.remove().

Jan suggested to keep the new function in 'ipa-client-install' and
move it around when we do installer re#$%@^ing.

Is that ok?

It looks better, ACK.

Jan NACKed your ACK.

Attaching updated patch.

Sorry, NACK

************* Module ipa-client-install
ipa-client/ipa-install/ipa-client-install:791:
[E1121(too-many-function-args), uninstall] Too many positional arguments
for function call)
ipa-client/ipa-install/ipa-client-install:797:
[E1121(too-many-function-args), uninstall] Too many positional arguments
for function call)

consult with Honza if option which show prompt user to delete file
manually, should be there or not.


Updated patch attached.

--
Martin^3 Babinsky
From c0fa8fa0896617ba4a4b78dab283fa9ca583d3f2 Mon Sep 17 00:00:00 2001
From: Martin Babinsky <mbabi...@redhat.com>
Date: Tue, 14 Apr 2015 13:55:33 +0200
Subject: [PATCH] suppress errors arising from deleting non-existent files
 during client uninstall

When rolling back partially configured IPA client a number of OSErrors pop up
due to uninstaller trying to remove files that do not exist anymore. This
patch supresses these errors while keeping them in log as debug messages.

https://fedorahosted.org/freeipa/ticket/4966
---
 ipa-client/ipa-install/ipa-client-install | 40 +++++++++++++++++--------------
 1 file changed, 22 insertions(+), 18 deletions(-)

diff --git a/ipa-client/ipa-install/ipa-client-install b/ipa-client/ipa-install/ipa-client-install
index bb59f47249f27a409683f825f204a501a90a0e7a..b87dd791eff599110e43f3c563e9d0be0c68138e 100755
--- a/ipa-client/ipa-install/ipa-client-install
+++ b/ipa-client/ipa-install/ipa-client-install
@@ -238,6 +238,25 @@ def logging_setup(options):
         console_format='%(message)s')
 
 
+def remove_file(filename):
+    """
+    Deletes a file. If the file does not exist (OSError 2) does nothing.
+    Otherwise logs an error message and instructs the user to remove the
+    offending file manually
+    :param filename: name of the file to be removed
+    """
+
+    try:
+        os.remove(filename)
+    except OSError as e:
+        if e.errno == 2:
+            return
+
+        root_logger.error("Failed to remove file %s: %s", filename, e)
+        root_logger.error('Please remove %s manually, as it can cause '
+                          'subsequent installation to fail.', filename)
+
+
 def log_service_error(name, action, error):
     root_logger.error("%s failed to %s: %s", name, action, str(error))
 
@@ -543,10 +562,7 @@ def uninstall(options, env):
                      os.path.join(ipa_db.secdir, 'key3.db'),
                      os.path.join(ipa_db.secdir, 'secmod.db'),
                      os.path.join(ipa_db.secdir, 'pwdfile.txt')):
-        try:
-            os.remove(filename)
-        except OSError, e:
-            root_logger.error("Failed to remove %s: %s", filename, e)
+        remove_file(filename)
 
     for nickname, trust_flags in ipa_certs:
         while sys_db.has_nickname(nickname):
@@ -774,25 +790,13 @@ def uninstall(options, env):
             'to its pre-installation state.')
 
     # Remove the IPA configuration file
-    try:
-        os.remove(paths.IPA_DEFAULT_CONF)
-    except OSError, e:
-        root_logger.warning('/etc/ipa/default.conf could not be removed: %s',
-                            str(e))
-        root_logger.warning('Please remove /etc/ipa/default.conf manually, '
-                            'as it can cause subsequent installation to fail.')
+    remove_file(paths.IPA_DEFAULT_CONF)
 
     # Remove the CA cert from the systemwide certificate store
     tasks.remove_ca_certs_from_systemwide_ca_store()
 
     # Remove the CA cert
-    try:
-        os.remove(CACERT)
-    except OSError, e:
-        root_logger.warning('%s could not be removed: %s', CACERT, str(e))
-        root_logger.warning('Please remove %s manually, '
-                            'as it can cause subsequent '
-                            'installation to fail.', CACERT)
+    remove_file(CACERT)
 
     root_logger.info("Client uninstall complete.")
 
-- 
2.1.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