Change in vdsm[master]: bridge: obtain method

nsoffer at redhat.com nsoffer at redhat.com
Tue Feb 16 09:50:26 UTC 2016


Nir Soffer has posted comments on this change.

Change subject: bridge: obtain method
......................................................................


Patch Set 2: Code-Review-1

(5 comments)

https://gerrit.ovirt.org/#/c/53472/2/tests/bridgeTests.py
File tests/bridgeTests.py:

Line 138:     def testHookError(self):
Line 139:         bridge = DynamicBridge()
Line 140: 
Line 141:         with self.assertRaises(JsonRpcError) as e:
Line 142:             bridge.dispatch('Host.ping')()
Nicer!
Line 143: 


https://gerrit.ovirt.org/#/c/53472/2/vdsm/rpc/Bridge.py
File vdsm/rpc/Bridge.py:

Line 68
Line 69
Line 70
Line 71
Line 72
dead code?

This look like the consumer of __getattr__


Line 93:             raise VdsmError(5, "Response is missing '%s' member" % member)
Line 94: 
Line 95:     def dispatch(self, method):
Line 96:         try:
Line 97:             className, methodName = method.split('.')
split('.', 1)
Line 98:             self.api['commands'][className][methodName]
Line 99:         except (KeyError, ValueError):
Line 100:             raise VdsmError(100, "Method %s.%s not found"
Line 101:                             % (className, methodName))


Line 96:         try:
Line 97:             className, methodName = method.split('.')
Line 98:             self.api['commands'][className][methodName]
Line 99:         except (KeyError, ValueError):
Line 100:             raise VdsmError(100, "Method %s.%s not found"
VdsmError is not a good choice, and initializing it with an error code is bad idea. Error code is hardcoded in each error as class attribute. If you want to use error code 100, you should use the error with error code 100.

We need to use an existing subclass that fit a client error, or invent a new error for client error (like 400 Bad Request in http).
Line 101:                             % (className, methodName))
Line 102:         return partial(self._dynamicMethod, className, methodName)
Line 103: 
Line 104:     def _convertClassName(self, name):


Line 97:             className, methodName = method.split('.')
Line 98:             self.api['commands'][className][methodName]
Line 99:         except (KeyError, ValueError):
Line 100:             raise VdsmError(100, "Method %s.%s not found"
Line 101:                             % (className, methodName))
Better use the original string provided by the caller.
Line 102:         return partial(self._dynamicMethod, className, methodName)
Line 103: 
Line 104:     def _convertClassName(self, name):
Line 105:         """


-- 
To view, visit https://gerrit.ovirt.org/53472
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9b6cb53d8e42335510e34dfe372c1c21b80ac219
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list