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