Dan Kenigsberg has posted comments on this change. Change subject: Applied PEP-8 guideline on code ...................................................................... Patch Set 5: Looks good to me, approved (2 inline comments) .................................................... File vdsm/vm.py Line 364: BALLOON_DEVICES: [], REDIR_DEVICES: []} Line 365: Line 366: def _get_lastStatus(self): Line 367: PAUSED_STATES = ('Powering down', 'RebootInProgress', 'Up') Line 368: if not self._guestCpuRunning and self._lastStatus in PAUSED_STATES: why is this change pep8-related? (I don't REALLY care) Line 369: return 'Paused' Line 370: return self._lastStatus Line 371: Line 372: def _set_lastStatus(self, value): Line 410: """ Line 411: Backward compatibility for qa scripts that specify direct paths. Line 412: """ Line 413: legacies = [] Line 414: DEVICE_SPEC = ((0, 'hda'), (1, 'hdb'), (2, 'hdc'), (3, 'hdd')) I don't think this name is pretty good - drivespec is used for completely different thing Line 415: for index, linuxName in DEVICE_SPEC: Line 416: path = self.conf.get(linuxName) Line 417: if path: Line 418: legacies.append({'type': DISK_DEVICES, 'device': 'disk', -- To view, visit http://gerrit.ovirt.org/8933 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3b1ce77b01d848adb008d375decb2c214617ad2e Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Vinzenz Feenstra <vfeenstr(a)redhat.com&gt; Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt; Gerrit-Reviewer: Gal Hammer <ghammer(a)redhat.com&gt; Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com&gt; Gerrit-Reviewer: Peter V. Saveliev <peet(a)redhat.com&gt; Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt; Gerrit-Reviewer: ShaoHe Feng <shaohef(a)linux.vnet.ibm.com&gt; Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr(a)redhat.com&gt; Gerrit-Reviewer: Zhou Zheng Sheng <zhshzhou(a)linux.vnet.ibm.com&gt;

Zhou Zheng Sheng has posted comments on this change. Change subject: Applied PEP-8 guideline on code ...................................................................... Patch Set 5: Looks good to me, but someone else must approve pydiff finds some changes in logic, examine them myself, all are OK. -- To view, visit http://gerrit.ovirt.org/8933 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3b1ce77b01d848adb008d375decb2c214617ad2e Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Vinzenz Feenstra <vfeenstr(a)redhat.com&gt; Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt; Gerrit-Reviewer: Gal Hammer <ghammer(a)redhat.com&gt; Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com&gt; Gerrit-Reviewer: Peter V. Saveliev <peet(a)redhat.com&gt; Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt; Gerrit-Reviewer: ShaoHe Feng <shaohef(a)linux.vnet.ibm.com&gt; Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr(a)redhat.com&gt; Gerrit-Reviewer: Zhou Zheng Sheng <zhshzhou(a)linux.vnet.ibm.com&gt;

Vinzenz Feenstra has posted comments on this change. Change subject: Applied PEP-8 guideline on code ...................................................................... Patch Set 2: (1 inline comment) .................................................... File vdsm/vm.py Line 332: self._lastStatus = 'WaitForLaunch' Line 333: self._migrationSourceThread = self.MigrationSourceThreadClass(self) Line 334: self._kvmEnable = self.conf.get('kvmEnable', 'true') Line 335: self._guestSocketFile = constants.P_VDSM_RUN + self.conf['vmId'] + \ Line 336: '.guest.socket' However this is following PEP8 the previous one is not. We have just too many things disabled in our checks, that's why we do not see those kind of errors. Line 337: self._incomingMigrationFinished = threading.Event() Line 338: self.id = self.conf['vmId'] Line 339: self._volPrepareLock = threading.Lock() Line 340: self._initTimePauseCode = None -- To view, visit http://gerrit.ovirt.org/8933 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3b1ce77b01d848adb008d375decb2c214617ad2e Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Vinzenz Feenstra <vfeenstr(a)redhat.com&gt; Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt; Gerrit-Reviewer: Gal Hammer <ghammer(a)redhat.com&gt; Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com&gt; Gerrit-Reviewer: Peter V. Saveliev <peet(a)redhat.com&gt; Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt; Gerrit-Reviewer: ShaoHe Feng <shaohef(a)linux.vnet.ibm.com&gt; Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr(a)redhat.com&gt; Gerrit-Reviewer: Zhou Zheng Sheng <zhshzhou(a)linux.vnet.ibm.com&gt;

Zhou Zheng Sheng has posted comments on this change. Change subject: Applied PEP-8 guideline on code ...................................................................... Patch Set 3: (2 inline comments) .................................................... File vdsm/vm.py Line 800: Line 801: if newSize is None: Line 802: # newSize is always in megabytes Line 803: newSize = (config.getint('irs', 'volume_utilization_chunk_mb') + Line 804: ((vmDrive.apparentsize + constants.MEGAB - 1) / I'm do not agree with you Shao He, Here slash is the dividing operator. Line 805: constants.MEGAB)) Line 806: Line 807: if getattr(vmDrive, 'diskReplicate', None): Line 808: volInfo = {'poolID': vmDrive.diskReplicate['poolID'], Line 1136: return stats Line 1137: memUsage = 0 Line 1138: realMemUsage = int(stats['memUsage']) Line 1139: if realMemUsage != 0: Line 1140: memUsage = (100 - float(realMemUsage) / I'm do not agree with you Shao He, Here slash is the dividing operator. Line 1141: int(self.conf['memSize']) * 100) Line 1142: stats['memUsage'] = utils.convertToStr(int(memUsage)) Line 1143: stats['balloonInfo'] = self._getBalloonInfo() Line 1144: return stats -- To view, visit http://gerrit.ovirt.org/8933 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3b1ce77b01d848adb008d375decb2c214617ad2e Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Vinzenz Feenstra <vfeenstr(a)redhat.com&gt; Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt; Gerrit-Reviewer: Gal Hammer <ghammer(a)redhat.com&gt; Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com&gt; Gerrit-Reviewer: Peter V. Saveliev <peet(a)redhat.com&gt; Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt; Gerrit-Reviewer: ShaoHe Feng <shaohef(a)linux.vnet.ibm.com&gt; Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr(a)redhat.com&gt; Gerrit-Reviewer: Zhou Zheng Sheng <zhshzhou(a)linux.vnet.ibm.com&gt;

Zhou Zheng Sheng has posted comments on this change. Change subject: Applied PEP-8 guideline on code ...................................................................... Patch Set 2: I would prefer that you didn't submit this (11 inline comments) Do you mean to submit a thorough PEP 8 clean patch? I find some problems with vdsm/vm.py. If you want to make vdsm/vm.py PEP 8 clean, please fix those problems and add vdsm/vm.py to PEP8_WHITELIST in Makefile.am. # pep8 vdsm/vm.py vdsm/vm.py:104:80: E501 line too long (95 characters) vdsm/vm.py:205:80: E501 line too long (95 characters) vdsm/vm.py:220:80: E501 line too long (95 characters) vdsm/vm.py:361:80: E501 line too long (80 characters) vdsm/vm.py:410:80: E501 line too long (81 characters) vdsm/vm.py:508:80: E501 line too long (83 characters) vdsm/vm.py:566:80: E501 line too long (80 characters) vdsm/vm.py:575:80: E501 line too long (80 characters) vdsm/vm.py:855:80: E501 line too long (80 characters) vdsm/vm.py:977:80: E501 line too long (92 characters) vdsm/vm.py:987:80: E501 line too long (101 characters) vdsm/vm.py:1116:80: E501 line too long (93 characters) vdsm/vm.py:1118:80: E501 line too long (80 characters) vdsm/vm.py:1145:80: E501 line too long (82 characters) .................................................... File vdsm/guestIF.py Line 68: 'appsList': [], Line 69: 'disksUsage': [], Line 70: 'netIfaces': [], Line 71: 'memoryStats': {} Line 72: } Does the closing brace need to stand in a single line? Can it be: ... 'netIfaces': [], 'memoryStats': {}} Line 73: self._agentTimestamp = 0 Line 74: self._channelListener = channelListener Line 75: if connect: Line 76: try: Line 205: 'session': 'Unknown', Line 206: 'memUsage': 0, Line 207: 'appsList': self.guestInfo['appsList'], Line 208: 'guestIPs': self.guestInfo['guestIPs'] Line 209: } Same as line 72. Line 210: Line 211: def onReboot(self): Line 212: self.guestStatus = 'RebootInProgress' Line 213: self.guestInfo['lastUser'] = '' + self.guestInfo['username'] .................................................... File vdsm/vm.py Line 128: self.destServer = vdscli.connect( Line 129: serverAddress, Line 130: useSSL=True, Line 131: TransportClass=kaxmlrpclib.TcpkeepSafeTransport Line 132: ) Can I suggest: self.destServer = vdscli.connect( serverAddress, useSSL=True, TransportClass=kaxmlrpclib.TcpkeepSafeTransport) To me, an extra indent for the parameters makes them looks different from the "destServer", and I think the closing parenthesis does not need a whole line. Line 133: else: Line 134: self.destServer = kaxmlrpclib.Server('http://' + serverAddress) Line 135: self.log.debug('Destination server is: ' + serverAddress) Line 136: try: Line 253: 'dst': self._dst, Line 254: 'mode': self._mode, Line 255: 'method': self._method, Line 256: 'dstparams': self._dstparams Line 257: } Same as line 132. Line 258: self._vm.saveState() Line 259: self._startUnderlyingMigration() Line 260: self._finishSuccessfully() Line 261: except libvirt.libvirtError, e: Line 264: 'status': { Line 265: 'code': errCode['migCancelErr'], Line 266: 'message': 'Migration canceled' Line 267: } Line 268: } How about the following. self.status = {'status': {'code': errCode['migCancelErr'], 'message': 'Migration canceled'}} Line 269: raise Line 270: finally: Line 271: if '_migrationParams' in self._vm.conf: Line 272: del self._vm.conf['_migrationParams'] Line 332: self._lastStatus = 'WaitForLaunch' Line 333: self._migrationSourceThread = self.MigrationSourceThreadClass(self) Line 334: self._kvmEnable = self.conf.get('kvmEnable', 'true') Line 335: self._guestSocketFile = constants.P_VDSM_RUN + self.conf['vmId'] + \ Line 336: '.guest.socket' The original indentation looks more readable to me. Line 337: self._incomingMigrationFinished = threading.Event() Line 338: self.id = self.conf['vmId'] Line 339: self._volPrepareLock = threading.Lock() Line 340: self._initTimePauseCode = None Line 394: res = self.cif.irs.getVolumeSize( Line 395: drv['domainID'], Line 396: drv['poolID'], Line 397: drv['imageID'], Line 398: drv['volumeID']) A little more indentation could make it more readable. res = self.cif.irs.getVolumeSize( drv['domainID'], drv['poolID'], drv['imageID'], drv['volumeID']) Line 399: drv['truesize'] = res['truesize'] Line 400: drv['apparentsize'] = res['apparentsize'] Line 401: else: Line 402: drv['truesize'] = 0 Line 494: 'device': 'memballoon', Line 495: 'specParams': { Line 496: 'model': 'none' Line 497: } Line 498: }) Since indentation has already given the information of the syntax level of the elements, I don't like single lines contains only braces and parentheses. Line 499: Line 500: return devices Line 501: Line 502: def getConfController(self): Line 798: if newSize is None: Line 799: # newSize is always in megabytes Line 800: newSize = (config.getint('irs', 'volume_utilization_chunk_mb') Line 801: + ((vmDrive.apparentsize + constants.MEGAB - 1) Line 802: / constants.MEGAB)) It's better to split the line after a binary operator as follow. newSize = (config.getint('irs', 'volume_utilization_chunk_mb') + ((vmDrive.apparentsize + constants.MEGAB - 1) / constants.MEGAB)) Line 803: Line 804: if getattr(vmDrive, 'diskReplicate', None): Line 805: volInfo = {'poolID': vmDrive.diskReplicate['poolID'], Line 806: 'domainID': vmDrive.diskReplicate['domainID'], Line 852: Line 853: if volSizeRes['status']['code']: Line 854: raise RuntimeError( Line 855: "Cannot get the volume size for %s (domainID: %s, volumeID: %s)" Line 856: % (volInfo['name'], It's better to split the line after a binary operator. For example, raise RuntimeError( "Cannot get the volume size for " "%s (domainID: %s, volumeID: %s)" % (volInfo['name'], volInfo['domainID'], volInfo['volumeID'])) Line 857: volInfo['domainID'], Line 858: volInfo['volumeID']) Line 859: ) Line 860: Line 1087: 'kvmEnable': self._kvmEnable, Line 1088: 'network': {}, 'disks': {}, Line 1089: 'monitorResponse': str(self._monitorResponse), Line 1090: 'elapsedTime': str(int(time.time() - self._startTime)), Line 1091: } The original style is OK. Need not to change. Line 1092: if 'cdrom' in self.conf: Line 1093: stats['cdrom'] = self.conf['cdrom'] Line 1094: if 'boot' in self.conf: Line 1095: stats['boot'] = self.conf['boot'] -- To view, visit http://gerrit.ovirt.org/8933 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3b1ce77b01d848adb008d375decb2c214617ad2e Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Vinzenz Feenstra <vfeenstr(a)redhat.com&gt; Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt; Gerrit-Reviewer: Gal Hammer <ghammer(a)redhat.com&gt; Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com&gt; Gerrit-Reviewer: Peter V. Saveliev <peet(a)redhat.com&gt; Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt; Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr(a)redhat.com&gt; Gerrit-Reviewer: Zhou Zheng Sheng <zhshzhou(a)linux.vnet.ibm.com&gt;