Nir Soffer has posted comments on this change. Change subject: BindingXmlRPC - do_PUT to return execution result in headers ......................................................................
Patch Set 1: Code-Review-1 (3 comments) http://gerrit.ovirt.org/#/c/27997/1/vdsm/BindingXMLRPC.py File vdsm/BindingXMLRPC.py: Line 116: HEADER_DOMAIN = 'Storage-Domain-Id' Line 117: HEADER_IMAGE = 'Image-Id' Line 118: HEADER_VOLUME = 'Volume-Id' Line 119: HEADER_TASK_ID = 'Task-Id' Line 120: HEADER_CODE = 'Code' Code and Message are not specific enough, and not consistent with other names. I suggest: Error-Code Error-Message Or: Status-Code Status-Message Line 121: HEADER_MESSAGE = 'Message' Line 122: HEADER_CONTENT_LENGTH = 'content-length' Line 123: HEADER_CONTENT_TYPE = 'content-type' Line 124: Line 169: response = image.downloadFromStream(methodArgs, Line 170: upload_finished, Line 171: volUUID) Line 172: Line 173: code, message, task = self._parse_response(response) This function does not help anyone. There is nothing to parse - this is just a dictionary and we can access it when needed. Please use more direct code: if response['status']['code'] != 0: handle failure... Sending headers: self.send_header(self.HEADER_CODE, response['status']['code']) self.send_header(self.HEADER_MESSAGE, response['status']['message']) Since we will need the same headers in uploadToStream, it will be a good idea to add method for adding these headers: def _sendResponseHeaders(self, response): self.send_header(self.HEADER_CODE, response['status']['code']) self.send_header(self.HEADER_MESSAGE, response['status']['message']) Line 174: Line 175: if code != 0: Line 176: self.send_response(httplib.INTERNAL_SERVER_ERROR) Line 177: else: Line 191: except Exception: Line 192: self.send_error(httplib.INTERNAL_SERVER_ERROR, Line 193: "error during execution", exc_info=True) Line 194: Line 195: @staticmethod Do not add static methods, there is no reason to call this outside of this class. static methods are just like module level functions and almost in all cases they do not add any value. In particular in this inner class, where you cannot access the RequestHandler._parse_response(). Line 196: def _parse_response(response): Line 197: return response['status']['code'], \ Line 198: response['status']['message'], \ Line 199: response['uuid'] -- To view, visit http://gerrit.ovirt.org/27997 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I25b13f44a65cb2c794c458b03ba361bf0af92120 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Liron Ar <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Nir Soffer <[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
