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

smizrahi at redhat.com smizrahi at redhat.com
Thu Jun 5 11:03:00 UTC 2014


Saggi Mizrahi has posted comments on this change.

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


Patch Set 32: Code-Review-1

(4 comments)

http://gerrit.ovirt.org/#/c/26300/32/lib/vdsm/utils.py
File lib/vdsm/utils.py:

Line 229:             self.end_headers()
Line 230:             self.wfile.write(response)
Line 231: 
Line 232: 
Line 233: class ConnectedTCPServer(SocketServer.TCPServer, object):
Why are you also inheriting from object?
Line 234: 
Line 235:     def __init__(self, RequestHandlerClass):
Line 236:         super(ConnectedTCPServer, self).__init__(None, RequestHandlerClass,
Line 237:                                                  False)


http://gerrit.ovirt.org/#/c/26300/32/lib/yajsonrpc/__init__.py
File lib/yajsonrpc/__init__.py:

Line 488:             res = True if res is None else res
Line 489: 
Line 490:             ctx.requestDone(JsonRpcResponse(res, None, req.id))
Line 491: 
Line 492:     @traceback(on=log.name)
I don't know who added that traceback function to the code but I don't like it. Please just use plain old try\except.

This kind of code makes things needlessly complex and it makes the log errors confusingly similar. Whenever you log an error you need to give context in addition to the traceback.
Line 493:     def serve_requests(self):
Line 494:         while True:
Line 495:             self.log.debug("Waiting for request")
Line 496:             obj = self._workQueue.get()


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

Line 136: 
Line 137:     def wakeup(self):
Line 138:         try:
Line 139:             os.write(self._write_fd, '1')
Line 140:         except OSError as e:
You would never get to the second condition since if e.errno == errno.EINTR it is necessarily not in (errno.EAGAIN, errno.EWOULDBLOCK). Switch the order of conditions.
Line 141:             if e.errno not in (errno.EAGAIN, errno.EWOULDBLOCK):
Line 142:                 raise
Line 143:             elif e.errno == errno.EINTR:
Line 144:                 self.wakeup()


Line 174:         try:
Line 175:             data = client_socket.recv(self._required_size, socket.MSG_PEEK)
Line 176:         except socket.error as e:
Line 177:             if e.errno not in (errno.EAGAIN, errno.EWOULDBLOCK):
Line 178:                 self.log.warning("Not able to read data")
s/Not Able/Unable/
Line 179:                 self._remove_connection(client_socket)
Line 180:                 client_socket.close()
Line 181:             return
Line 182: 


-- 
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: 32
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: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Liron Ar <laravot at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek 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: Vinzenz Feenstra <vfeenstr 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