Change in vdsm[master]: json-rpc: Protocol detection

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: json-rpc: Protocol detection
......................................................................

Patch Set 31:

(5 comments)

The issue in version 29 fixed, but I found few easy to fix issues.

Waiting for confirmation that xmlrpc works correctly with last engine stable release.

http://gerrit.ovirt.org/#/c/26300/31/vdsm/BindingXMLRPC.py
File vdsm/BindingXMLRPC.py:

Line 1131:         self.xml_binding = xml_binding
Line 1132: 
Line 1133:     def detect(self, data):
Line 1134:         return (data.startswith("PUT /") or data.startswith("GET /") or
Line 1135:                 data.startswith("POST /RPC2"))
Did you verify that both old and new engine versions use /RPC2?
Line 1136: 
Line 1137:     def handleSocket(self, client_socket, socket_address):
Line 1138:         self.xml_binding.add_socket(client_socket, socket_address)

http://gerrit.ovirt.org/#/c/26300/31/vdsm/protocolDetector.py
File vdsm/protocolDetector.py:

Line 137:     def wakeup(self):
Line 138:         try:
Line 139:             os.write(self._write_fd, '1')
Line 140:         except OSError as e:
Line 141:             if e.erron not in (errno.EAGAIN, errno.EINPROGRESS):
- A careless reviewer suggested to check EINPROGRESS here - should be EWOULDBLOCK.
- e.erron will raise AttriuteError, should be e.errno
- If we get EINTR, we should retry the call, otherwise the wakeup fill fail.
Line 142:                 raise
Line 143: 
Line 144:     def _cleanup_wakeup_pipe(self):
Line 145:         try:

Line 144:     def _cleanup_wakeup_pipe(self):
Line 145:         try:
Line 146:             os.read(self._read_fd, 128)
Line 147:         except OSError as e:
Line 148:             if e.erron not in (errno.EAGAIN, errno.EINPROGRESS):
- A careless reviewer suggested to check EINPROGRESS - here should be EWOULDBLOCK.
- e.erron will raise AttriuteError, should be e.errno
Line 149:                 raise
Line 150: 
Line 151:     def _accept_connection(self):
Line 152:         client_socket, _ = self._socket.accept()

Line 171:         _, client_socket = self._pending_connections[fd]
Line 172:         try:
Line 173:             data = client_socket.recv(self._required_size, socket.MSG_PEEK)
Line 174:         except socket.error as e:
Line 175:             if e.errno not in (errno.EAGAIN, errno.EINPROGRESS):
- A careless reviewer suggested to check EINPROGRESS here - should be EWOULDBLOCK.
Line 176:                 self.log.warning("Not able to read data")
Line 177:                 self._remove_connection(client_socket)
Line 178:                 client_socket.close()
Line 179:             return

Line 198:                             % (self._host, str(self._port)))
Line 199:         server_socket = socket.socket(addr[0][0], addr[0][1], addr[0][2])
Line 200:         server_socket.bind(addr[0][4])
Line 201:         server_socket.listen(5)
Line 202:         utils.closeOnExec(server_socket.fileno())
This must be done as soon as possible after creating the socket. Performing it after possibly blocking operation like bind and listen is not a good idea.
Line 203: 
Line 204:         if self._sslctx:
Line 205:             server_socket = SSLServerSocket(raw=server_socket,
Line 206:                                             certfile=self._sslctx.cert_file,