Change in vdsm[master]: blockSD: raise an exception if missing physical volume

nsoffer at redhat.com nsoffer at redhat.com
Mon Jun 30 11:02:55 UTC 2014


Nir Soffer has posted comments on this change.

Change subject: blockSD: raise an exception if missing physical volume
......................................................................


Patch Set 4: Code-Review-1

(2 comments)

http://gerrit.ovirt.org/#/c/27442/4/vdsm/storage/blockSD.py
File vdsm/storage/blockSD.py:

Line 669:                 pv = lvm.getPV(dev)
Line 670:             except se.InaccessiblePhysDev:
Line 671:                 cls.log.error("INTERNAL: Unable to get metadata from PV: %s",
Line 672:                               dev)
Line 673:                 raise
Please remove this log and let this error bubble up the engine - we will have a clear traceback in the log showing this call, so this error is not needed.

We have this bad style of double errors in lot of places, lets not continue with this. Exceptions should simplify error handling, so you handle only stuff you *can* handle and letting others handle other errors. You clearly cannot handle this error as you raise.
Line 674: 
Line 675:             pvInfo = {}
Line 676:             pvInfo["guid"] = os.path.basename(pv.name)
Line 677:             pvInfo["uuid"] = pv.uuid


http://gerrit.ovirt.org/#/c/27442/4/vdsm/storage/lvm.py
File vdsm/storage/lvm.py:

Line 824: def getPV(pv):
Line 825:     pv = _lvminfo.getPv(_fqpvname(pv))
Line 826:     if not pv:
Line 827:         raise se.InaccessiblePhysDev(pv)
Line 828:     else:
> Style: I wouldn't use an else block here.
+1
Line 829:         return pv
Line 830: 
Line 831: 
Line 832: def getAllPVs():


-- 
To view, visit http://gerrit.ovirt.org/27442
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9932b044c8b439dc8b1f09191a5d89f4bc44c38a
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Xavi Francisco <xfrancis at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list