Change in vdsm[master]: [WIP] Added blockSD.getImageMap(), [block|file]SD.getAllVolu...

ilvovsky at redhat.com ilvovsky at redhat.com
Mon Apr 16 15:12:11 UTC 2012


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 <ewarszaw at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw 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