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