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

danken at redhat.com danken at redhat.com
Mon May 14 08:58:17 UTC 2012


Dan Kenigsberg has posted comments on this change.

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


Patch Set 11: (2 inline comments)

....................................................
File vdsm/storage/blockSD.py
Line 161:         if vImg not in res[vName]['imgs']:
but if, indeed, the self-image should always come first, this code is overly complex and misleading! Do you confirm? We can decide whether to rewrite it now, tomorrow, or never, but we should first agree on what this function does.

....................................................
File vdsm/storage/fileSD.py
Line 279:                 if len(tuple(vPath for vPath in volMetaPaths
> have no list this is a generator expression and we need an iterable

ok, I've missed that.

> this code was extensively tested I prefer to do performance

I hope that once you have a unit test for this function, you would feel more confidence to reimplement it. Please do not dismiss this out-of-hand.

--
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