Nir Soffer has posted comments on this change.

Change subject: vdsm: lastclient info in jsonrpc
......................................................................


Patch Set 9:

(1 comment)

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)
> Correct if the code is part of Bridge class which is not true for Host_getC
Right - I was fooled by gerrit. So this show that conversion functions need to 
be part of the bridge, or get some context.

Please do not add context for specific method, instead have a simple design 
that works for any case. So either all functions get the context, or they 
should be bridge methods, or all of them belong to some other object that have 
this context.
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()


-- 
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

Reply via email to