Yoav Kleinberger has posted comments on this change. Change subject: core: BindingXMLRPC - exporting logic out from do_PUT. ......................................................................
Patch Set 3: Code-Review-1 (3 comments) the main thing wrong here is the way you did the refactoring. I explained how I think it should be done in the comments. http://gerrit.ovirt.org/#/c/26740/3/vdsm/BindingXMLRPC.py File vdsm/BindingXMLRPC.py: Line 126: return basehandler.setup(self) Line 127: Line 128: def do_PUT(self): Line 129: contentLength = self._getIntHeader(self.HEADER_CONTENT_LENGTH, Line 130: httplib.LENGTH_REQUIRED) when _getIntHeader doesn't find the header, you will continue here to the next line even though you should not. use exceptions. Line 131: image = self._constructImageFromRequestInfo() Line 132: methodArgs = {'fileObj': self.rfile, Line 133: 'length': contentLength} Line 134: # Optional header Line 133: 'length': contentLength} Line 134: # Optional header Line 135: volUUID = self.headers.getheader(self.HEADER_VOLUME) Line 136: Line 137: def operation(): this code is totally cryptic. way too abstract. you should do this differently: refactor every simple thing into a method that throws an exception on failure. e.g. def _extractHeader( header ): value = self.headers.getheader( header ) if not value: raise Exception( 'could not find header %s' % header ) return value then use all these simple functions one after the other inside a big try/except. if you catch an Exception, then send_error etc. Line 138: opEndEvent, opEndCallback = self._createEventWithCallback() Line 139: response = image.downloadFromStream(methodArgs, Line 140: opEndCallback, volUUID) Line 141: json_response = self._convertToJson(response) Line 211: Line 212: return value Line 213: Line 214: def handle_response(self, headers, body=None): Line 215: self.send_response(httplib.OK) handle_response is a bad name. This is not a handler, it is a helper. Line 216: Line 217: for k, v in headers.iteritems(): Line 218: self.send_header(k, v) Line 219: -- To view, visit http://gerrit.ovirt.org/26740 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I64bb1b0a4cb85ce822929f1907847dd63eb69fc2 Gerrit-PatchSet: 3 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: Tal Nisan <[email protected]> Gerrit-Reviewer: Yoav Kleinberger <[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
