Ayal Baron has posted comments on this change.
Change subject: BZ#854151 - Fix disconnect iscsi connections.
......................................................................
Patch Set 1: I would prefer that you didn't submit this
(11 inline comments)
....................................................
File vdsm/storage/hsm.py
Line 2035: except Exception as err:
Line 2036: self.log.error("Could not disconnect from
storageServer", exc_info=True)
Line 2037: status, _ = self._translateConnectionError(err)
Line 2038: else:
Line 2039: status = 0
there is no functional difference here right? just your style preference?
Line 2040:
Line 2041: res.append({'id': conDef["id"], 'status': status})
Line 2042:
Line 2043: # Disconnecting a device may change the visible storage
domain list
....................................................
File vdsm/storage/storageServer.py
Line 281: def __hash__(self):
Line 282: return hash(type(self)) ^ hash(self._mountCon)
Line 283:
Line 284: class IscsiConnection(object):
Line 285: log = logging.getLogger("StorageServer.IscsiConnection")
empty line to separate?
Line 286: @property
Line 287: def target(self):
Line 288: return self._target
Line 289:
Line 316: try:
Line 317: ip = socket.gethostbyname(host)
Line 318: except socket.gaierror:
Line 319: return False
Line 320: else:
again, looks like there is no functional difference here, just extra code.
Line 321: if ip != portal.hostname:
Line 322: return False
Line 323:
Line 324: elif self._target.portal.port != portal.port:
Line 320: else:
Line 321: if ip != portal.hostname:
Line 322: return False
Line 323:
Line 324: elif self._target.portal.port != portal.port:
this elif seems wrong.
If the hostname is not identical we check IP and if it *is* identical then
session can still be the same.
Line 325: return False
Line 326:
Line 327: elif self._target.iqn != target.iqn:
Line 328: return False
Line 323:
Line 324: elif self._target.portal.port != portal.port:
Line 325: return False
Line 326:
Line 327: elif self._target.iqn != target.iqn:
tpgt is of no interest?
Line 328: return False
Line 329:
Line 330: elif self._iface.name != iface.name:
Line 331: return False
Line 344: self.log.warning("Can't get session info,
self._lastSessionId %s", self._lastSessionId)
Line 345: else:
Line 346: sessions = chain(info, sessions)
Line 347:
Line 348: self.log.debug("self._target: %s", self._target)
logging self._target here seems out of place and devoid of context
Line 349: for session in sessions:
Line 350: self.log.debug("session %s", session)
Line 351: if self.isSession(session):
Line 352: self._lastSessionId = session.id
Line 346: sessions = chain(info, sessions)
Line 347:
Line 348: self.log.debug("self._target: %s", self._target)
Line 349: for session in sessions:
Line 350: self.log.debug("session %s", session)
doesn't this flood the log? maybe put it in trace level?
Line 351: if self.isSession(session):
Line 352: self._lastSessionId = session.id
Line 353: self.log.debug("self._lastSessionId: %s",
self._lastSessionId)
Line 354: return session
Line 349: for session in sessions:
Line 350: self.log.debug("session %s", session)
Line 351: if self.isSession(session):
Line 352: self._lastSessionId = session.id
Line 353: self.log.debug("self._lastSessionId: %s",
self._lastSessionId)
again, devoid of context, plus seems redundant. Again, consider moving to
trace.
Line 354: return session
Line 355: else:
Line 356: raise OSError(errno.ENOENT, "Session not found in
sessions: %s", sessions)
Line 357:
Line 352: self._lastSessionId = session.id
Line 353: self.log.debug("self._lastSessionId: %s",
self._lastSessionId)
Line 354: return session
Line 355: else:
Line 356: raise OSError(errno.ENOENT, "Session not found in
sessions: %s", sessions)
which session wasn't found?
Line 357:
Line 358: def isConnected(self):
Line 359: try:
Line 360: self.getSessionInfo()
Line 364: return False
Line 365: raise
Line 366:
Line 367: def disconnect(self):
Line 368: self.log.debug("Iscsi disconnect called.")
again Trace at most
Line 369: try:
Line 370: sid = self.getSessionInfo().id
Line 371: except OSError, e:
Line 372: if e.errno == errno.ENOENT:
Line 374: return
Line 375: self.log.warning("Cant get iscsi session id, errno: %s",
e.errno)
Line 376: raise
Line 377:
Line 378: self.log.debug("Disconnecting iscsi session id: %s", sid)
again Trace at most
Line 379: iscsi.disconnectiScsiSession(sid)
Line 380:
Line 381: def __eq__(self, other):
Line 382: if not isinstance(other, IscsiConnection):
--
To view, visit http://gerrit.ovirt.org/8275
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I82f8680a3297da2ab8b766bfb7df9bde1cd20533
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo <[email protected]>
Gerrit-Reviewer: Ayal Baron <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Eduardo <[email protected]>
Gerrit-Reviewer: Yeela Kaplan <[email protected]>
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
