Re: [Freeipa-devel] [PATCH] 0031 ipa-restore: Check if directory is provided + better errors.
On 11/21/2014 02:28 PM, David Kupka wrote: On 11/21/2014 02:12 PM, Tomas Babej wrote: On 11/21/2014 01:56 PM, David Kupka wrote: [...] 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.[...] Python does take care of it eventually, but relying on this is messy – a bit like relying on the OS to close files when the process exits. CPython will currently close the file as soon as there are no more references to the object, but different garbage collectors might take much longer. Python 3 will issue warnings when opened files are left around. -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 0031 ipa-restore: Check if directory is provided + better errors.
https://fedorahosted.org/freeipa/ticket/4683
--
David Kupka
From f5f5ae55999b2057549306331b07a3c41c0cabeb 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 | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/ipaserver/install/ipa_restore.py b/ipaserver/install/ipa_restore.py
index 93f176d302a49319940555a0be3037620143e1f3..c634588b5bc5a87896244166a9f8f7a571b5911f 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:
+raise admintool.ScriptError('Cannot read backup metadata')
# 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
--
2.1.0
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0031 ipa-restore: Check if directory is provided + better errors.
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
--
Tomas Babej
Associate Software Engineer | Red Hat | Identity Management
RHCE | Brno Site | IRC: tbabej | freeipa.org
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0031 ipa-restore: Check if directory is provided + better errors.
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.
--
Tomas Babej
Associate Software Engineer | Red Hat | Identity Management
RHCE | Brno Site | IRC: tbabej | freeipa.org
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0031 ipa-restore: Check if directory is provided + better errors.
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
Re: [Freeipa-devel] [PATCH] 0031 ipa-restore: Check if directory is provided + better errors.
On 11/21/2014 02:28 PM, David Kupka wrote:
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.
Works quite nicely.
Thanks, ACK.
Pushed to:
master: 373bbee4e3c25fd6fb41a75b62b09d60da1a5d82
ipa-4-1: b40cf4b283c9df7d960ec80124b45d5361c75320
--
Tomas Babej
Associate Software Engineer | Red Hat | Identity Management
RHCE | Brno Site | IRC: tbabej | freeipa.org
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
