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:
--
To view, visit
http://gerrit.ovirt.org/20535
To unsubscribe, visit
http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib81c292f900154819e8852c21ae389c323034999
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes