Ayal Baron has posted comments on this change.
Change subject: BZ#788640 - Add [block|file]SD.getAllVolumes
......................................................................
Patch Set 7: I would prefer that you didn't submit this
(8 inline comments)
....................................................
Commit Message
Line 7: BZ#788640 - Add [block|file]SD.getAllVolumes
why?
....................................................
File vdsm/storage/blockSD.py
Line 127: def _makeBlockSDVols(lvs):
s/makeBlockSDVols/getVolsTree/ ?
Line 142: raise se.IncorrectFormat("Volume %s lacks minimal tag
set"
a single bad LV prevents running all operations?
Line 147: def getAllVolumes(sdUUID):
the fact that it *can* be a static method doesn't mean it *should* be, esp.
seeing as this is specific to blockSD
Line 153: lvs = lvm.getLV(sdUUID)
why is this called here and not in the 'makeBlockSDVols'
Line 157: res[vName] = {'imgs': [], 'parent': None}
why doesn't makeBlockSDVols already return a dictionary of this structure?
i.e. get rid of lv.name in BlockSDVol and return a mutable dictionary:
vols[lv.name] = {'imgs': [image], 'parent': parent}
Then you could get rid of this for loop as well as the first three lines in the
next for loop
....................................................
File vdsm/storage/fileSD.py
Line 272: if len(tuple(vPath for vPath in volMetaPaths
since you're not storing it, converting the list into tuple is redundant.
Line 273: if imgUUID in vPath)) > 1:
shouldn't this be volUUID?
--
To view, visit http://gerrit.ovirt.org/3462
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I7eccf5ca100bd354aa09208ca60bb112fb697063
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo <[email protected]>
Gerrit-Reviewer: Ayal Baron <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Eduardo <[email protected]>
Gerrit-Reviewer: Igor Lvovsky <[email protected]>
Gerrit-Reviewer: Saggi Mizrahi <[email protected]>
_______________________________________________
vdsm-patches mailing list
[email protected]
https://fedorahosted.org/mailman/listinfo/vdsm-patches
