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

nsoffer at redhat.com nsoffer at redhat.com
Mon May 19 18:53:09 UTC 2014


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 <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: Barak Azulay <bazulay at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: Yeela Kaplan <ykaplan at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list