Change in vdsm[master]: [WIP] Add text-based console support
wudxw at linux.vnet.ibm.com
wudxw at linux.vnet.ibm.com
Tue Sep 4 03:44:23 UTC 2012
Mark Wu has posted comments on this change.
Change subject: [WIP] Add text-based console support
......................................................................
Patch Set 2: (15 inline comments)
Some small suggestions. I didn't do a full review on the consoleServer implementation because you're going to change it to the way of fix port. I also think the idea is cooler than the current one. Look forward to your next patch!
....................................................
File vdsm/consoleServer.py
Line 26: import select
Line 27: import os
Line 28: import threading
Line 29: import errno
Line 30:
intentional blank line?
Line 31: import paramiko
Line 32:
Line 33: from vdsm import constants
Line 34: from vdsm.betterPopen import BetterPopen
Line 28: import threading
Line 29: import errno
Line 30:
Line 31: import paramiko
Line 32:
and here
Line 33: from vdsm import constants
Line 34: from vdsm.betterPopen import BetterPopen
Line 35: from storage import misc
Line 36: from storage.remoteFileHandler import CrabRPCProxy, CrabRPCServer
Line 76: sock.close()
Line 77: continue
Line 78: return p, sock
Line 79:
Line 80: raise Exception("can not allocated any port on %s", self._addr)
You should never raise a general exception. I learn it from Adam :)
Line 81:
Line 82: def setTicket(self, passwd, timeout, connAct):
Line 83: try:
Line 84: return self._crabRPCProxy.callCrabRPCFunction(10, "setTicket",
Line 107: str(self._rpcProxyRd),
Line 108: str(self._rpcServerWr),
Line 109: self._hostKeyFile],
Line 110: close_fds=False, env=env)
Line 111: finally:
No error handling for BetterPopen failure?
Line 112: pty.close()
Line 113:
Line 114: def isOpen(self):
Line 115: return self._prog != None and self._prog.poll() == None
Line 111: finally:
Line 112: pty.close()
Line 113:
Line 114: def isOpen(self):
Line 115: return self._prog != None and self._prog.poll() == None
I don't understand what the poll function checks here.
Line 116:
Line 117: def close(self):
Line 118: if self.isOpen():
Line 119: self._prog.terminate()
....................................................
File vdsm/constants.py.in
Line 170: #
Line 171: # ssh console server port constants
Line 172: #
Line 173: SSH_CONSOLE_MAX_PORT = 65535
Line 174: SSH_CONSOLE_MIN_PORT = 12200
it's not welcomed to put the definition here because it's only used by console server. Probably you will remove it in your next implementation. Just let you know.
....................................................
File vdsm/libvirtvm.py
Line 1283: self.guestAgent = guestIF.GuestAgent(self._guestSocketFile,
Line 1284: self.cif.channelListener, self.log,
Line 1285: connect=utils.tobool(self.conf.get('vmchannel', 'true')))
Line 1286:
Line 1287: if utils.tobool(self.conf.get("enableConsole", False)):
is 'textConsole' better than 'enableConsole' ?
Line 1288: self.log.info("vm console enabled")
Line 1289:
Line 1290: supervdsm.getProxy().changePtyOwner(
Line 1291: int(os.path.basename(self.conf["consolePath"])))
Line 1989: hooks.before_vm_set_ticket(self._lastXMLDesc, self.conf, params)
Line 1990:
Line 1991: if actOn == "graphics":
Line 1992: graphics = xml.dom.minidom.parseString(self._dom.XMLDesc(0)) \
Line 1993: .childNodes[0].getElementsByTagName('graphics')[0]
Why change _domParseStr to xml.dom.minidom.parseString? Is it intentional?
Line 1994: graphics.setAttribute('passwd', otp)
Line 1995: if int(seconds) > 0:
Line 1996: validto = time.strftime('%Y-%m-%dT%H:%M:%S',
Line 1997: time.gmtime(time.time() + float(seconds)))
Line 2007: return {'status':
Line 2008: {'code': errCode['ticketErr']['status']['code'],
Line 2009: 'message': 'setTicket failed on console'}}
Line 2010: else:
Line 2011: self.log.error("console was disable")
was disabled. Maybe 'was not enabled' is better.
Line 2012: return {'status': {'code':
Line 2013: errCode['ticketErr']['status']['code'],
Line 2014: 'message': 'console was disable'}}
Line 2015: else:
Line 2010: else:
Line 2011: self.log.error("console was disable")
Line 2012: return {'status': {'code':
Line 2013: errCode['ticketErr']['status']['code'],
Line 2014: 'message': 'console was disable'}}
the same problem as line 2011
Line 2015: else:
Line 2016: self.log.error("unsupported actOn argument: %s", actOn)
Line 2017: return {'status': {'code': errCode['ticketErr']['status']['code'],
Line 2018: 'message': 'the value of actOn was unsupported'}}
Line 2014: 'message': 'console was disable'}}
Line 2015: else:
Line 2016: self.log.error("unsupported actOn argument: %s", actOn)
Line 2017: return {'status': {'code': errCode['ticketErr']['status']['code'],
Line 2018: 'message': 'the value of actOn was unsupported'}}
'actOn' type %s was unsupported' % actOn
more informative?
Line 2019:
Line 2020: hooks.after_vm_set_ticket(self._lastXMLDesc, self.conf, params)
Line 2021: return {'status': doneCode}
Line 2022:
Line 2309: def _getUnderlyingConsoleDeviceInfo(self):
Line 2310: """
Line 2311: Obtain console device info from libvirt
Line 2312: """
Line 2313: consolexml = xml.dom.minidom.parseString(self._lastXMLDesc) \
The same problem as line 1993.
Line 2314: .childNodes[0].getElementsByTagName('devices')[0] \
Line 2315: .getElementsByTagName('console')[0]
Line 2316:
Line 2317: self.conf["consolePath"] = str(consolexml.getElementsByTagName(
....................................................
File vdsm/Makefile.am
Line 30: blkid.py \
Line 31: caps.py \
Line 32: clientIF.py \
Line 33: configNetwork.py \
Line 34: consoleServer.py \
please also add it to pep8 whitelist
Line 35: debugPluginClient.py \
Line 36: guestIF.py \
Line 37: hooking.py \
Line 38: hooks.py \
....................................................
File vdsm/supervdsmServer.py
Line 282: def changePtyOwner(self, ptyNum):
Line 283: if type(ptyNum) != int:
Line 284: raise TypeError("ptyNum must be integer")
Line 285:
Line 286: cmd = [EXT_CHOWN, "vdsm:kvm", "/dev/pts/" + str(ptyNum)]
Use the macro VDSM_USER and VDSM_GROUP instead?
Line 287: rc, out, err = storage.misc.execCmd(cmd, sudo=False)
Line 288: if rc:
Line 289: raise OSError(errno.EINVAL, "can not change own of /dev/pts/%d, " \
Line 290: "out: %s\nerr: %s" %
Line 285:
Line 286: cmd = [EXT_CHOWN, "vdsm:kvm", "/dev/pts/" + str(ptyNum)]
Line 287: rc, out, err = storage.misc.execCmd(cmd, sudo=False)
Line 288: if rc:
Line 289: raise OSError(errno.EINVAL, "can not change own of /dev/pts/%d, " \
EINVAL could not reflect all cases, how about using rc as error code?
Please change 'own' to 'owner'
Line 290: "out: %s\nerr: %s" %
Line 291: (ptyNum, out, err))
Line 292:
Line 293:
--
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 <xuhj at linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl at us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw at linux.vnet.ibm.com>
Gerrit-Reviewer: Royce Lv <lvroyce at linux.vnet.ibm.com>
Gerrit-Reviewer: Ryan Harper <ryanh at us.ibm.com>
Gerrit-Reviewer: ShaoHe Feng <shaohef at linux.vnet.ibm.com>
Gerrit-Reviewer: Xu He Jie <xuhj at linux.vnet.ibm.com>
Gerrit-Reviewer: Zhou Zheng Sheng <zhshzhou at linux.vnet.ibm.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
More information about the vdsm-patches
mailing list