Yaniv Bronhaim has posted comments on this change. Change subject: jsonrpc: Vdsm changes ......................................................................
Patch Set 18: Code-Review-1 (20 comments) http://gerrit.ovirt.org/#/c/19497/18/lib/yajsonrpc/__init__.py File lib/yajsonrpc/__init__.py: Line 52: Line 53: class JsonRpcInvalidRequestError(JsonRpcError): Line 54: def __init__(self): Line 55: JsonRpcError.__init__(self, -32600, Line 56: "The JSON sent is not a valid Request object.") can you add here a print of the request itself and an specific reason what value is wrong ? Line 57: Line 58: Line 59: class JsonRpcMethodNotFoundError(JsonRpcError): Line 60: def __init__(self): Line 148: obj = json.loads(msg, 'utf-8') Line 149: # TODO: More validations Line 150: result = obj.get('result') Line 151: error = JsonRpcError(**obj.get('error')) Line 152: reqId = obj.get('id') why here you don't verify the obj data? Line 153: return JsonRpcResponse(result, error, reqId) Line 154: Line 155: @staticmethod Line 156: def fromRawObject(obj): Line 254: Line 255: Line 256: class JsonRpcClientPool(object): Line 257: log = logging.getLogger("JsonRpcClientPool") Line 258: why don't you define log inside the constructor? self._log = .. it doesn't need class related Line 259: def __init__(self, reactors): Line 260: self._reactors = reactors Line 261: self._inbox = Queue() Line 262: self._clients = {} Line 318: try: Line 319: mobj = json.loads(message) Line 320: isResponse = self._isResponse(mobj) Line 321: except: Line 322: self.log.warning("Problem parsing message from client") better to print the traceback here. Line 323: transport.close() Line 324: del self._clients[transport] Line 325: continue Line 326: Line 353: self.responses = None Line 354: Line 355: def _callback(self, c, resp): Line 356: if not isinstance(resp, list): Line 357: resp = [resp] its being used outside, remove the _ , it should be public Line 358: Line 359: self.responses = resp Line 360: self._ev.set() Line 361: Line 367: return self._ev.is_set() Line 368: Line 369: Line 370: class JsonRpcClient(object): Line 371: log = logging.getLogger("jsonrpc.JsonRpcClient") same.. define it inside the __init__ Line 372: Line 373: def __init__(self, transport): Line 374: self._transport = transport Line 375: self._runningRequests = {} http://gerrit.ovirt.org/#/c/19497/18/lib/yajsonrpc/betterAsyncore.py File lib/yajsonrpc/betterAsyncore.py: Line 115: return SSLSocket(SSL.Connection(context, sock=sock), self) Line 116: Line 117: Line 118: # This is a copy of the standard library asyncore converted to support Line 119: # compositing. Also fixes races in original implementation. don't forget to ask saggi to put more info here Line 120: class AsyncChat(object): Line 121: # these are overridable defaults Line 122: Line 123: ac_in_buffer_size = 4096 Line 117: Line 118: # This is a copy of the standard library asyncore converted to support Line 119: # compositing. Also fixes races in original implementation. Line 120: class AsyncChat(object): Line 121: # these are overridable defaults why? Line 122: Line 123: ac_in_buffer_size = 4096 Line 124: ac_out_buffer_size = 4096 Line 125: Line 261: self.producer_fifo.append(producer) Line 262: Line 263: def readable(self, dispatcher): Line 264: "predicate for inclusion in the readable for select()" Line 265: return 1 why not False? Line 266: Line 267: def writable(self, dispatcher): Line 268: "predicate for inclusion in the writable for select()" Line 269: with self._fifoLock: Line 317: # we tried to send some actual data Line 318: return Line 319: Line 320: def discard_buffers(self): Line 321: # Emergencies only! what does it mean? elaborate in that comment.. Line 322: self.ac_in_buffer = '' Line 323: del self.incoming[:] Line 324: with self._fifoLock: Line 325: self.producer_fifo.clear() Line 378: Line 379: try: Line 380: impl.init(self) Line 381: except AttributeError: Line 382: pass put a comment why you ignore AttributeError here (e.g. # impl.init() is optional.) Line 383: Line 384: def create_socket(self, family, type): Line 385: self.family_and_type = family, type Line 386: sock = SSL.Connection(self.__sslcontext.context, family=family) http://gerrit.ovirt.org/#/c/19497/18/lib/yajsonrpc/protonReactor.py File lib/yajsonrpc/protonReactor.py: Line 1: # Copyright (C) 2012 Saggi Mizrahi, Red Hat Inc. 2014? Line 2: # Line 3: # This program is free software; you can redistribute it and/or modify Line 4: # it under the terms of the GNU General Public License version 2 as Line 5: # published by the Free Software Foundation. Line 25: FAILED = 0 Line 26: CONNECTED = 1 Line 27: SERVER_AUTH = 2 Line 28: CLIENT_AUTH = 3 Line 29: all my previous comments about that file waits for saggi's response.. btw, do you have tests for this reactor? Line 30: MBUFF_SIZE = 10 Line 31: Line 32: Line 33: class ProtonError(RuntimeError): http://gerrit.ovirt.org/#/c/19497/18/tests/Makefile.am File tests/Makefile.am: Line 133: Line 134: # crossImportsTests.py has to be run separate due to different tests which Line 135: # load modules first, making the env dirty. Unloading python modules Line 136: # is not supported, see http://bugs.python.org/issue9072 . Line 137: check-local: jsonrpc-tests.server.crt so we move on with this approach ? fine by me... Line 138: @echo '*** Running tests. To skip this step place NOSE_EXCLUDE=.* ***' Line 139: @echo '*** into your environment. Do not submit untested code! ***' Line 140: $(top_srcdir)/tests/run_tests_local.sh crossImportsTests.py http://gerrit.ovirt.org/#/c/19497/18/tests/jsonRpcTests.py File tests/jsonRpcTests.py: Line 182: bridge = _DummyBridge() Line 183: with constructServer(rt, bridge, ssl) as (server, clientFactory): Line 184: with self._client(clientFactory) as client: Line 185: self.assertEquals(self._callTimeout(client, "echo", (data,), Line 186: '2c8134fd-7dd4-4cfc-b7f8-6b7549399cb6', use the id you declared in apiTests.id .. Line 187: CALL_TIMEOUT), data) Line 188: Line 189: @permutations(CONNECTION_PERMUTATIONS) Line 190: def testMethodCallArgDict(self, rt, ssl): http://gerrit.ovirt.org/#/c/19497/18/tests/jsonRpcUtils.py File tests/jsonRpcUtils.py: Line 16: protonReactor = None Line 17: try: Line 18: import proton Line 19: from yajsonrpc import protonReactor Line 20: proton # Squash pyflakes error for you don't use that, so why do you import? Line 21: protonReactor # unused import Line 22: except ImportError: Line 23: pass Line 24: Line 17: try: Line 18: import proton Line 19: from yajsonrpc import protonReactor Line 20: proton # Squash pyflakes error for Line 21: protonReactor # unused import but you do use that ... remove Line 22: except ImportError: Line 23: pass Line 24: Line 25: Line 59: Line 60: REACTOR_CONSTRUCTORS = {"tcp": _tcpServerConstructor, Line 61: "amqp": _protonServerConstructor} Line 62: REACTOR_TYPE_PERMUTATIONS = [[r] for r in REACTOR_CONSTRUCTORS.iterkeys()] Line 63: SSL_OPTIONS = (True, False) is there a meaning for this constant? Line 64: CONNECTION_PERMUTATIONS = tuple(product(REACTOR_CONSTRUCTORS.iterkeys(), Line 65: SSL_OPTIONS)) Line 66: Line 67: CERT_DIR = os.path.abspath(os.path.dirname(__file__)) http://gerrit.ovirt.org/#/c/19497/18/vdsm.spec.in File vdsm.spec.in: Line 114: Requires: rpm-python Line 115: Requires: nfs-utils Line 116: Requires: m2crypto Line 117: Requires: libguestfs-tools-c Line 118: Requires: %{name}-xmlrpc = %{version}-%{release} you can choose now if to use xmlrpc or jsonrpc , so we don't need to require specifically xmlrpc anymore. anyway, we can handle it later Line 119: Requires: mom >= 0.3.2-3 Line 120: Line 121: %ifarch x86_64 Line 122: Requires: python-dmidecode http://gerrit.ovirt.org/#/c/19497/18/vdsm/API.py File vdsm/API.py: Line 1192: self.log.error("failed to retrieve hardware info", exc_info=True) Line 1193: return errCode['hwInfoErr'] Line 1194: Line 1195: def getAllVmStats(self): Line 1196: """ put this and the bindingXMLRPC.py change in new patch, and I will take care that it'll be merged before we take this one. than you'll rebase and we won't mix between wrong implementation fix to json rpc code Line 1197: Get statistics of all running VMs. Line 1198: """ Line 1199: vms = self.getVMList() Line 1200: statsList = [] -- To view, visit http://gerrit.ovirt.org/19497 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If828355b7efe28fe6a2e784069425fefd2f3f25c Gerrit-PatchSet: 18 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Saggi Mizrahi <smizr...@redhat.com> Gerrit-Reviewer: Adam Litke <ali...@redhat.com> Gerrit-Reviewer: Barak Azulay <bazu...@redhat.com> Gerrit-Reviewer: Eduardo <ewars...@redhat.com> Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: Saggi Mizrahi <smizr...@redhat.com> Gerrit-Reviewer: Sahina Bose <sab...@redhat.com> Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: mooli tayer <mta...@redhat.com> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
