Change in vdsm[master]: BZ#788640 - Add [block|file]SD.getAllVolumes

danken at redhat.com danken at redhat.com
Sun May 13 19:15:48 UTC 2012


Dan Kenigsberg has posted comments on this change.

Change subject: BZ#788640 - Add [block|file]SD.getAllVolumes
......................................................................


Patch Set 11: I would prefer that you didn't submit this

(7 inline comments)

....................................................
File vdsm/storage/blockSD.py
Line 127: def _getVolsTree(sdUUID):
Return dict {volUUID: (name, image, parent)}

Line 151: 
imgUUIDs is a list of all images dependant on volUUID. For template base volumes, the first image is the template's image. For other volumes, there is just a single imageUUID.

Line 161:         if vImg not in res[vName]['imgs']:
I'm lost. And I've passed Graph Theory 101.

Why not put the self-image first for EVERYTHING, when initializing res[vName] above?

Either way, I would be much happier if you add a unit test for this function: mock _getVolsTree() to return an interesting set of BlockSdVol tuples, and verify that getAllVolumes() generates the needed dict.

Line 168:     return dict((k, sd.ImgsPar(tuple(v['imgs']), v['parent'])) for k, v in res.iteritems())
new code should have shorter lines.

....................................................
File vdsm/storage/fileSD.py
Line 63:     sdUUID = len(os.path.normpath(config.get('irs', 'repository')).split('/')) + sdUUIDPos
no idea why this change was so urgent to do in this patch.

Line 261:         volMetaPaths = self.oop.glob.glob(volMetaPattern)
here, too, I'd feel more comfortable if you add a unit test, supplying a list of paths and validating the output.

Line 279:                 if len(tuple(vPath for vPath in volMetaPaths
Just like Ayal, I have no idea why you bother to convert a temporary list to a tuple. it is not a returned object. The conversion of line 288 is comprehensible.

More importantly, this line takes you into O(volMetaPaths**2). You could do this in O(volMetaPaths) using itertools.groupby() with keyfunc=imgOfMetaPath

--
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: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo <ewarszaw at redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw at redhat.com>
Gerrit-Reviewer: Haim Ateya <hateya at redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>


More information about the vdsm-patches mailing list