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
