Saggi Mizrahi has posted comments on this change.
Change subject: [WIP] Add text-based console support
......................................................................
Patch Set 2: I would prefer that you didn't submit this
(19 inline comments)
Also, a thread per console is wasteful. Always use non-blocking pipes.
Use poll and have all connections multiplexed in the same thread.
Run pep8 on everything!
....................................................
File vdsm/API.py
Line 511: Used only by QA and might be discontinued in next version.
Line 512: """
Line 513: return errCode['noimpl']
Line 514:
Line 515: def setTicket(self, password, ttl, existingConnAction, params,
actOn):
update the docs
Also actOn should be something more descriptive.
'interface' maybe?
Line 516: """
Line 517: Set the ticket (password) to be used to connect to a VM
display
Line 518:
Line 519: :param vmId: specify the VM whos ticket is to be changed.
....................................................
File vdsm/consoleServer.py
Line 125: os.close(self._rpcServerWr)
Line 126: self._sock.close()
Line 127:
Line 128:
Line 129: class _ParamikoServerIf(paramiko.ServerInterface):
Please choose a more descriptive name
Line 130:
Line 131: def __init__(self):
Line 132: self._passwd = None
Line 133:
Line 130:
Line 131: def __init__(self):
Line 132: self._passwd = None
Line 133:
Line 134: def setPasswd(self, passwd):
Saving two chars isn't worth obfuscating the interface.
Line 135: self._passwd = passwd
Line 136:
Line 137: def clearPasswd(self):
Line 138: self._passwd = None
Line 134: def setPasswd(self, passwd):
Line 135: self._passwd = passwd
Line 136:
Line 137: def clearPasswd(self):
Line 138: self._passwd = None
'clear' Seems redundant.
Line 139:
Line 140: def check_channel_request(self, kind, chanid):
Line 141: if kind == 'session':
Line 142: return paramiko.OPEN_SUCCEEDED
Line 165: def fileno(self):
Line 166: return self._fd
Line 167:
Line 168:
Line 169: class _ClientThread(threading.Thread):
Never inherit from thread. It makes no sense.
You are not making a specific type of thread.
Line 170:
Line 171: def __init__(self, logger, pty, clientSock, hostKey, serverIf,
addr):
Line 172: threading.Thread.__init__(self)
Line 173: self._logger = logger
Line 169: class _ClientThread(threading.Thread):
Line 170:
Line 171: def __init__(self, logger, pty, clientSock, hostKey, serverIf,
addr):
Line 172: threading.Thread.__init__(self)
Line 173: self._logger = logger
We generally use self._log not self._logger
Line 174: self._pty = pty
Line 175: self._clientSock = clientSock
Line 176: self._hostKey = hostKey
Line 177: self._serverIf = serverIf
Line 176: self._hostKey = hostKey
Line 177: self._serverIf = serverIf
Line 178: self._addr = addr
Line 179: self._isTerminated = False
Line 180: self._pipeRd, self._pipeWr = os.pipe()
Don't forget to add a desctructor to clear these.
Line 181: misc.setNonBlocking(self._pipeRd)
Line 182: misc.setNonBlocking(self._pipeWr)
Line 183:
Line 184: def fileno(self):
Line 188: def send(self, data):
Line 189: try:
Line 190: os.write(self._pipeWr, data)
Line 191: except OSError as e:
Line 192: if e.errno not in (errno.EAGAIN, errno.EINTR):
retry?
The use will assume the method succeeded even though it failed
Line 193: raise
Line 194:
Line 195: def terminate(self):
Line 196: self._logger.info("terminate client thread: %s", self._addr)
Line 209: if chan == None:
Line 210: self._logger.error("get SSH channel failed")
Line 211: return
Line 212:
Line 213: pipeRdFdw = _FdWraper(self._pipeRd)
Seems redundant. Everything in python accepts a raw fd
Line 214:
Line 215: while True:
Line 216: rd, wr, er = select.select([chan, pipeRdFdw], [], [])
Line 217:
Line 212:
Line 213: pipeRdFdw = _FdWraper(self._pipeRd)
Line 214:
Line 215: while True:
Line 216: rd, wr, er = select.select([chan, pipeRdFdw], [], [])
never use select. It's broken (doesn't support FDs above 1024) and slow
Use poll or epoll
Line 217:
Line 218: if self._isTerminated:
Line 219: break
Line 220:
Line 218: if self._isTerminated:
Line 219: break
Line 220:
Line 221: if chan in rd:
Line 222: buf = chan.recv(4096)
self._bufferSize
Also, change the FD to non-blocking if you are using a select loop.
Line 223:
Line 224: if len(buf) == 0:
Line 225: self._logger.info("ssh channel was closed %s",
Line 226: self._addr)
Line 225: self._logger.info("ssh channel was closed %s",
Line 226: self._addr)
Line 227: break
Line 228:
Line 229: os.write(self._pty.fileno(), buf)
When you change to non-blocking make sure everything gets written
Line 230: elif pipeRdFdw in rd:
Line 231: try:
Line 232: buf = os.read(self._pipeRd, 4096)
Line 233: if len(buf) == 0:
Line 228:
Line 229: os.write(self._pty.fileno(), buf)
Line 230: elif pipeRdFdw in rd:
Line 231: try:
Line 232: buf = os.read(self._pipeRd, 4096)
again
Line 233: if len(buf) == 0:
Line 234: raise IOError(errno.EIO, "pipe closed")
Line 235: except OSError as e:
Line 236: if e.errno not in (errno.EAGAIN, errno.EINTR):
Line 229: os.write(self._pty.fileno(), buf)
Line 230: elif pipeRdFdw in rd:
Line 231: try:
Line 232: buf = os.read(self._pipeRd, 4096)
Line 233: if len(buf) == 0:
Doesn't belong inside the try block
Line 234: raise IOError(errno.EIO, "pipe closed")
Line 235: except OSError as e:
Line 236: if e.errno not in (errno.EAGAIN, errno.EINTR):
Line 237: self._logger.error("PIPE read error",
exc_info=True)
Line 232: buf = os.read(self._pipeRd, 4096)
Line 233: if len(buf) == 0:
Line 234: raise IOError(errno.EIO, "pipe closed")
Line 235: except OSError as e:
Line 236: if e.errno not in (errno.EAGAIN, errno.EINTR):
These (espscially EAGAIN) pop up when the system is stressed. No need to freak
out and break. retry
Line 237: self._logger.error("PIPE read error",
exc_info=True)
Line 238: break
Line 239:
Line 240: if chan.send(buf) == 0:
Line 251:
Line 252: class SSHConsoleServer(object):
Line 253:
Line 254: def __init__(self, logger, pty, sock, rpcRd, rpcWr, hostKeyFile):
Line 255: self._logger = logger
_log
Line 256: self._pty = _FdWraper(pty)
Line 257: self._sock = sock
Line 258: self._rpcRd = rpcRd
Line 259: self._rpcWr = rpcWr
Line 270: # if 0 returned means input was closed
Line 271: buf = os.read(self._pty.fileno(), 4096)
Line 272:
Line 273: if len(buf) == 0:
Line 274: raise IOError(errno.EIO, "console was closed")
When a file is closed the errno is EBADF
Line 275:
Line 276: for client in self._clients:
Line 277: client.send(buf)
Line 278:
Line 284: self._serverIf,
Line 285: addr)
Line 286:
Line 287: self._clients.append(newClient)
Line 288: newClient.start()
setDaemon(True)
Line 289:
Line 290: def _closeClient(self, client):
Line 291: client.terminate()
Line 292: client.join()
Line 327: rpcRd = _FdWraper(self._rpcRd)
Line 328:
Line 329: try:
Line 330: while True:
Line 331: rd, wr, er = select.select([self._sock,
Too much indentation
select is evil
Line 332: self._pty,
Line 333: rpcRd] + self._clients,
Line 334: [], [], self._timeout)
Line 335:
--
To view, visit http://gerrit.ovirt.org/7165
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I69904bf7aafd4f764a256d4075c9bf71d988e7c5
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Xu He Jie <[email protected]>
Gerrit-Reviewer: Adam Litke <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Mark Wu <[email protected]>
Gerrit-Reviewer: Royce Lv <[email protected]>
Gerrit-Reviewer: Ryan Harper <[email protected]>
Gerrit-Reviewer: Saggi Mizrahi <[email protected]>
Gerrit-Reviewer: ShaoHe Feng <[email protected]>
Gerrit-Reviewer: Xu He Jie <[email protected]>
Gerrit-Reviewer: Zhou Zheng Sheng <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
