Change in vdsm[master]: vdsm: storage/task.py replace/remove too generic except hand...

[Vinzenz Feenstra]

Vinzenz Feenstra has uploaded a new change for review.

Change subject: vdsm: storage/task.py replace/remove too generic except handlers
......................................................................

vdsm: storage/task.py replace/remove too generic except handlers

Change-Id: Ia09dcada40bca3f16f94ad7a6c83e6d7a85ade77
Signed-off-by: Vinzenz Feenstra 
---
M vdsm/storage/task.py
1 file changed, 2 insertions(+), 4 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/51/17751/1

diff --git a/vdsm/storage/task.py b/vdsm/storage/task.py
index 4eff5c1..5b8613f 100644
--- a/vdsm/storage/task.py
+++ b/vdsm/storage/task.py
@@ -173,7 +173,7 @@
                 self.state = self.unknown
             else:
                 self.state = getattr(self, state)
-        except:
+        except AttributeError:
             self.state = self.unknown

def value(self):
@@ -751,7 +751,7 @@
             self.log.error("Unexpected error", exc_info=True)
             try:
                 getProcPool().fileUtils.cleanupdir(taskDir)
-            except:
+            except Exception:
                 self.log.warning("can't remove temp taskdir %s" % taskDir)
             raise se.TaskPersistError("%s persist failed: %s" % (self, e))
         # Make sure backup dir doesn't exist
@@ -862,8 +862,6 @@
         except Exception as e:
             message = unicode(e)
             self._setError(e)
-        except:
-            self._setError()

self.log.debug("Task._run: %s %s %s failed - stopping task",
                        self, args, kargs)

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vdsm: storage/task.py replace/remove too generic except handlers
......................................................................

Patch Set 1:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/3766/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/3682/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/2875/ : SUCCESS

Change in vdsm[master]: vdsm: storage replace/remove too generic except handlers

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vdsm: storage replace/remove too generic except handlers
......................................................................

Patch Set 2:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/3768/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/3684/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/2877/ : SUCCESS

Change in vdsm[master]: vdsm: storage replace/remove too generic except handlers

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: vdsm: storage replace/remove too generic except handlers
......................................................................

Patch Set 2:

Good idea, will review later

Change in vdsm[master]: vdsm: storage replace/remove too generic except handlers

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: vdsm: storage replace/remove too generic except handlers
......................................................................

Patch Set 2: Code-Review-1

(2 comments)

very partial review. I think that we should use "except:" as pointers to find bad code. Once found, the bad code should be truly fixed.

....................................................
File vdsm/storage/dispatcher.py
Line 40:         self.help = None
Line 41:         try:
Line 42:             if hasattr(func.im_self, "help"):
Line 43:                 self.help = getattr(func.im_self, "help")
Line 44:         except AttributeError:
how can AttributeError be raised here?

self.help = getattr(func.im_self, "help", None) or getattr(func, "__doc__", "No help available for method %s" % name)

is a shorter equivalent of the current code...
Line 45:             pass
Line 46: 
Line 47:         if not self.help:
Line 48:             try:

....................................................
File vdsm/storage/hsm.py
Line 3481:                                          exc_info=True)
Line 3482:                         continue
Line 3483: 
Line 3484:             self.taskMng.prepareForShutdown()
Line 3485:         except Exception:
keeping the silent "pass" here is a sin. drop the try-block - our only caller should be able to test if this failed.
Line 3486:             pass
Line 3487: 
Line 3488:     @classmethod
Line 3489:     def __releaseLocks(cls):

Change in vdsm[master]: vdsm: storage replace/remove too generic except handlers

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: vdsm: storage replace/remove too generic except handlers
......................................................................

Patch Set 2:

(4 comments)

This is too big. Lets fix this module by module, correcting each case in the best way.

A start could be dispatcher.py, after addressing current comments.

