Nir Soffer has posted comments on this change. Change subject: vdsm: lastclient info in jsonrpc ......................................................................
Patch Set 9: Code-Review-1 (3 comments) http://gerrit.ovirt.org/#/c/28817/9/lib/yajsonrpc/__init__.py File lib/yajsonrpc/__init__.py: Line 449: class ClientContext(): Line 450: Line 451: def __init__(self, ctx): Line 452: self._ctx = ctx Line 453: This api does not make any sense as is. Client context can be nice idea if we have read only properties for stuff we need: @property def client_address(self): return self._ctx.client.socket.getpeername() @property def server_address(self): return self._ctx.client.socket.getsockname() I'm not sure that server address is needed here because we can get it from the configuration or from binding/acceptor/whatever - this is not related to the clilent, but lets forget about it for a minute. Now, the code in the bridge that hack the result of getCaps should use this object to modify the response. Line 454: def __call__(self, res): Line 455: res['info']['lastClient'] = self._ctx.client.socket.getpeername()[0] Line 456: res['info']['lastClientIface'] = getDeviceByIP( Line 457: self._ctx.client.socket.getsockname()[0]) http://gerrit.ovirt.org/#/c/28817/9/vdsm/rpc/Bridge.py File vdsm/rpc/Bridge.py: Line 253: Line 254: retfield = command_info.get(cmd, {}).get('ret') Line 255: if isinstance(retfield, types.FunctionType): Line 256: if cmd == 'Host_getCapabilities': Line 257: ret = retfield(self._threadLocal.ctx, result) Why support special cases? This object has a thread local ctx, why not access it directly instead of adding a special argument to one of the methods? Any code that need the context can do: self._threadlocal.ctx Line 258: else: Line 259: ret = retfield(result) Line 260: elif _glusterEnabled and className.startswith('Gluster'): Line 261: ret = dict([(key, value) for key, value in result.items() Line 269: """ Line 270: We need to add additional information to getCaps as it is done for xmlrpc. Line 271: """ Line 272: if hasattr(ctx, '__call__'): Line 273: ctx(ret) This does not make sense. This object is responsible for modifying the info - why do you have to ask an object that has nothing to do with this info to do that? And why use the __call__ interface? A callable makes sense when you can register it in the bridge as the "fucntion" that modify the return value. This should use the client interface that I suggested in my other comment: ret['info']['lastClient'] = ctx.client_address ... Line 274: Line 275: return ret['info'] Line 276: Line 277: -- To view, visit http://gerrit.ovirt.org/28817 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I402e15cb05f89a98dab14491d9da5985335e095e Gerrit-PatchSet: 9 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Piotr Kliczewski <[email protected]> Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[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: [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
