Merge lp:~mascha/sbackup/trunkFixFor632605 into lp:sbackup

[Martin Schaaf]

[Jean-Peer Lorenz (peer.loz)]

Thank you so much. I really appreciate your help.

Before merging your branch I've a few comments that need fixing though:

* don't import sys. Please use (more simple and intuitive):
...
except FileAccessError, error:
    log.error(_("message: %s" % error)

* use logger.error instead of logger.warning

* please re-raise an exception within the close_stream methods. Use the same exception in both backends. Purpose is to decide outside (i.e. when using close_stream) whether or not we want to ignore errors, how to handle them etc. This could be done by translating *some* caught errors into well defined one. Maybe it's worth to declare a new custom exception. Just have a look into 'utils.exception' at the FileAccessError section and propose one. E.g. in fuse_utils:

...
except (IOError, OSError), error:
    raise FileAccessError(_("Error while closing stream: %s") % error)

Similar for gio_utils (gio.Error and glib.Error). Just have a look at similar methods.

* please don't catch *all* exceptions. This is bad style. Catch only exceptions meaningful in the specific context. That is IOError and OSError in fuse backend and gio.Error and glib.Error in gio backend.

* catch according exception in module tar (line ~820) and log a message. I really don't like the idea of catching errors on stream closing in general.

* be sure your code complys to Python conventions: spaces instead of tabs, variable names at least 3 characters long, no camel case (I know, there are lots of not compliant LOC though I try to improve code quality whenever possible.) Use pylint if you want.

review: Needs Fixing

[Martin Schaaf (mascha)]

Hej Jean-Peer,

the patch should now follow your advices. Please have a look.

Bye,
martin.

review: Needs Resubmitting

[Martin Schaaf (mascha)]

review: Abstain

[Jean-Peer Lorenz (peer.loz)]

Hi Martin,
the patch looks great. Before I'll merge it into trunk I'd suggest a few changes:

1. make the warning in tar.py more verbose/descriptive e.g.
"Error (was ignored) while closing snapshot file: %s"
2. make this string translatable
3. revert the debug.error back into debug.warning (was my bad, of course it's a warning rather an error if we ignore the underlying error)
4. rename the exception FileAlreadyClosedException into FileAlreadyClosedError in order to follow Python's convention
4. in _fuse_utils: also throw a FileAlreadyClosedError and make the error message the same as in _gio_utils:
_("Error while closing stream: %s") % error (the method name isn't required since it has no value for users and is contained in the traceback)
5. make the error message here: raise FileAccessException(get_gio_errmsg(error, "Error in 'close_stream'"))
also look the same (get_gio_errmsg(error, _("Error while closing stream"))
6. add 'close_stream' to the interface class IOperations in util.interfaces

Thanks a lot.

review: Needs Fixing

[lp:~mascha/sbackup/trunkFixFor632605]

202. By Martin Schaaf on 2010-10-18

* do not import exceptions twice

203. By Martin Schaaf on 2010-10-18

* prefix errors with module name

[Jean-Peer Lorenz (peer.loz)]

Thanks for your effort and contribution.

review: Approve

=== modified file 'src/sbackup/ar_backend/tar.py'
--- src/sbackup/ar_backend/tar.py	2010-10-03 11:47:42 +0000
+++ src/sbackup/ar_backend/tar.py	2010-10-18 20:06:06 +0000
@@ -35,7 +35,6 @@
 import tempfile
 import shutil
 import re
-import sys
 
 from datetime import datetime
 
@@ -46,7 +45,6 @@
 
 from sbackup.util.log import LogFactory
 from sbackup.util.structs import SBdict
-from sbackup.util.exceptions import SBException
 from sbackup.util import exceptions
 from sbackup.util import constants
 from sbackup.util import system
@@ -1050,9 +1048,9 @@
                 n += 1
             header += c
         try:
-            fd.close()
-        except:
-            LogFactory.getLogger().warn("error on closing the snarfile header after reading: " + str(sys.exc_info()[1]))
+            _FOP.close_stream(fd)
+        except exceptions.FileAlreadyClosedError, error:
+            log.LogFactory.getLogger().warn(_("File was already closed (ignored): %s") % error)
 
         return header
 

=== modified file 'src/sbackup/fs_backend/_fuse_utils.py'
--- src/sbackup/fs_backend/_fuse_utils.py	2010-08-12 23:53:28 +0000
+++ src/sbackup/fs_backend/_fuse_utils.py	2010-10-18 20:06:06 +0000
@@ -33,7 +33,7 @@
 from sbackup.util import interfaces
 from sbackup.util import structs
 from sbackup.util import pathparse
-
+from sbackup.util import exceptions
 
 #TODO: move available services into FUSE plugin manager and retrieve them dynamically
 REMOTE_SERVICE_SFTP = 101
@@ -162,6 +162,12 @@
     def is_dir(cls, path):
         return local_file_utils.is_dir(path)
 
+    @classmethod
+    def close_stream(cls, file_desc):
+        try:
+            file_desc.close()
+        except IOError, error:
+            raise exceptions.FileAlreadyClosedError(_("Error while closing stream: %s") % error)
 
 def get_scheme_from_service(service):
     if not isinstance(service, types.IntType):

=== modified file 'src/sbackup/fs_backend/_gio_utils.py'
--- src/sbackup/fs_backend/_gio_utils.py	2010-10-03 15:02:59 +0000
+++ src/sbackup/fs_backend/_gio_utils.py	2010-10-18 20:06:06 +0000
@@ -45,7 +45,6 @@
 from sbackup.util import system
 from sbackup.util import log
 
-
 errorcodes = {
     gio.ERROR_ALREADY_MOUNTED : exceptions.ErrorDescription(gio.ERROR_ALREADY_MOUNTED,
                                                             "ERROR_ALREADY_MOUNTED",
@@ -815,6 +814,15 @@
         _ostr.write(content)
         _ostr.close()
 
+    @classmethod
+    def close_stream(cls, file_desc):
+        try:
+            file_desc.close()
+        except gio.Error, error:
+            if error.code == gio.ERROR_CLOSED:
+                raise exceptions.FileAlreadyClosedException(_("Error while closing stream: %s") % error)
+            else:
+                raise exceptions.FileAccessException(get_gio_errmsg(error, _("Error while closing stream")))
 
 def get_gio_errmsg(error, title):
     _error_descr = errorcodes[error.code]

=== modified file 'src/sbackup/util/exceptions.py'
--- src/sbackup/util/exceptions.py	2010-08-14 00:06:35 +0000
+++ src/sbackup/util/exceptions.py	2010-10-18 20:06:06 +0000
@@ -163,3 +163,8 @@
     """This Exception is thrown when remote path is not mountable.
 
     """
+
+class FileAlreadyClosedError(SBException) :
+    """This Exception is thrown when a file or stream cannot be closed because it is already closed.
+
+    """

=== modified file 'src/sbackup/util/interfaces.py'
--- src/sbackup/util/interfaces.py	2010-10-03 11:47:42 +0000
+++ src/sbackup/util/interfaces.py	2010-10-18 20:06:06 +0000
@@ -181,3 +181,7 @@
     @classmethod
     def is_dir(cls, path):
         raise NotImplementedError(_get_notimplemented_msg("IOperations", "is_dir"))
+
+    @classmethod
+    def close_stream(cls, file_desc):
+        raise NotImplementedError(_get_notimplemented_msg("IOperations", "close_stream"))