....................................................
File vdsm/storage/dispatcher.py
Line 40:         self.help = None
Line 41:         try:
Line 42:             if hasattr(func.im_self, "help"):
Line 43:                 self.help = getattr(func.im_self, "help")
Line 44:         except AttributeError:
+1 for removing this stupid pointless try block as Dan suggests.
Line 45:             pass
Line 46: 
Line 47:         if not self.help:
Line 48:             try:

Line 50:             except AttributeError:
Line 51:                 pass
Line 52: 
Line 53:         if not self.help:
Line 54:             self.help = "No help available for method %s" % name
Lines 45-54 can be remove if using Dan suggested code.
Line 55: 
Line 56:     def run(self, *args, **kwargs):
Line 57:         try:
Line 58:             ctask = task.Task(id=None, name=self.name)

Line 66:             except se.GeneralException as e:
Line 67:                 self.log.error(e.response())
Line 68:                 return e.response()
Line 69:             except BaseException as e:
Line 70:                 # Do we really need to catch BaseException?
No, replace it to Exception.
Line 71:                 self.log.error(e, exc_info=True)
Line 72:                 defaultException = ctask.defaultException
Line 73:                 if defaultException and hasattr(defaultException, "response"):
Line 74:                     resp = defaultException.response()

Line 118:             if hasattr(self, method):
Line 119:                 help = getattr(self, method).im_self.help
Line 120:         except AttributeError:
Line 121:             pass
Line 122:         return help
Replace whole body with:

try:
        return getattr(self, method).im_self.help
    except AttributeError:
        return "No help available for method %s" % method

Change in vdsm[master]: vdsm: storage replace/remove too generic except handlers

[Vinzenz Feenstra]

Vinzenz Feenstra has posted comments on this change.

Change subject: vdsm: storage replace/remove too generic except handlers
......................................................................

Patch Set 2:

(2 comments)

....................................................
File vdsm/storage/dispatcher.py
Line 40:         self.help = None
Line 41:         try:
Line 42:             if hasattr(func.im_self, "help"):
Line 43:                 self.help = getattr(func.im_self, "help")
Line 44:         except AttributeError:
well if func has no attribute im_self...
Line 45:             pass
Line 46: 
Line 47:         if not self.help:
Line 48:             try:

....................................................
File vdsm/storage/hsm.py
Line 3481:                                          exc_info=True)
Line 3482:                         continue
Line 3483: 
Line 3484:             self.taskMng.prepareForShutdown()
Line 3485:         except Exception:
ok
Line 3486:             pass
Line 3487: 
Line 3488:     @classmethod
Line 3489:     def __releaseLocks(cls):

Change in vdsm[master]: vdsm: storage replace/remove too generic except handlers

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: vdsm: storage replace/remove too generic except handlers
......................................................................

Patch Set 2:

(1 comment)

....................................................
File vdsm/storage/dispatcher.py
Line 40:         self.help = None
Line 41:         try:
Line 42:             if hasattr(func.im_self, "help"):
Line 43:                 self.help = getattr(func.im_self, "help")
Line 44:         except AttributeError:
Correct! although we use Protect for HSM instance methods which always have im_self [1], this code is trying to support any function, and also trying to skip empty help or __doc__. So Dan code is not keeping the original behavior.

This code can replace lines 40-54, removing 2 bare except instances.

self.help = getattr(getattr(func, 'im_self', None), 'help', None)
    if not self.help:
        self.help = getattr(func, '__doc__', None)
        if not self.help:
            self.help =  "No help avialable for method %s" % name

[1] http://docs.python.org/2.6/reference/datamodel.html
Line 45:             pass
Line 46: 
Line 47:         if not self.help:
Line 48:             try:

Change in vdsm[master]: vdsm: storage replace/remove too generic except handlers

Itamar Heim has posted comments on this change.

Change subject: vdsm: storage replace/remove too generic except handlers
......................................................................

Patch Set 2:

ping

Change in vdsm[master]: vdsm: storage replace/remove too generic except handlers

Itamar Heim has abandoned this change.

Change subject: vdsm: storage replace/remove too generic except handlers
......................................................................

Abandoned

abandoning per no reply. please restore if still relevant.