Change in vdsm[master]: cleanup: drop several unused local variables

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(a)redhat.com&gt;
---
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':


-- 
To view, visit http://gerrit.ovirt.org/20535
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib81c292f900154819e8852c21ae389c323034999
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;

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.

-- 
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&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

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: 


-- 
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&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

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)


-- 
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&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

[Dan Kenigsberg]

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)


-- 
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&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

[Jenkins CI RO]

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

-- 
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: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

[Jenkins CI RO]

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

-- 
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: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

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'],


-- 
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: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

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.

-- 
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&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Itamar Heim <iheim(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Antoni Segura Puimedon has posted comments on this change.

Change subject: cleanup: drop several unused local variables
......................................................................


Patch Set 5:

will do

-- 
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&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Itamar Heim <iheim(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

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.

-- 
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&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Itamar Heim <iheim(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Saggi Mizrahi has posted comments on this change.

Change subject: cleanup: drop several unused local variables
......................................................................


Patch Set 5: Code-Review-1

-- 
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&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Itamar Heim <iheim(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Itamar Heim has posted comments on this change.

Change subject: cleanup: drop several unused local variables
......................................................................


Patch Set 5:

ping

-- 
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&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Itamar Heim <iheim(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

[Jenkins CI RO]

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

-- 
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: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

[Dan Kenigsberg]

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'],


-- 
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: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

[Jenkins CI RO]

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

-- 
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: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

[Jenkins CI RO]

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

-- 
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&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

[Nir Soffer]

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&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

Antoni Segura Puimedon has abandoned this change.

Change subject: cleanup: drop several unused local variables
......................................................................


Abandoned

A huge rework will be needed.

-- 
To view, visit http://gerrit.ovirt.org/20535
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: abandon
Gerrit-Change-Id: Ib81c292f900154819e8852c21ae389c323034999
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server

Itamar Heim has posted comments on this change.

Change subject: cleanup: drop several unused local variables
......................................................................


Patch Set 5:

ping

-- 
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&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Itamar Heim <iheim(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No