Alon Bar-Lev has posted comments on this change.
Change subject: engine: [WIP] Integrate noVNC support
......................................................................
Patch Set 5: (18 inline comments)
....................................................
File packaging/services/ovirt-websocket-proxy.py
Line 21: import websockify as wsproxy
Line 22: import base64
Line 23: import json
Line 24: _ = lambda m: gettext.dgettext(message=m, domain='ovirt-engine')
Line 25:
two spaces
Line 26: import config
Line 27: import service
Line 28:
Line 29: from M2Crypto import X509
Line 24: _ = lambda m: gettext.dgettext(message=m, domain='ovirt-engine')
Line 25:
Line 26: import config
Line 27: import service
Line 28:
two spaces
Line 29: from M2Crypto import X509
Line 30:
Line 31: '''
Line 32: Websocket proxy for usage with oVirt engine.
Line 26: import config
Line 27: import service
Line 28:
Line 29: from M2Crypto import X509
Line 30:
two spaces
Line 31: '''
Line 32: Websocket proxy for usage with oVirt engine.
Line 33: Leverages wsproxy.py by Joel Martin
Line 34: '''
Line 27: import service
Line 28:
Line 29: from M2Crypto import X509
Line 30:
Line 31: '''
comment should be within the class...
class xxx():
"""comment"""
Line 32: Websocket proxy for usage with oVirt engine.
Line 33: Leverages wsproxy.py by Joel Martin
Line 34: '''
Line 35: class OvirtWebSocketProxy(wsproxy.WebSocketProxy):
Line 34: '''
Line 35: class OvirtWebSocketProxy(wsproxy.WebSocketProxy):
Line 36: def __init__(self, *args, **kwargs):
Line 37: self.force_data_verification =
kwargs.pop('force_data_verification')
Line 38: wsproxy.WebSocketProxy.__init__(self, *args, **kwargs)
use super?
Line 39:
Line 40: def new_client(self):
Line 41: """
Line 42: Called after a new WebSocket connection has been established.
Line 41: """
Line 42: Called after a new WebSocket connection has been established.
Line 43: """
Line 44:
Line 45: cookie = Cookie.SimpleCookie()
why do we need cookie?
Line 46: cookie.load(self.headers.getheader('cookie'))
Line 47:
Line 48: connection_data = cookie['connection_data'].value.split(',');
Line 49: signature = cookie['signature'].value
Line 50: target_host = connection_data[1]
Line 51: target_port = connection_data[2]
Line 52: ticket = connection_data[0]
Line 53:
Line 54: if (self.force_data_verification and connection_data !=
ticketDecoder.decode(signature)):
why don't you extract the data out of the ticket?
You should always use single data source and not duplicate.
Line 55: raise Exception('VNC data verification failed.')
Line 56:
Line 57: # Connect to the target
Line 58: self.msg("connecting to: %s:%s" % (
Line 60: tsock = self.socket(target_host, target_port,
Line 61: connect=True)
Line 62:
Line 63: if self.verbose and not self.daemon:
Line 64: print(self.traffic_legend)
Print use the self.logger.
Line 65:
Line 66: # Start proxying
Line 67: try:
Line 68: self.do_proxy(tsock)
Line 63: if self.verbose and not self.daemon:
Line 64: print(self.traffic_legend)
Line 65:
Line 66: # Start proxying
Line 67: try:
I do not follow, how can multiple connection exist in such configuration?
Line 68: self.do_proxy(tsock)
Line 69: except:
Line 70: if tsock:
Line 71: tsock.shutdown(socket.SHUT_RDWR)
Line 75:
Line 76:
Line 77: class TicketDecoder(object):
Line 78:
Line 79: def __init__(self, certificate):
Add a parameter of insecure= to allow working without certificate and return
the decoded data as-is without signature validation.
Line 80: self._key = X509.load_cert(
Line 81: certificate,
Line 82: X509.FORMAT_PEM,
Line 83: ).get_pubkey()
Line 141: self._checkInstallation(
Line 142: pidfile=self.pidfile,
Line 143: )
Line 144:
Line 145: self._executable = 'ovirt-websocket-proxy'
no need.
Line 146:
Line 147: self._args = []
Line 148:
Line 149: def daemonContext(self):
Line 143: )
Line 144:
Line 145: self._executable = 'ovirt-websocket-proxy'
Line 146:
Line 147: self._args = []
no need.
Line 148:
Line 149: def daemonContext(self):
Line 150: try:
Line 151: log = self._config.getBoolean('LOG_FILE')
Line 147: self._args = []
Line 148:
Line 149: def daemonContext(self):
Line 150: try:
Line 151: log = self._config.getBoolean('LOG_FILE')
indent is wrong.
Line 152:
Line 153: force_data_verification =
self._config.getBoolean('FORCE_DATA_VERIFICATION')
Line 154:
Line 155: ticketDecoder = None
Line 153: force_data_verification =
self._config.getBoolean('FORCE_DATA_VERIFICATION')
Line 154:
Line 155: ticketDecoder = None
Line 156: if (force_data_verification):
Line 157: ticketDecoder =
TicketDecoder(self._config.getString('CERT_FOR_DATA_VERIFICATION'))
see above comment regarding insecure.
Line 158:
Line 159: server = OvirtWebSocketProxy(
Line 160:
listen_host=self._config.getString('PROXY_HOST'),
Line 161:
listen_port=self._config.getString('PROXY_PORT'),
Line 156: if (force_data_verification):
Line 157: ticketDecoder =
TicketDecoder(self._config.getString('CERT_FOR_DATA_VERIFICATION'))
Line 158:
Line 159: server = OvirtWebSocketProxy(
Line 160:
listen_host=self._config.getString('PROXY_HOST'),
indent is bad.
Line 161:
listen_port=self._config.getString('PROXY_PORT'),
Line 162:
source_is_ipv6=self._config.getBoolean('SOURCE_IS_IPV6'),
Line 163: verbose=self._config.getBoolean('VERBOSE'),
Line 164:
force_data_verification=force_data_verification,
Line 170: web=None,
Line 171: target_host='ignore',
Line 172: target_port='ignore',
Line 173: wrap_mode='exit',
Line 174: wrap_cmd=None)
please use:
f(
p1,
p2,
)
convention.
Line 175:
Line 176: server.start_server()
Line 177: except Exception as inst:
Line 178: print inst
Line 172: target_port='ignore',
Line 173: wrap_mode='exit',
Line 174: wrap_cmd=None)
Line 175:
Line 176: server.start_server()
you can:
OvirtWebSocketProxy(
...
...
...
).start_server()
No need for the temp variable...
Line 177: except Exception as inst:
Line 178: print inst
Line 179:
Line 180:
Line 174: wrap_cmd=None)
Line 175:
Line 176: server.start_server()
Line 177: except Exception as inst:
Line 178: print inst
No need to try/except, the daemon will handle that properly (exit, log etc)
Line 179:
Line 180:
Line 181: if __name__ == '__main__':
Line 182: service.setupLogger()
--
To view, visit http://gerrit.ovirt.org/13931
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I44e9870b88537360a1886e89c08f18865eae2ef0
Gerrit-PatchSet: 5
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Frank Kobzik <fkob...@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com>
Gerrit-Reviewer: Barak Azulay <bazu...@redhat.com>
Gerrit-Reviewer: Frank Kobzik <fkob...@redhat.com>
Gerrit-Reviewer: Itamar Heim <ih...@redhat.com>
Gerrit-Reviewer: Martin Beták <mbe...@redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skriva...@redhat.com>
Gerrit-Reviewer: Tomas Jelinek <tjeli...@redhat.com>
Gerrit-Reviewer: Vojtech Szocs <vsz...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
