Alon Bar-Lev has posted comments on this change.
Change subject: engine: Integrate noVNC support
......................................................................
Patch Set 6: (13 inline comments)
....................................................
File backend/manager/modules/root/src/main/webapp/ovirt-engine-novnc-main.html
Line 128: 'shared':
WebUtil.getQueryVar('shared', true),
Line 129: 'view_only':
WebUtil.getQueryVar('view_only', false),
Line 130: 'updateState': updateState,
Line 131: 'onPasswordRequired': passwordRequired});
Line 132: rfb.connect(host, port, ticket, path);
ok... now I understand host and port are of proxy, this is somewhat strange to
me... as I expected this framework to work with URLs... but OK.
Line 133: }catch(e) {alert(e);}
Line 134: }
Line 135:
Line 136: if (window.addEventListener) {
....................................................
File
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/uicommon/NoVncImpl.java
Line 36: this.listensOnPostMessage = listensOnPostMessage;
Line 37: }
Line 38:
Line 39: private String getClientUrl() {
Line 40: String val = Location.getProtocol() + "//" +
Location.getHost() + "/ovirt-engine-novnc-main.html?host=" + getProxyHost() +
"&port=" + getProxyPort();//$NON-NLS-1$//$NON-NLS-2$//$NON-NLS-3$
Hmm.... /ovirt-engine-novnc-main.html will do the same work, no?
Line 41: return val;
Line 42: }
Line 43:
Line 44: public NoVncImpl() {
....................................................
File packaging/conf/engine.conf.defaults.in
Line 33: ENGINE_DOC="@ENGINE_DOC@"
Line 34: ENGINE_VAR="@ENGINE_VAR@"
Line 35: ENGINE_CACHE="@ENGINE_CACHE@"
Line 36: SPICE_DIR="@SPICE_DIR@"
Line 37: ENGINE_NOVNC_DIR="@ENGINE_NOVNC_DIR@"
Oh... I think you can safely hardcode this here.
Line 38:
Line 39: #
Line 40: # Intervals for stoping the engine:
Line 41: #
....................................................
File packaging/conf/ovirt-engine-proxy.conf.in
Line 24: <Location /ovirt-engine/>
Line 25: ProxyPass ajp://localhost:@JBOSS_AJP_PORT@/
Line 26: </Location>
Line 27:
Line 28: <LocationMatch
^/(UserPortal($|/)|OvirtEngineWeb($|/)|webadmin($|/)|docs($|/)|ovirt-engine-novnc($|/)|spice/|ca.crt$|engine.ssh.key.txt$|rhevm.ssh.key.txt$|ovirt-engine-style.css$|console.vv$)>
you should cover the main as well.
Line 29: ProxyPassMatch ajp://localhost:@JBOSS_AJP_PORT@
Line 30: <IfModule deflate_module>
Line 31: AddOutputFilterByType DEFLATE text/javascript text/css
text/html text/xml text/json application/xml application/json application/x-yaml
Line 32: </IfModule>
....................................................
File packaging/conf/ovirt-websocket-proxy.conf.defaults.in
Line 11: SSL_KEY=
Line 12: FORCE_DATA_VERIFICATION=False
Line 13: CERT_FOR_DATA_VERIFICATION=
Line 14: SSL_ONLY=False
Line 15: LOG_FILE=False
What is LOG_FILE false?
I guess it should be ${ENGINE_LOG}/websocket-proxy.log or something?
Line 16: VERBOSE=False
Line 17: ENGINE_USR="@ENGINE_USR@"
Line 12: FORCE_DATA_VERIFICATION=False
Line 13: CERT_FOR_DATA_VERIFICATION=
Line 14: SSL_ONLY=False
Line 15: LOG_FILE=False
Line 16: VERBOSE=False
LOG_LEVEL=DEBUG or LOG_VERBOSE=False?
Line 17: ENGINE_USR="@ENGINE_USR@"
....................................................
File packaging/services/ovirt-websocket-proxy.py
Line 15: # limitations under the License.
Line 16:
Line 17: import os
Line 18: import gettext
Line 19: import Cookie
Not needed.
Please execute pyflakes and pep8 over the python files.
Line 20: import socket
Line 21: import websockify as wsproxy
Line 22: import base64
Line 23: import json
Line 17: import os
Line 18: import gettext
Line 19: import Cookie
Line 20: import socket
Line 21: import websockify as wsproxy
why rename plain name?
Please also separate the standard python modules and the external ones...
standard
self (config...)
external (websockify, m2crypto ...)
Line 22: import base64
Line 23: import json
Line 24: _ = lambda m: gettext.dgettext(message=m, domain='ovirt-engine')
Line 25:
Line 44: def new_client(self):
Line 45: """
Line 46: Called after a new WebSocket connection has been established.
Line 47: """
Line 48: path_json = self._ticketDecoder.decode(self.path[1:])
so why do you call this json? it is a string of host,port, right?
if that's so, better to use the standard convention of host:port :)
Line 49: connection_data = path_json.split(',')
Line 50: target_host = connection_data[1].encode('utf8')
Line 51: target_port = connection_data[2].encode('utf8')
Line 52:
Line 47: """
Line 48: path_json = self._ticketDecoder.decode(self.path[1:])
Line 49: connection_data = path_json.split(',')
Line 50: target_host = connection_data[1].encode('utf8')
Line 51: target_port = connection_data[2].encode('utf8')
as far as I know, if you already used string to split... you get string so the
encode is not required.
or something else is wrong. Can you please explain why encode is needed?
Line 52:
Line 53: # Connect to the target
Line 54: self.msg("connecting to: %s:%s" % (
Line 55: target_host, target_port))
Line 51: target_port = connection_data[2].encode('utf8')
Line 52:
Line 53: # Connect to the target
Line 54: self.msg("connecting to: %s:%s" % (
Line 55: target_host, target_port))
where does this message go? to its own log file?
Line 56: tsock = self.socket(target_host, target_port,
Line 57: connect=True)
Line 58:
Line 59: if self.verbose and not self.daemon:
Line 151: cert=self._config.getString('SSL_CERTIFICATE'),
Line 152: key=self._config.getString('SSL_KEY'),
Line 153: ssl_only=self._config.getBoolean('SSL_ONLY'),
Line 154: daemon=False,
Line 155: record=self._config.getBoolean('LOG_FILE'),
Doesn't it has an option to use standard python log? It is much better to use
standard log and at one place register handler to a file.
Line 156: web=None,
Line 157: target_host='ignore',
Line 158: target_port='ignore',
Line 159: wrap_mode='exit',
Line 157: target_host='ignore',
Line 158: target_port='ignore',
Line 159: wrap_mode='exit',
Line 160: wrap_cmd=None
Line 161: ).start_server();
No need for ';'
Line 162:
Line 163:
Line 164: if __name__ == '__main__':
Line 165: 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: 6
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Frank Kobzik <[email protected]>
Gerrit-Reviewer: Alon Bar-Lev <[email protected]>
Gerrit-Reviewer: Barak Azulay <[email protected]>
Gerrit-Reviewer: Frank Kobzik <[email protected]>
Gerrit-Reviewer: Itamar Heim <[email protected]>
Gerrit-Reviewer: Martin Beták <[email protected]>
Gerrit-Reviewer: Michal Skrivanek <[email protected]>
Gerrit-Reviewer: Sandro Bonazzola <[email protected]>
Gerrit-Reviewer: Tomas Jelinek <[email protected]>
Gerrit-Reviewer: Vojtech Szocs <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
