Igor Lvovsky has posted comments on this change.
Change subject: [WIP] Added blockSD.getImageMap(), [block|file]SD.getAllVolumes
......................................................................
Patch Set 3: I would prefer that you didn't submit this
(16 inline comments)
....................................................
File vdsm/storage/blockSD.py
Line 59: BlockSDVol = namedtuple("BlockSDVol", "name, image, parent")
why you don't use similar in fileSD?
Line 142: raise se.IncorrectFormat("Volume %s lacks minimal tag set"
We are using IncorrectFormat to absolutelly different exception (wrong volume
format).
So, please pick more proper exception here or create a new one
Line 149: def getChain(leaf):
Just don't forget to remove former getChain
Line 157: metadata.
my comment from patch 1 still valid :)
I know that it's not easy, but please try to explain this somehow. I am really
don't understand what do you mean here.
Line 160: #leaf = vols[leafName]
dead code
Line 169: "references for volume %s" % (image,
p))
In previous behaviour in getChain we collect the chain without its template.
In this 'while' you will collect the whole chain include the template if exists.
Did you meant for that?
Line 172: res = BlockSDWalk({p.image: tuple((p,))}, {image:
tuple(chain)})
WOW, I'm realy don't understand this (and it's definitely different from doc
string ). But, maybe it's my age... :)
Line 179: chainImg, chainVols = walk.chain.itervalues()
Are you sure that this is work?
Line 181: headImg, head = walk.template.itervalues()
Same as above
Line 197: # template should be in the map already
That's it. I am quit, I can't review this anymore :)
Line 215: except KeyError:
hmmm, you ignored my comment from previous patchset.
you can't get KeyError here? if v.xxx doesn't exist you will get AttributeError
but in your case if v.parent or v.name don't exist you will get IncorrectFormat
from _makeBlockSDVols, right ?
Line 220: leafs = tuple(vol for vName, vol in vols.iteritems() if vName not
in parents)
As I wrote before.
it's so complicated. Can you at least add some comments here?
Line 222: walk = getChain(leaf)
walk ??? what does it mean?
Line 929: Return dict {volUUID: ([imgUUID1, imgUUID2], parentUUID)]}.
looks like redundant ']'
....................................................
File vdsm/storage/fileSD.py
Line 245: Return dict {volUUID: imgUUID, parentUUID} of the domain.
according to your code it should be
{volUUID: ([imgUUID], parentUUID)}
right?
Why so complicate, dict of sets of lists ????
Line 251: volMetaPaths =
self.oop.getProcessPool(self.sdUUID).glob.glob(volMetaPattern)
hmmm, I meant to
self.oop.glob.glob(volMetaPattern)
--
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: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo <[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
