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 <[email protected]> Gerrit-Reviewer: Adam Litke <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
