Federico Simoncelli has posted comments on this change.
Change subject: Live Merge: Get volume chain for multiple drives
......................................................................
Patch Set 8:
(8 comments)
http://gerrit.ovirt.org/#/c/31366/8/vdsm/virt/vm.py
File vdsm/virt/vm.py:
Line 5514: return child
Line 5515: return None
Line 5516:
Line 5517: ret = {}
Line 5518: domXML = self._getUnderlyingVmInfo()
A whitespace would help readability.
Line 5519: for drive in drives:
Line 5520: alias = drive['alias']
Line 5521: diskXML = lookupDeviceXMLByAlias(domXML, alias)
Line 5522: if not diskXML:
Line 5516:
Line 5517: ret = {}
Line 5518: domXML = self._getUnderlyingVmInfo()
Line 5519: for drive in drives:
Line 5520: alias = drive['alias']
(Whitespace possibily)
Line 5521: diskXML = lookupDeviceXMLByAlias(domXML, alias)
Line 5522: if not diskXML:
Line 5523: continue
Line 5524: volChain = []
Line 5519: for drive in drives:
Line 5520: alias = drive['alias']
Line 5521: diskXML = lookupDeviceXMLByAlias(domXML, alias)
Line 5522: if not diskXML:
Line 5523: continue
What's this case? Is it problematic? Is it worth logging?
Line 5524: volChain = []
Line 5525: while True:
Line 5526: sourceXML = findElement(diskXML, 'source')
Line 5527: if not sourceXML:
Line 5521: diskXML = lookupDeviceXMLByAlias(domXML, alias)
Line 5522: if not diskXML:
Line 5523: continue
Line 5524: volChain = []
Line 5525: while True:
How hard would it be to split this while loop in an outside function method?
As a general comment, when you pluralize a command (as in this case "Get volume chain
for multiple drives") you add a for loop.
As a best practice you would add the for loop in another pluralized method that calls the
old one. This is often not possible because it's not efficient enough, anyway having
an utility method/function that processes the single entry is always nice.
Line 5526: sourceXML = findElement(diskXML, 'source')
Line 5527: if not sourceXML:
Line 5528: break
Line 5529: sourceAttr = ('file', 'dev')[drive.blockDev]
Line 5531: entry = VolumeChainEntry(pathToVolID(drive, path), path)
Line 5532: bsXML = findElement(diskXML, 'backingStore')
Line 5533: if not bsXML:
Line 5534: self.log.debug("<backingStore/> missing from
backing "
Line 5535: "chain for block device %s",
drive.name)
Is this always a block device?
Line 5536: break
Line 5537: diskXML = bsXML
Line 5538: volChain.insert(0, entry)
Line 5539: if volChain:
Line 5532: bsXML = findElement(diskXML, 'backingStore')
Line 5533: if not bsXML:
Line 5534: self.log.debug("<backingStore/> missing from
backing "
Line 5535: "chain for block device %s",
drive.name)
Line 5536: break
Could this return a partial volChain? shouldn't we raise an exception?
Line 5537: diskXML = bsXML
Line 5538: volChain.insert(0, entry)
Line 5539: if volChain:
Line 5540: ret[alias] = volChain
Line 5534: self.log.debug("<backingStore/> missing from
backing "
Line 5535: "chain for block device %s",
drive.name)
Line 5536: break
Line 5537: diskXML = bsXML
Line 5538: volChain.insert(0, entry)
(Whitespace possibily)
Line 5539: if volChain:
Line 5540: ret[alias] = volChain
Line 5541: return ret
Line 5542:
Line 5536: break
Line 5537: diskXML = bsXML
Line 5538: volChain.insert(0, entry)
Line 5539: if volChain:
Line 5540: ret[alias] = volChain
(Whitespace possibily)
Line 5541: return ret
Line 5542:
Line 5543: def _syncVolumeChain(self, drive):
Line 5544: def getVolumeInfo(device, volumeID):
--
To view, visit
http://gerrit.ovirt.org/31366
To unsubscribe, visit
http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I93d5a641b0814e3764c70bea8a6c1910a821adcc
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <alitke(a)redhat.com>
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes