Nir Soffer has posted comments on this change. Change subject: json-rpc: Protocol detection ......................................................................
Patch Set 15: (4 comments) http://gerrit.ovirt.org/#/c/26300/15/vdsm/protocolDetector.py File vdsm/protocolDetector.py: Line 63: self._cleanup() Line 64: elif fd is self.socket.fileno(): Line 65: (client_socket, socket_address) = self._accept_conn() Line 66: try: Line 67: data = client_socket.recv(4096, socket.MSG_PEEK) This socket may block stopping the whole server timeout seconds (60 by default). While the server is blocked, no other socket will be accepted. The accepted socket should be in nonblocking mode, and EAGAIN or EINPROGRESS (and probably others) errors must be handled when peeking into the socket data. It is possible that this recv will return data here, but it is more likely that the accepted socket does not have data at this time. If you got data, you can try to detect the handler. Detection may fail because you may get only few bytes from the socket, although this is unlikely. So you may need to try the detection several times. You need to add the socket to the poller, and wait until it is readable and try to read again. Finally, you have to decide when to stop detecting and close this socket. For example, no data was received or detection fails after some timeout. I wonder why we are going in this tricky direction, while using different port for each protocol requires no code at all. Line 68: except: Line 69: data = None Line 70: Line 71: if not data: Line 64: elif fd is self.socket.fileno(): Line 65: (client_socket, socket_address) = self._accept_conn() Line 66: try: Line 67: data = client_socket.recv(4096, socket.MSG_PEEK) Line 68: except: This should catch only the relevant errors, otherwise it will hide any errors like: data = client_socket.recv(4096, socket.MSG_PEEEK) Line 69: data = None Line 70: Line 71: if not data: Line 72: self.log.warn("Unrecognized protocol from %s", Line 69: data = None Line 70: Line 71: if not data: Line 72: self.log.warn("Unrecognized protocol from %s", Line 73: socket_address) This warning is bogus. It just means that the socket has no data yet. Line 74: continue Line 75: Line 76: for handler in self.handlers.values(): Line 77: if handler.detect(data): Line 116: os.read(self._read_fd, 1) Line 117: Line 118: def _accept_conn(self): Line 119: client_socket, socket_address = self.socket.accept() Line 120: client_socket.settimeout(self._timeout) This socket is in blocking mode now... Line 121: return (client_socket, socket_address) Line 122: Line 123: def _create_socket(self, host, port): Line 124: addr = socket.getaddrinfo(host, port, socket.AF_INET, -- To view, visit http://gerrit.ovirt.org/26300 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id739a40e2b37dcc175137ec91cd5ec166ad24a75 Gerrit-PatchSet: 15 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Piotr Kliczewski <[email protected]> Gerrit-Reviewer: Barak Azulay <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Piotr Kliczewski <[email protected]> Gerrit-Reviewer: Saggi Mizrahi <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: Yeela Kaplan <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
