Change in vdsm[master]: Refactor communication infra

danken at redhat.com danken at redhat.com
Fri Dec 28 21:12:45 UTC 2012


Dan Kenigsberg has posted comments on this change.

Change subject: Refactor communication infra
......................................................................


Patch Set 13: I would prefer that you didn't submit this

(6 inline comments)

....................................................
File vdsm_api/BindingJsonRpc.py
Line 37:         self._reactors = reactors
Line 38: 
Line 39:     def _createTcpReactor(self, cfg):
Line 40:         address = cfg.get("ip", "0.0.0.0")
Line 41:         port = cfg.get("port", 4044)
do we really want this 4044 default in this level?
Line 42:         return TCPReactor((address, port), self.server)
Line 43: 
Line 44:     def start(self):
Line 45:         for reactor in self._reactors:


....................................................
File vdsm_api/jsonrpc/__init__.py
Line 136:                 res = req.invokeFunction(method)
Line 137:                 resp = self._generateResponse(req, res)
Line 138: 
Line 139:         except JsonRpcError as e:
Line 140:                 resp = self._jsonError2Response(e, req)
pep8 does not mind this over-indentation?
Line 141:                 self.log.error("Error processing request", exc_info=True)
Line 142:         except:
Line 143:                 resp = self._jsonError2Response(JsonRpcInternalError(), req)
Line 144:                 self.log.error("Unexpected error while processing request",


....................................................
File vdsm_api/jsonrpc/tcpReactor.py
Line 61:                 raise
Line 62: 
Line 63:         buffLen = len(self._buffer)
Line 64:         if buffLen < _Size.size:
Line 65:             return
it would be nicer to use the same form as you do below:

  return None
Line 66: 
Line 67:         if buffLen > self.buffsize:
Line 68:             self._conn.close()
Line 69:             self.sendData = []


Line 63:         buffLen = len(self._buffer)
Line 64:         if buffLen < _Size.size:
Line 65:             return
Line 66: 
Line 67:         if buffLen > self.buffsize:
could this happen unless there's a bug in self._conn.recv()?

what is this non-private self.sendData ?
Line 68:             self._conn.close()
Line 69:             self.sendData = []
Line 70: 
Line 71:         msgLen = _Size.unpack(self._buffer[:_Size.size])[0]


....................................................
File vdsm/utils.py
Line 922:         with self._lock:
Line 923:             if self._isSet:
Line 924:                 return
Line 925: 
Line 926:             os.write(self._w, "a")
should not you handle EINTR just to fend off future improbable bugs?
Line 927:             self._isSet = True
Line 928: 
Line 929:     def isSet(self):
Line 930:         return self._isSet


Line 934:             if not self._isSet:
Line 935:                 return
Line 936: 
Line 937:             os.read(self._r, 1)
Line 938: 
don't you need to reset self._isSet sometime?
Line 939:     def __del__(self):
Line 940:         os.close(self._r)
Line 941:         os.close(self._w)
Line 942: 


--
To view, visit http://gerrit.ovirt.org/9986
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7a65364824cc8433263ccf6ed44f1f97141eff2c
Gerrit-PatchSet: 13
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Adam Litke <agl at us.ibm.com>
Gerrit-Reviewer: Barak Azulay <bazulay at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.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: oVirt Jenkins CI Server


More information about the vdsm-patches mailing list