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():