On 11/21/2014 02:12 PM, Tomas Babej wrote:


On 11/21/2014 01:56 PM, David Kupka wrote:
On 11/21/2014 01:42 PM, Tomas Babej wrote:

On 11/21/2014 01:33 PM, David Kupka wrote:
https://fedorahosted.org/freeipa/ticket/4683


_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

-            self.read_header()
+            try:
+                self.read_header()
+            except:
+                raise admintool.ScriptError('Cannot read backup
metadata')


Is the bare except clause really necessary?

https://docs.python.org/2.7/howto/doanddont.html#except


Not really. I can't imagine other reaction to any exception raised in
read_header() than complain and exit.
But you're right that it can hide code errors and make debugging more
complicated.
Fixed patch attached.

On another note, I also noticed that read_header leaves leaking file
descriptor fd.

Can you convert that part to use the with statement? This is a perfect
opportunity to fix this as you're touching related code.

I thought that python takes care of it. Thanks.
Fixed patch attached.

--
David Kupka
From 026a4eecd2c516d0ae1d579337432c43e189bd7d Mon Sep 17 00:00:00 2001
From: David Kupka <dku...@redhat.com>
Date: Fri, 21 Nov 2014 06:30:17 -0500
Subject: [PATCH] ipa-restore: Check if directory is provided + better errors.

https://fedorahosted.org/freeipa/ticket/4683
---
 ipaserver/install/ipa_restore.py | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/ipaserver/install/ipa_restore.py b/ipaserver/install/ipa_restore.py
index 93f176d302a49319940555a0be3037620143e1f3..f290bae4dc6455bb22c4e726e72efe98205d970e 100644
--- a/ipaserver/install/ipa_restore.py
+++ b/ipaserver/install/ipa_restore.py
@@ -152,6 +152,9 @@ class Restore(admintool.AdminTool):
         else:
             self.backup_dir = dirname
 
+        if not os.path.isdir(dirname):
+            raise self.option_parser.error("must provide path to backup directory")
+
         if options.gpg_keyring:
             if (not os.path.exists(options.gpg_keyring + '.pub') or
                not os.path.exists(options.gpg_keyring + '.sec')):
@@ -213,7 +216,10 @@ class Restore(admintool.AdminTool):
         try:
             dirsrv = services.knownservices.dirsrv
 
-            self.read_header()
+            try:
+                self.read_header()
+            except IOError as e:
+                raise admintool.ScriptError('Cannot read backup metadata: %s' % e)
             # These two checks would normally be in the validate method but
             # we need to know the type of backup we're dealing with.
             if (self.backup_type != 'FULL' and not options.data_only and
@@ -546,9 +552,9 @@ class Restore(admintool.AdminTool):
         Read the backup file header that contains the meta data about
         this particular backup.
         '''
-        fd = open(self.header)
-        config = SafeConfigParser()
-        config.readfp(fd)
+        with open(self.header) as fd:
+            config = SafeConfigParser()
+            config.readfp(fd)
 
         self.backup_type = config.get('ipa', 'type')
         self.backup_time = config.get('ipa', 'time')
-- 
2.1.0

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to