Change in vdsm[master]: vdsm: lastclient info in jsonrpc
nsoffer at redhat.com
nsoffer at redhat.com
Mon Jun 30 16:14:04 UTC 2014
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 <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken 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: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list