Antoni Segura Puimedon has uploaded a new change for review.
Change subject: cleanup: drop several unused local variables ......................................................................
cleanup: drop several unused local variables
Change-Id: Ib81c292f900154819e8852c21ae389c323034999 Signed-off-by: Antoni S. Puimedon asegurap@redhat.com --- M client/vdsClient.py M lib/vdsm/netinfo.py M lib/yajsonrpc/protonReactor.py M tests/functional/networkTests.py M tests/functional/virtTests.py M tests/hookValidation.py M tests/hooksTests.py M tests/jsonRpcTests.py M tests/miscTests.py M tests/testValidation.py M tests/vmTests.py M vds_bootstrap/setup M vds_bootstrap/vds_bootstrap.py M vds_bootstrap/vds_bootstrap_complete.py M vdsm/storage/hsm.py M vdsm/storage/iscsi.py M vdsm/storage/misc.py M vdsm/storage/remoteFileHandler.py M vdsm/storage/resourceManager.py M vdsm/storage/sp.py M vdsm/storage/task.py M vdsm/vm.py M vdsm_api/Bridge.py M vdsm_api/process-schema.py M vdsm_api/vdsmapi.py 25 files changed, 23 insertions(+), 49 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/35/20535/1
diff --git a/client/vdsClient.py b/client/vdsClient.py index 37dd7cb..4c09546 100644 --- a/client/vdsClient.py +++ b/client/vdsClient.py @@ -842,7 +842,6 @@ masterDom = args[3] domList = args[4].split(",") mVer = int(args[5]) - pool = None if len(args) > 6: pool = self.s.createStoragePool(poolType, spUUID, poolName, masterDom, diff --git a/lib/vdsm/netinfo.py b/lib/vdsm/netinfo.py index e8f2b8d..cf70089 100644 --- a/lib/vdsm/netinfo.py +++ b/lib/vdsm/netinfo.py @@ -93,8 +93,6 @@ The list of nics is built by filtering out nics defined as hidden, fake or hidden bonds (with related nics'slaves). """ - res = [] - def isHiddenNic(device): """ Returns boolean given the device name stating @@ -397,7 +395,7 @@ "Convert an integer to the corresponding ip address in the dot-notation" ip_address = []
- for i in xrange(4): + for _ in xrange(4): ip_num, ip_val = divmod(ip_num, 256) ip_address.append(str(ip_val))
diff --git a/lib/yajsonrpc/protonReactor.py b/lib/yajsonrpc/protonReactor.py index 557600c..7892e3c 100644 --- a/lib/yajsonrpc/protonReactor.py +++ b/lib/yajsonrpc/protonReactor.py @@ -376,7 +376,6 @@ proton.pn_link_advance(link)
def createListener(self, address, acceptHandler): - host, port = address return self._scheduleOp(True, self._createListener, address, acceptHandler)
diff --git a/tests/functional/networkTests.py b/tests/functional/networkTests.py index a9149be..ba93343 100644 --- a/tests/functional/networkTests.py +++ b/tests/functional/networkTests.py @@ -377,7 +377,7 @@ for index in range(VLAN_COUNT)] with dummyIf(1) as nics: firstVlan, firstVlanId = NET_VLANS[0] - status, msg = self.vdsm_net.addNetwork(firstVlan, vlan=firstVlanId, + _ = self.vdsm_net.addNetwork(firstVlan, vlan=firstVlanId, bond=BONDING_NAME, nics=nics, opts=opts) with nonChangingOperstate(BONDING_NAME): diff --git a/tests/functional/virtTests.py b/tests/functional/virtTests.py index cdb695a..85781f7 100644 --- a/tests/functional/virtTests.py +++ b/tests/functional/virtTests.py @@ -83,7 +83,7 @@ def _genInitramfs(): fd, path = tempfile.mkstemp() cmd = [_mkinitrd.cmd, "-f", path, _kernelVer] - rc, out, err = execCmd(cmd, sudo=False) + _ = execCmd(cmd, sudo=False) os.chmod(path, 0o644) return path
diff --git a/tests/hookValidation.py b/tests/hookValidation.py index 80e7239..208ed35 100644 --- a/tests/hookValidation.py +++ b/tests/hookValidation.py @@ -67,8 +67,6 @@
cookie_file = _createHookScript(hook_path, hook_name, hook_script)
- output = None - try: kwargs['hook_cookiefile'] = cookie_file output = test_function(*args, **kwargs) diff --git a/tests/hooksTests.py b/tests/hooksTests.py index 1018a4e..ddb3530 100644 --- a/tests/hooksTests.py +++ b/tests/hooksTests.py @@ -42,7 +42,7 @@ echo -n %s >> "$_hook_domxml" """ scripts = [tempfile.NamedTemporaryFile(dir=dirName, delete=False) - for n in xrange(Q)] + for _ in xrange(Q)] scripts.sort(key=lambda f: f.name) for n, script in enumerate(scripts): script.write(code % n) diff --git a/tests/jsonRpcTests.py b/tests/jsonRpcTests.py index a7b565f..00025ae 100644 --- a/tests/jsonRpcTests.py +++ b/tests/jsonRpcTests.py @@ -85,7 +85,7 @@ def serve(reactor): try: reactor.process_requests() - except socket.error as e: + except socket.error: pass except Exception as e: self.log.error("Reactor died unexpectedly", exc_info=True) diff --git a/tests/miscTests.py b/tests/miscTests.py index c836e55..1a9a16c 100644 --- a/tests/miscTests.py +++ b/tests/miscTests.py @@ -432,7 +432,7 @@ os.chmod(dstPath, 0o666)
#Copy - rc, out, err = misc.ddWatchCopy(srcPath, dstPath, None, len(data)) + _ = misc.ddWatchCopy(srcPath, dstPath, None, len(data))
#Get copied data readData = open(dstPath).read() @@ -448,7 +448,7 @@ fd, path = tempfile.mkstemp()
try: - for i in xrange(repetitions): + for _ in xrange(repetitions): os.write(fd, data) self.assertEquals(os.stat(path).st_size, misc.MEGA) except: @@ -474,7 +474,7 @@ self.assertEquals(os.stat(path).st_size, misc.MEGA * 2)
with open(path, "r") as f: - for i in xrange(repetitions): + for _ in xrange(repetitions): self.assertEquals(f.read(len(data)), data) finally: os.unlink(path) @@ -501,7 +501,7 @@ misc.MEGA * 2 + len(add_data))
with open(path, "r") as f: - for i in xrange(repetitions): + for _ in xrange(repetitions): self.assertEquals(f.read(len(data)), data) # Checking the additional data self.assertEquals(f.read(len(add_data)), add_data) @@ -535,7 +535,7 @@ os.chmod(dstPath, 0o666)
#Copy - rc, out, err = misc.ddWatchCopy(srcPath, dstPath, None, len(data)) + _ = misc.ddWatchCopy(srcPath, dstPath, None, len(data))
#Get copied data readData = open(dstPath).read() diff --git a/tests/testValidation.py b/tests/testValidation.py index d370971..92790d9 100644 --- a/tests/testValidation.py +++ b/tests/testValidation.py @@ -110,7 +110,7 @@ def wrapper(*args, **kwargs): if not os.path.exists('/sys/module/dummy'): cmd_modprobe = [modprobe.cmd, "dummy"] - rc, out, err = utils.execCmd(cmd_modprobe, sudo=True) + _ = utils.execCmd(cmd_modprobe, sudo=True) return f(*args, **kwargs) return wrapper
diff --git a/tests/vmTests.py b/tests/vmTests.py index 1f69f0a..9d91723 100644 --- a/tests/vmTests.py +++ b/tests/vmTests.py @@ -390,7 +390,7 @@ driveInput.update({'shared': 'UNKNOWN-VALUE'})
with self.assertRaises(ValueError): - drive = vm.Drive({}, self.log, **driveInput) + _ = vm.Drive({}, self.log, **driveInput)
def testDriveXML(self): SERIAL = '54-a672-23e5b495a9ea' diff --git a/vds_bootstrap/setup b/vds_bootstrap/setup index 778dc12..701df8b 100755 --- a/vds_bootstrap/setup +++ b/vds_bootstrap/setup @@ -63,7 +63,6 @@ return False, HYPERVISOR_RELEASE_FILE + ", " + REDHAT_RELEASE_FILE
def get_id_line(): - line = '' RELEASE_FILE = None
try: @@ -193,7 +192,6 @@ import calendar
return_value = False - ticket = None
try: time_struct = time.strptime(systime, '%Y-%m-%dT%H:%M:%S') diff --git a/vds_bootstrap/vds_bootstrap.py b/vds_bootstrap/vds_bootstrap.py index a9dc901..e45c8b6 100755 --- a/vds_bootstrap/vds_bootstrap.py +++ b/vds_bootstrap/vds_bootstrap.py @@ -289,7 +289,6 @@ """ status = "OK" message = 'Host properly registered with RHN/Satellite.' - rc = True
try: rc = bool(deployUtil.yumListPackages(VDSM_NAME)) @@ -316,7 +315,6 @@ """ status = "OK" message = 'Available VDSM matches requirements' - rc = True
try: rc = deployUtil.yumSearchVersion(VDSM_NAME, VDSM_MIN_VER) @@ -393,7 +391,6 @@ """ os_status = "FAIL" kernel_status = "FAIL" - os_message = "Unsupported platform version" os_name = "Unknown OS" kernel_message = '' self.rc = True @@ -741,8 +738,6 @@ return self.rc
def _addNetwork(self, vdcName, vdcPort): - fReturn = True - #add management bridge try: fReturn = deployUtil.makeBridge( @@ -859,9 +854,6 @@ # TODO remove legacy if deployUtil.getBootstrapInterfaceVersion() == 1 and \ engine_ssh_key is None: - vdcAddress = None - vdcPort = None - vdcAddress, vdcPort = deployUtil.getAddress(url) if vdcAddress is not None: strKey = deployUtil.getAuthKeysFile(vdcAddress, vdcPort) diff --git a/vds_bootstrap/vds_bootstrap_complete.py b/vds_bootstrap/vds_bootstrap_complete.py index fd18847..07c3610 100755 --- a/vds_bootstrap/vds_bootstrap_complete.py +++ b/vds_bootstrap/vds_bootstrap_complete.py @@ -101,7 +101,6 @@ except: arg = 1
- res = True try: res = deployUtil.instCert(rnum, VDSM_CONF_FILE) if res: diff --git a/vdsm/storage/hsm.py b/vdsm/storage/hsm.py index 4579763..322ee8b 100644 --- a/vdsm/storage/hsm.py +++ b/vdsm/storage/hsm.py @@ -1095,7 +1095,7 @@ misc.validateN(hostID, 'hostID') # already disconnected/or pool is just unknown - return OK try: - pool = self.getPool(spUUID) + _ = self.getPool(spUUID) except se.StoragePoolUnknown: self.log.warning("disconnect sp: %s failed. Known pools %s", spUUID, self.pools) @@ -1861,7 +1861,7 @@ self.log.info("spUUID=%s master=%s", spUUID, masterDom)
try: - pool = self.getPool(spUUID) + _ = self.getPool(spUUID) except se.StoragePoolUnknown: pool = sp.StoragePool(spUUID, self.domainMonitor, self.taskMng) else: diff --git a/vdsm/storage/iscsi.py b/vdsm/storage/iscsi.py index 7da94ab..9976026 100644 --- a/vdsm/storage/iscsi.py +++ b/vdsm/storage/iscsi.py @@ -415,7 +415,7 @@ log.debug("Performing SCSI scan, this will take up to %s seconds", maxTimeout) time.sleep(minTimeout) - for i in xrange(maxTimeout - minTimeout): + for _ in xrange(maxTimeout - minTimeout): for p in processes[:]: (hba, proc) = p if proc.wait(0): @@ -429,7 +429,7 @@ time.sleep(1) else: log.warning("Still waiting for scsi scan of hbas: %s", - tuple(hba for p in processes)) + tuple(hba for _ in processes))
def devIsiSCSI(dev): diff --git a/vdsm/storage/misc.py b/vdsm/storage/misc.py index fc13a9c..5245264 100644 --- a/vdsm/storage/misc.py +++ b/vdsm/storage/misc.py @@ -484,7 +484,6 @@ log.debug("dir: %s, prefixName: %s, versions: %s" % (directory, prefixName, gen)) gen = int(gen) - files = os.listdir(directory) files = glob.glob("%s*" % prefixName) fd = {} for fname in files: @@ -614,7 +613,6 @@ return self.acquire(True)
def acquire(self, exclusive): - currentEvent = None currentThread = threading.currentThread()
# Handle reacquiring lock in the same thread @@ -1081,7 +1079,7 @@ maxthreads -= 1
# waiting for rest threads to end - for i in xrange(threadsCount): + for _ in xrange(threadsCount): yield respQueue.get()
diff --git a/vdsm/storage/remoteFileHandler.py b/vdsm/storage/remoteFileHandler.py index 5b24053..accf51c 100644 --- a/vdsm/storage/remoteFileHandler.py +++ b/vdsm/storage/remoteFileHandler.py @@ -275,7 +275,7 @@ def __init__(self, numOfHandlers): self._numOfHandlers = numOfHandlers self.handlers = [None] * numOfHandlers - self.occupied = [Lock() for i in xrange(numOfHandlers)] + self.occupied = [Lock() for _ in xrange(numOfHandlers)]
def _isHandlerAvailable(self, poolHandler): if poolHandler is None: diff --git a/vdsm/storage/resourceManager.py b/vdsm/storage/resourceManager.py index 14049dc..486ea18 100644 --- a/vdsm/storage/resourceManager.py +++ b/vdsm/storage/resourceManager.py @@ -926,7 +926,7 @@ return req.wait(timeout)
# req not found - check that it is not granted - for fullName in self.resources: + for _ in self.resources: return True
# Note that there is a risk of another thread that is racing with us diff --git a/vdsm/storage/sp.py b/vdsm/storage/sp.py index 38cd453..db66662 100644 --- a/vdsm/storage/sp.py +++ b/vdsm/storage/sp.py @@ -1326,7 +1326,6 @@ self.log.info("spUUID=%s sdUUID=%s", self.spUUID, sdUUID) vms = self._getVMsPath(sdUUID) # We should exclude 'masterd' link from IMG_METAPATTERN globing - vmUUID = ovf = imgList = '' for vm in vmList: if not vm: continue diff --git a/vdsm/storage/task.py b/vdsm/storage/task.py index 4eff5c1..0532b02 100644 --- a/vdsm/storage/task.py +++ b/vdsm/storage/task.py @@ -872,10 +872,7 @@
def _runJobs(self): result = "" - code = 100 - message = "Unknown Error" i = 0 - j = None try: if self.aborting(): raise se.TaskAborted("shutting down") @@ -891,7 +888,6 @@ if result is None: result = "" i += 1 - j = None self._updateResult(0, "%s jobs completed successfully" % i, result) self._updateState(State.finished) self.log.debug('Task.run: exit - success: result %s' % result) diff --git a/vdsm/vm.py b/vdsm/vm.py index 0c12334..5e1c7f1 100644 --- a/vdsm/vm.py +++ b/vdsm/vm.py @@ -3058,7 +3058,7 @@ self._dom.attachDevice(nicXml) except libvirt.libvirtError as e: self.log.error("Hotplug failed", exc_info=True) - nicXml = hooks.after_nic_hotplug_fail( + _ = hooks.after_nic_hotplug_fail( nicXml, self.conf, params=customProps) if e.get_error_code() == libvirt.VIR_ERR_NO_DOMAIN: return errCode['noVM'] @@ -3760,7 +3760,7 @@ "trying again without it (%s)", e) try: self._dom.snapshotCreateXML(snapxml, snapFlags) - except Exception as e: + except Exception: self.log.error("Unable to take snapshot", exc_info=True) if memoryParams: self.cif.teardownVolumePath(memoryVol) diff --git a/vdsm_api/Bridge.py b/vdsm_api/Bridge.py index b9fdaf8..4812354 100644 --- a/vdsm_api/Bridge.py +++ b/vdsm_api/Bridge.py @@ -34,7 +34,6 @@
def dispatch(self, name, argobj): methodName = name.replace('.', '_') - result = None try: fn = getattr(self, methodName) except AttributeError: diff --git a/vdsm_api/process-schema.py b/vdsm_api/process-schema.py index c4bda0d..307d498 100755 --- a/vdsm_api/process-schema.py +++ b/vdsm_api/process-schema.py @@ -255,12 +255,12 @@ # Union member types names = strip_stars(s.get('data', [])) types = filter_types(names) - details = [None for n in names] + details = [None for _ in names] attr_table('Types', names, types, details) elif 'enum' in s: # Enum values names = strip_stars(s.get('data', [])) - types = [None for n in names] + types = [None for _ in names] details = [s['info_data'][n] for n in names] attr_table('Values', names, types, details) elif 'map' in s: diff --git a/vdsm_api/vdsmapi.py b/vdsm_api/vdsmapi.py index db29c13..c38ff01 100644 --- a/vdsm_api/vdsmapi.py +++ b/vdsm_api/vdsmapi.py @@ -92,7 +92,6 @@ def parse_schema(fp): exprs = [] expr = '' - expr_eval = None
for line in fp: if line.startswith('#') or line == '\n':
oVirt Jenkins CI Server has posted comments on this change.
Change subject: cleanup: drop several unused local variables ......................................................................
Patch Set 1: Code-Review-1 Verified-1
Build Failed
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/5067/ : UNSTABLE
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4263/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/5141/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_network_functional_tests/739/ : FAILURE
Dan Kenigsberg has posted comments on this change.
Change subject: cleanup: drop several unused local variables ......................................................................
Patch Set 1: Code-Review-1
(9 comments)
could you explain (in the commit message) which tool did you use for the static ananlysis? we may want to add it to `make check-local`.
.................................................... File tests/functional/networkTests.py Line 376: NET_VLANS = [(NETWORK_NAME + str(index), str(index)) Line 377: for index in range(VLAN_COUNT)] Line 378: with dummyIf(1) as nics: Line 379: firstVlan, firstVlanId = NET_VLANS[0] Line 380: _ = self.vdsm_net.addNetwork(firstVlan, vlan=firstVlanId, adding
self.assertEquals(status, SUCCESS, msg)
makes more sense here. Line 381: bond=BONDING_NAME, Line 382: nics=nics, opts=opts) Line 383: with nonChangingOperstate(BONDING_NAME): Line 384: for netVlan, vlanId in NET_VLANS[1:]:
.................................................... File tests/functional/virtTests.py Line 82: Line 83: def _genInitramfs(): Line 84: fd, path = tempfile.mkstemp() Line 85: cmd = [_mkinitrd.cmd, "-f", path, _kernelVer] Line 86: _ = execCmd(cmd, sudo=False) let's raise an exception if this fails. Line 87: os.chmod(path, 0o644) Line 88: return path Line 89: Line 90:
.................................................... File tests/miscTests.py Line 431: dstFd, dstPath = tempfile.mkstemp() Line 432: os.chmod(dstPath, 0o666) Line 433: Line 434: #Copy Line 435: _ = misc.ddWatchCopy(srcPath, dstPath, None, len(data)) better assert rc == 0. Since this requires moving the cleanup to a "finally" block, this may be postponed to a later patch. Line 436: Line 437: #Get copied data Line 438: readData = open(dstPath).read() Line 439:
.................................................... File tests/vmTests.py Line 389: # Negative flow, unsupported value Line 390: driveInput.update({'shared': 'UNKNOWN-VALUE'}) Line 391: Line 392: with self.assertRaises(ValueError): Line 393: _ = vm.Drive({}, self.log, **driveInput) no need for assignment Line 394: Line 395: def testDriveXML(self): Line 396: SERIAL = '54-a672-23e5b495a9ea' Line 397: devConfs = [
.................................................... File vds_bootstrap/setup Line 192 Line 193 Line 194 Line 195 Line 196 this assignment is the list of this code's problem...
.................................................... File vdsm/storage/hsm.py Line 1860: Line 1861: self.log.info("spUUID=%s master=%s", spUUID, masterDom) Line 1862: Line 1863: try: Line 1864: _ = self.getPool(spUUID) no assignment needed. Line 1865: except se.StoragePoolUnknown: Line 1866: pool = sp.StoragePool(spUUID, self.domainMonitor, self.taskMng) Line 1867: else: Line 1868: raise se.StoragePoolConnected(spUUID)
.................................................... File vdsm/storage/iscsi.py Line 428: else: Line 429: time.sleep(1) Line 430: else: Line 431: log.warning("Still waiting for scsi scan of hbas: %s", Line 432: tuple(hba for _ in processes)) I'm pretty sure there's a bigger bug here, and that this should be changed to
tuple(hba for (hba, proc) in processes) Line 433: Line 434: Line 435: def devIsiSCSI(dev): Line 436: hostdir = os.path.realpath(os.path.join("/sys/block", dev,
.................................................... File vdsm/storage/resourceManager.py Line 925: req = self.requests[fullName] Line 926: return req.wait(timeout) Line 927: Line 928: # req not found - check that it is not granted Line 929: for _ in self.resources: isn't this equivalent to
if self.resources: return True
? Please submit as a separate patch for Saggi's review. Line 930: return True Line 931: Line 932: # Note that there is a risk of another thread that is racing with us Line 933: # and releases this resource - but this should be synced above us
.................................................... File vdsm/vm.py Line 3057: try: Line 3058: self._dom.attachDevice(nicXml) Line 3059: except libvirt.libvirtError as e: Line 3060: self.log.error("Hotplug failed", exc_info=True) Line 3061: _ = hooks.after_nic_hotplug_fail( no assignment needed. Line 3062: nicXml, self.conf, params=customProps) Line 3063: if e.get_error_code() == libvirt.VIR_ERR_NO_DOMAIN: Line 3064: return errCode['noVM'] Line 3065: return {'status': {'code': errCode['hotplugNic']['status']['code'],
Antoni Segura Puimedon has posted comments on this change.
Change subject: cleanup: drop several unused local variables ......................................................................
Patch Set 1:
(8 comments)
Thanks a lot for the fast review Dan!
.................................................... File tests/functional/networkTests.py Line 376: NET_VLANS = [(NETWORK_NAME + str(index), str(index)) Line 377: for index in range(VLAN_COUNT)] Line 378: with dummyIf(1) as nics: Line 379: firstVlan, firstVlanId = NET_VLANS[0] Line 380: _ = self.vdsm_net.addNetwork(firstVlan, vlan=firstVlanId, Done Line 381: bond=BONDING_NAME, Line 382: nics=nics, opts=opts) Line 383: with nonChangingOperstate(BONDING_NAME): Line 384: for netVlan, vlanId in NET_VLANS[1:]:
.................................................... File tests/functional/virtTests.py Line 82: Line 83: def _genInitramfs(): Line 84: fd, path = tempfile.mkstemp() Line 85: cmd = [_mkinitrd.cmd, "-f", path, _kernelVer] Line 86: _ = execCmd(cmd, sudo=False) Done Line 87: os.chmod(path, 0o644) Line 88: return path Line 89: Line 90:
.................................................... File tests/miscTests.py Line 431: dstFd, dstPath = tempfile.mkstemp() Line 432: os.chmod(dstPath, 0o666) Line 433: Line 434: #Copy Line 435: _ = misc.ddWatchCopy(srcPath, dstPath, None, len(data)) Done Line 436: Line 437: #Get copied data Line 438: readData = open(dstPath).read() Line 439:
.................................................... File tests/vmTests.py Line 389: # Negative flow, unsupported value Line 390: driveInput.update({'shared': 'UNKNOWN-VALUE'}) Line 391: Line 392: with self.assertRaises(ValueError): Line 393: _ = vm.Drive({}, self.log, **driveInput) Done Line 394: Line 395: def testDriveXML(self): Line 396: SERIAL = '54-a672-23e5b495a9ea' Line 397: devConfs = [
.................................................... File vdsm/storage/hsm.py Line 1860: Line 1861: self.log.info("spUUID=%s master=%s", spUUID, masterDom) Line 1862: Line 1863: try: Line 1864: _ = self.getPool(spUUID) Done Line 1865: except se.StoragePoolUnknown: Line 1866: pool = sp.StoragePool(spUUID, self.domainMonitor, self.taskMng) Line 1867: else: Line 1868: raise se.StoragePoolConnected(spUUID)
.................................................... File vdsm/storage/iscsi.py Line 428: else: Line 429: time.sleep(1) Line 430: else: Line 431: log.warning("Still waiting for scsi scan of hbas: %s", Line 432: tuple(hba for _ in processes)) Done Line 433: Line 434: Line 435: def devIsiSCSI(dev): Line 436: hostdir = os.path.realpath(os.path.join("/sys/block", dev,
.................................................... File vdsm/storage/resourceManager.py Line 925: req = self.requests[fullName] Line 926: return req.wait(timeout) Line 927: Line 928: # req not found - check that it is not granted Line 929: for _ in self.resources: I have to say that in these modules I found more weird stuff that I didn't dare touch. I'll move them to a separate patch. Line 930: return True Line 931: Line 932: # Note that there is a risk of another thread that is racing with us Line 933: # and releases this resource - but this should be synced above us
.................................................... File vdsm/vm.py Line 3057: try: Line 3058: self._dom.attachDevice(nicXml) Line 3059: except libvirt.libvirtError as e: Line 3060: self.log.error("Hotplug failed", exc_info=True) Line 3061: _ = hooks.after_nic_hotplug_fail( Done Line 3062: nicXml, self.conf, params=customProps) Line 3063: if e.get_error_code() == libvirt.VIR_ERR_NO_DOMAIN: Line 3064: return errCode['noVM'] Line 3065: return {'status': {'code': errCode['hotplugNic']['status']['code'],
oVirt Jenkins CI Server has posted comments on this change.
Change subject: cleanup: drop several unused local variables ......................................................................
Patch Set 2: Verified-1
Build Failed
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/5075/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4271/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/5149/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_network_functional_tests/743/ : FAILURE
oVirt Jenkins CI Server has posted comments on this change.
Change subject: cleanup: drop several unused local variables ......................................................................
Patch Set 3: Verified-1
Build Failed
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/5077/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4273/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/5151/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_network_functional_tests/744/ : FAILURE
oVirt Jenkins CI Server has posted comments on this change.
Change subject: cleanup: drop several unused local variables ......................................................................
Patch Set 4: Code-Review-1 Verified-1
Build Failed
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/5086/ : UNSTABLE
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4282/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/5160/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_network_functional_tests/748/ : FAILURE
oVirt Jenkins CI Server has posted comments on this change.
Change subject: cleanup: drop several unused local variables ......................................................................
Patch Set 5: Verified-1
Build Failed
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/5087/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4283/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/5161/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_network_functional_tests/749/ : FAILURE
Dan Kenigsberg has posted comments on this change.
Change subject: cleanup: drop several unused local variables ......................................................................
Patch Set 5: Code-Review-1
(1 comment)
This patch is growing too quickly and produces too frequent risks. Could you slow down or break it to sub components?
.................................................... File vdsm/storage/blockVolume.py Line 602: def newVolumeLease(cls, metaId, sdUUID, volUUID): Line 603: cls.log.debug("Initializing volume lease volUUID=%s sdUUID=%s, " Line 604: "metaId=%s", volUUID, sdUUID, metaId) Line 605: dom = sdCache.produce(sdUUID) Line 606: mdSlot = metaId ? Line 607: Line 608: leasePath = dom.getLeasesFilePath() Line 609: leaseOffset = ((mdSlot + RESERVED_LEASES) Line 610: * dom.logBlkSize * sd.LEASE_BLOCKS)
Antoni Segura Puimedon has posted comments on this change.
Change subject: cleanup: drop several unused local variables ......................................................................
Patch Set 5:
(1 comment)
Will split it in smaller parts.
.................................................... File vdsm/storage/blockVolume.py Line 602: def newVolumeLease(cls, metaId, sdUUID, volUUID): Line 603: cls.log.debug("Initializing volume lease volUUID=%s sdUUID=%s, " Line 604: "metaId=%s", volUUID, sdUUID, metaId) Line 605: dom = sdCache.produce(sdUUID) Line 606: mdSlot = metaId Good catch, I misread it as: metaSdUUID = mdSlot = metaId Line 607: Line 608: leasePath = dom.getLeasesFilePath() Line 609: leaseOffset = ((mdSlot + RESERVED_LEASES) Line 610: * dom.logBlkSize * sd.LEASE_BLOCKS)
Nir Soffer has posted comments on this change.
Change subject: cleanup: drop several unused local variables ......................................................................
Patch Set 5: Code-Review-1
(33 comments)
Most of these changes do not improve the code.
Place where unused variables are define are obvious - good cleanup.
In places where tuples or lists are split to variables, such in: rc, out, err = execCmd(...)
I don't see much value in replacing unused variables with _. I prefer to see all the returned values even if they are not used now. When we need to add a log or print for debugging, it is nice to have the variable there.
In such places, if you need only rc or rc, out, using slice is cleaner:
rc, out = execCmd(...)[:2]
Although I suspect that all these places should have used err to raise an exception. So better leave them until we fix error handling.
I also don't see value in replacing temporary vars in for loops with _:
for _ in xrange(count): pass
Is not better then:
for n in xrange(count): pass
The fact that that n is not used is not very important, and the readability is worse.
I did not commented all places with these changes since the amount of changes was huge.
Must most worries me is that this patch changes lot of possibly untested code, and stupid error may change the behavior without being noticed in the review. I would fill more confident about such patch if all changed functions had 100% test coverage.
.................................................... File tests/functional/virtTests.py Line 84: fd, path = tempfile.mkstemp() Line 85: cmd = [_mkinitrd.cmd, "-f", path, _kernelVer] Line 86: rc, _, err = execCmd(cmd, sudo=False) Line 87: if rc: Line 88: raise SkipTest('Cannot generate the initramfs: %s' % err) We do not use out, but renaming it to _ does not help. Line 89: os.chmod(path, 0o644) Line 90: return path Line 91: Line 92:
.................................................... File tests/hookValidation.py Line 68: cookie_file = _createHookScript(hook_path, hook_name, hook_script) Line 69: Line 70: try: Line 71: kwargs['hook_cookiefile'] = cookie_file Line 72: output = test_function(*args, **kwargs) You can remove output completely if you return here:
return test_function(*args, **kwargs)
Not sure if this should be done in this patch. Line 73: finally: Line 74: if directory_existed: Line 75: os.unlink(cookie_file) Line 76: else:
.................................................... File tests/hooksTests.py Line 41: code = """#! /bin/bash Line 42: echo -n %s >> "$_hook_domxml" Line 43: """ Line 44: scripts = [tempfile.NamedTemporaryFile(dir=dirName, delete=False) Line 45: for _ in xrange(Q)] I don't think _ is better then n here. Line 46: scripts.sort(key=lambda f: f.name) Line 47: for n, script in enumerate(scripts): Line 48: script.write(code % n) Line 49: os.chmod(os.path.join(dirName, script.name), 0o775)
.................................................... File tests/miscTests.py Line 448: fd, path = tempfile.mkstemp() Line 449: Line 450: try: Line 451: for _ in xrange(repetitions): Line 452: os.write(fd, data) I don't see why _ is better then i. Line 453: self.assertEquals(os.stat(path).st_size, misc.MEGA) Line 454: except: Line 455: os.unlink(path) Line 456: raise
.................................................... File tests/testValidation.py Line 109: @wraps(f) Line 110: def wrapper(*args, **kwargs): Line 111: if not os.path.exists('/sys/module/dummy'): Line 112: cmd_modprobe = [modprobe.cmd, "dummy"] Line 113: rc, _, err = utils.execCmd(cmd_modprobe, sudo=True) I don't see why _ is better then out here. Line 114: if rc: Line 115: raise SkipTest('Failed to load the required dummy module: %s' % Line 116: err) Line 117: return f(*args, **kwargs)
.................................................... File vdsm/blkid.py Line 42: return self.message Line 43: Line 44: Line 45: def getDeviceByUuid(uuid): Line 46: rc, out, _ = utils.execCmd([constants.EXT_BLKID, '-U', uuid]) I don't see how _ is better then err here. Line 47: if rc: Line 48: raise BlockIdException(rc, uuid)
.................................................... File vdsm/configNetwork.py Line 423: Line 424: def _validateNetworkSetup(networks, bondings): Line 425: _netinfo = netinfo.NetInfo() Line 426: Line 427: for _, networkAttrs in networks.iteritems(): If you don't care about the keys, iterate on networks.values(). Line 428: if networkAttrs.get('remove', False): Line 429: if set(networkAttrs) - set(['remove']): Line 430: raise ConfigNetworkError(ne.ERR_BAD_PARAMS, 'Cannot specify ' Line 431: 'any attribute when removing')
.................................................... File vdsm/netconf/ifcfg.py Line 729: Line 730: Line 731: def ifdown(iface): Line 732: "Bring down an interface" Line 733: rc, _, _ = utils.execCmd([constants.EXT_IFDOWN, iface], raw=True) If you care only about rc, use:
utils.execCmd(...)[0]
This not only make the code more clear, but let Python collect out and err sooner. Line 734: return rc Line 735: Line 736: Line 737: def ifup(iface, async=False):
.................................................... File vdsm/storage/hsm.py Line 2271: # Get full iso files dictionary Line 2272: isodict = isoDom.getFileList(pattern='*.' + extension, Line 2273: caseSensitive=False) Line 2274: # Get list of iso images with proper permissions only Line 2275: isolist = [key for key in isodict if isodict[key]['status'] == 0] You need value for checking status, and iteritems may be more efficient. Also using names that describe the content of isodict is more clear than using generic key and value:
isolist = [name for name, stats in isodict.iteritems() if stats['status'] == 0] Line 2276: return {'isolist': isolist} Line 2277: Line 2278: @public Line 2279: def getFloppyList(self, spUUID, options=None):
.................................................... File vdsm/storage/iscsi.py Line 428: else: Line 429: time.sleep(1) Line 430: else: Line 431: log.warning("Still waiting for scsi scan of hbas: %s", Line 432: tuple(hba for (hba, process) in processes)) I think we can remove the parenthesis:
tuple(hba for hba, process in processes))
And if you really want to use _ for unused vars:
tuple(hba for hba, _ in processes)) Line 433: Line 434: Line 435: def devIsiSCSI(dev): Line 436: hostdir = os.path.realpath(os.path.join("/sys/block", dev,
.................................................... File vdsm/storage/iscsiadm.py Line 122: raise IscsiInterfaceUpdateError(name, rc, out, err) Line 123: Line 124: Line 125: def iface_delete(name): Line 126: rc, _, _ = _runCmd(["-m", "iface", "-I", name, "--op=delete"]) Use _runCmd(...)[0] Line 127: if rc == 0: Line 128: return Line 129: Line 130: if not iface_exists(name):
.................................................... File vdsm/storage/lvm.py Line 479: """ Line 480: Used only during bootstrap. Line 481: """ Line 482: cmd = list(LVS_CMD) Line 483: rc, out, _ = self.cmd(cmd) Maybe this:
rc, out = self.cmd(cmd)[:2] Line 484: if rc == 0: Line 485: updatedLVs = set() Line 486: for line in out: Line 487: fields = [field.strip() for field in line.split(SEPARATOR)]
Line 883: _initpvs(pvs, metadataSize, force) Line 884: # Activate the 1st PV metadata areas Line 885: cmd = ["pvchange", "--metadataignore", "n"] Line 886: cmd.append(pvs[0]) Line 887: rc, _, _ = _lvminfo.cmd(cmd, tuple(pvs)) Use _lvminfo.cmd(...)[0] Line 888: if rc != 0: Line 889: raise se.PhysDevInitializationError(pvs[0]) Line 890: Line 891: options = ["--physicalextentsize", extentsize]
Line 902: Line 903: Line 904: def removeVG(vgName): Line 905: cmd = ["vgremove", "-f", vgName] Line 906: rc, _, _ = _lvminfo.cmd(cmd, _lvminfo._getVGDevs((vgName, ))) Use _lvminfo.cmd(...)[0] Line 907: pvs = tuple(pvName for pvName, pv in _lvminfo._pvs.iteritems() Line 908: if not isinstance(pv, Stub) and pv.vg_name == vgName) Line 909: # PVS needs to be reloaded anyhow: if vg is removed they are staled, Line 910: # if vg remove failed, something must be wrong with devices and we want
Line 933: _initpvs(pvs, int(vg.vg_mda_size) / 2 ** 20, force) Line 934: Line 935: cmd = ["vgextend", vgName] + pvs Line 936: devs = tuple(_lvminfo._getVGDevs((vgName, )) + tuple(pvs)) Line 937: rc, _, _ = _lvminfo.cmd(cmd, devs) Use _lvminfo.cmd(...)[0] Line 938: if rc == 0: Line 939: _lvminfo._invalidatepvs(pvs) Line 940: _lvminfo._invalidatevgs(vgName) Line 941: log.debug("Cache after extending vg %s", _lvminfo._vgs)
Line 1026: cmd.extend(("--contiguous", cont, "--size", "%sm" % size)) Line 1027: if initialTag is not None: Line 1028: cmd.extend(("--addtag", initialTag)) Line 1029: cmd.extend(("--name", lvName, vgName)) Line 1030: rc, _, _ = _lvminfo.cmd(cmd, _lvminfo._getVGDevs((vgName, ))) Use _lvminfo.cmd(...)[0] Line 1031: Line 1032: if rc == 0: Line 1033: _lvminfo._invalidatevgs(vgName) Line 1034: _lvminfo._invalidatelvs(vgName, lvName)
Line 1069: cmd = ["lvremove", "-f"] Line 1070: cmd.extend(LVM_NOBACKUP) Line 1071: for lvName in lvNames: Line 1072: cmd.append("%s/%s" % (vgName, lvName)) Line 1073: rc, _, _ = _lvminfo.cmd(cmd, _lvminfo._getVGDevs((vgName, ))) Use _lvminfo.cmd(...)[0] Line 1074: if rc == 0: Line 1075: for lvName in lvNames: Line 1076: # Remove the LV from the cache Line 1077: _lvminfo._lvs.pop((vgName, lvName), None)
Line 1093: # (p)etabytes, (e)xabytes. Line 1094: # Capitalise to use multiples of 1000 (S.I.) instead of 1024. Line 1095: cmd = (op,) + LVM_NOBACKUP Line 1096: cmd += ("--size", "%sm" % (size,), "%s/%s" % (vgName, lvName)) Line 1097: rc, _, _ = _lvminfo.cmd(cmd, _lvminfo._getVGDevs((vgName, ))) Use _lvminfo.cmd(...)[0] Line 1098: if rc == 0: Line 1099: _lvminfo._invalidatevgs(vgName) Line 1100: _lvminfo._invalidatelvs(vgName, lvName) Line 1101:
Line 1140: Line 1141: Line 1142: def renameLV(vg, oldlv, newlv): Line 1143: cmd = ("lvrename",) + LVM_NOBACKUP + (vg, oldlv, newlv) Line 1144: rc, _, _ = _lvminfo.cmd(cmd, _lvminfo._getVGDevs((vg, ))) Use _lvminfo.cmd(...)[0] Line 1145: if rc != 0: Line 1146: raise se.LogicalVolumeRenameError("%s %s %s" % (vg, oldlv, newlv)) Line 1147: Line 1148: _lvminfo._lvs.pop((vg, oldlv), None)
Line 1151: Line 1152: def refreshLV(vgName, lvName): Line 1153: # If the logical volume is active, reload its metadata. Line 1154: cmd = ['lvchange', '--refresh', "%s/%s" % (vgName, lvName)] Line 1155: rc, _, _ = _lvminfo.cmd(cmd, _lvminfo._getVGDevs((vgName, ))) Use _lvminfo.cmd(...)[0] Line 1156: _lvminfo._invalidatelvs(vgName, lvName) Line 1157: if rc != 0: Line 1158: raise se.LogicalVolumeRefreshError("%s failed" % list2cmdline(cmd)) Line 1159:
Line 1162: # may be for all the LVs in the whole VG? Line 1163: def addtag(vg, lv, tag): Line 1164: lvname = "%s/%s" % (vg, lv) Line 1165: cmd = ("lvchange",) + LVM_NOBACKUP + ("--addtag", tag) + (lvname,) Line 1166: rc, _, _ = _lvminfo.cmd(cmd, _lvminfo._getVGDevs((vg, ))) Use _lvminfo.cmd(...)[0] Line 1167: _lvminfo._invalidatelvs(vg, lv) Line 1168: if rc != 0: Line 1169: # Fix me: should be se.ChangeLogicalVolumeError but this not exists. Line 1170: raise se.MissingTagOnLogicalVolume("%s/%s" % (vg, lv), tag)
Line 1254: Line 1255: def addVGTag(vgName, tag): Line 1256: _lvminfo._invalidatevgs(vgName) Line 1257: cmd = ["vgchange", "--addtag", tag, vgName] Line 1258: rc, _, _ = _lvminfo.cmd(cmd, _lvminfo._getVGDevs((vgName, ))) Use _lvminfo.cmd(...)[0] Line 1259: if rc != 0: Line 1260: raise se.VolumeGroupAddTagError("Failed adding tag %s to VG %s." % Line 1261: (tag, vgName)) Line 1262:
Line 1263: Line 1264: def remVGTag(vgName, tag): Line 1265: _lvminfo._invalidatevgs(vgName) Line 1266: cmd = ["vgchange", "--deltag", tag, vgName] Line 1267: rc, _, _ = _lvminfo.cmd(cmd, _lvminfo._getVGDevs((vgName, ))) Use _lvminfo.cmd(...)[0] Line 1268: if rc != 0: Line 1269: raise se.VolumeGroupRemoveTagError(vgName) Line 1270: Line 1271:
Line 1319: """ Line 1320: lvname = "%s/%s" % (vg, lv) Line 1321: cmd = (("lvchange",) + LVM_NOBACKUP + ("--deltag", deltag) + Line 1322: ("--addtag", addtag) + (lvname,)) Line 1323: rc, _, _ = _lvminfo.cmd(cmd, _lvminfo._getVGDevs((vg, ))) Use _lvminfo.cmd(...)[0] Line 1324: _lvminfo._invalidatelvs(vg, lv) Line 1325: if rc != 0: Line 1326: raise se.LogicalVolumeReplaceTagError("%s/%s" % (vg, lv),
.................................................... File vdsm/storage/multipath.py Line 212: "--whitelisted", Line 213: "--export", Line 214: "--replace-whitespace", Line 215: "--device=" + blkdev] Line 216: rc, out, _ = misc.execCmd(cmd, sudo=False) Maybe:
rc, out = misc.execCmd(...)[:2] Line 217: if rc == 0: Line 218: for line in out: Line 219: if line.startswith("ID_SERIAL="): Line 220: return line.split("=")[1]
.................................................... File vdsm/storage/storage_mailbox.py Line 274: self.start() Line 275: Line 276: def _initMailbox(self): Line 277: # Sync initial incoming mail state with storage view Line 278: rc, out, _ = _mboxExecCmd(self._inCmd, sudo=False, raw=True) rc, out = _mboxExecCmd(...)[:2] Line 279: if rc == 0: Line 280: self._incomingMail = out Line 281: self._init = True Line 282: else:
Line 371: Line 372: def _checkForMail(self): Line 373: #self.log.debug("HSM_MailMonitor - checking for mail") Line 374: #self.log.debug("Running command: " + str(self._inCmd)) Line 375: rc, in_mail, _ = misc.execCmd(self._inCmd, sudo=False, raw=True) rc, in_mail = misc.execCmd(...)[:2] Line 376: if rc: Line 377: raise RuntimeError("_handleResponses.Could not read mailbox - rc " Line 378: "%s" % rc) Line 379: if (len(in_mail) != MAILBOX_SIZE):
Line 574: # Clear outgoing mail Line 575: self.log.debug("SPM_MailMonitor - clearing outgoing mail, command is: " Line 576: "%s", self._outCmd) Line 577: cmd = self._outCmd + ['bs=' + str(self._outMailLen)] Line 578: rc, _, _ = _mboxExecCmd(cmd, sudo=False, data=self._outgoingMail) rc = _mboxExecCmd(...)[0] Line 579: if rc: Line 580: self.log.warning("SPM_MailMonitor couldn't clear outgoing mail, " Line 581: "dd failed") Line 582:
Line 729: #self.log.debug("SPM_MailMonitor -_checking for mail") Line 730: cmd = self._inCmd + ['bs=' + str(self._outMailLen)] Line 731: #self.log.debug("SPM_MailMonitor - reading incoming mail, " Line 732: # "command: " + str(cmd)) Line 733: rc, in_mail, _ = misc.execCmd(cmd, sudo=False, raw=True) rc, in_mail = ...[:2] Line 734: if rc: Line 735: raise IOError(errno.EIO, "_handleRequests._checkForMail - " Line 736: "Could not read mailbox: %s" % self._inbox) Line 737:
Line 772: cmd = self._outCmd + ['bs=' + str(MAILBOX_SIZE), Line 773: 'seek=' + str(mailboxOffset / MAILBOX_SIZE)] Line 774: #self.log.debug("Running command: %s, for message id: %s", Line 775: # str(cmd), str(msgID)) Line 776: rc, _, _ = _mboxExecCmd(cmd, sudo=False, data=mailbox) rc = ...[0] Line 777: if rc: Line 778: self.log.error("SPM_MailMonitor: sendReply - couldn't send " Line 779: "reply, dd failed") Line 780: finally:
.................................................... File vdsm/storage/volume.py Line 205: rc, _, _ = qemuRebase( Line 206: vol.getVolumePath(), vol.getFormat(), Line 207: os.path.join('..', srcImg, srcParent), Line 208: int(dstFormat), misc.parseBool(unsafe), Line 209: vars.task.aborting, False) rc = qemuRebase(...)[0] Line 210: if rc: Line 211: raise se.MergeVolumeRollbackError(srcVol) Line 212: Line 213: vol.setParent(srcParent)
Line 242: pvol.volUUID, str(True)])) Line 243: Line 244: rc, _, _ = qemuRebase(self.getVolumePath(), self.getFormat(), Line 245: backingVolPath, backingFormat, unsafe, Line 246: vars.task.aborting, rollback) rc = qemuRebase(...)[0] Line 247: if rc: Line 248: raise se.MergeSnapshotsError(self.volUUID) Line 249: self.setParent(backingVol) Line 250: self.recheckIfLeaf()
Line 1038: # +1 is so that odd numbers will round upwards. Line 1039: cmd += [volume, "%uK" % ((size + 1) / 2)] Line 1040: Line 1041: rc, out, _ = misc.execCmd(cmd, sudo=False, cwd=cwd, Line 1042: deathSignal=signal.SIGKILL) rc, out = misc.execCmd(...)[:2] Line 1043: if rc: Line 1044: raise se.VolumeCreationError(out) Line 1045: return True Line 1046:
Yaniv Bronhaim has posted comments on this change.
Change subject: cleanup: drop several unused local variables ......................................................................
Patch Set 5:
(3 comments)
.................................................... File lib/vdsm/netinfo.py Line 120: return (not isHiddenNic(device) and Line 121: not isEnslavedByHiddenBond(device)) Line 122: Line 123: res = [dev for dev in _nic_devices() if isManagedByVdsm(dev)] Line 124: return res if you already clean.. just:
return [dev for dev in _nic_devices() if isManagedByVdsm(dev)] Line 125: Line 126: Line 127: def bondings(): Line 128: """
.................................................... File tests/functional/networkTests.py Line 379: firstVlan, firstVlanId = NET_VLANS[0] Line 380: status, msg = self.vdsm_net.addNetwork(firstVlan, vlan=firstVlanId, Line 381: bond=BONDING_NAME, Line 382: nics=nics, opts=opts) Line 383: self.assertEquals(status, SUCCESS, msg) is it related to the cleanup? Line 384: with nonChangingOperstate(BONDING_NAME): Line 385: for netVlan, vlanId in NET_VLANS[1:]: Line 386: status, msg = self.vdsm_net.addNetwork(netVlan, Line 387: vlan=vlanId,
.................................................... File tests/functional/virtTests.py Line 84: fd, path = tempfile.mkstemp() Line 85: cmd = [_mkinitrd.cmd, "-f", path, _kernelVer] Line 86: rc, _, err = execCmd(cmd, sudo=False) Line 87: if rc: Line 88: raise SkipTest('Cannot generate the initramfs: %s' % err) this raise doesn't relate to the cleanup .. separate to tests fix and cleanup if you prefer. Line 89: os.chmod(path, 0o644) Line 90: return path Line 91: Line 92:
Saggi Mizrahi has posted comments on this change.
Change subject: cleanup: drop several unused local variables ......................................................................
Patch Set 5:
One of SaggiMizrahi's automated scripts discovered this patch might require his approval. Please wait until he had time to check it out.
Itamar Heim has posted comments on this change.
Change subject: cleanup: drop several unused local variables ......................................................................
Patch Set 5:
ping
Saggi Mizrahi has posted comments on this change.
Change subject: cleanup: drop several unused local variables ......................................................................
Patch Set 5:
Please split up to smaller pieces. Shouldn't be too hard.
Saggi Mizrahi has posted comments on this change.
Change subject: cleanup: drop several unused local variables ......................................................................
Patch Set 5: Code-Review-1
Antoni Segura Puimedon has posted comments on this change.
Change subject: cleanup: drop several unused local variables ......................................................................
Patch Set 5:
will do
Yaniv Bronhaim has posted comments on this change.
Change subject: cleanup: drop several unused local variables ......................................................................
Patch Set 5:
why to split it? what pieces saggi, how would you split it? its just a removal of redundant all-over project warnings. i less agree. agree with the rest of the comments.
Itamar Heim has posted comments on this change.
Change subject: cleanup: drop several unused local variables ......................................................................
Patch Set 5:
ping
Antoni Segura Puimedon has abandoned this change.
Change subject: cleanup: drop several unused local variables ......................................................................
Abandoned
A huge rework will be needed.
vdsm-patches@lists.fedorahosted.org
-
asegurap＠redhat.com -
Dan Kenigsberg -
iheim＠redhat.com -
Jenkins CI RO -
Nir Soffer -
smizrahi＠redhat.com -
ybronhei＠redhat.com