Liron Ar has posted comments on this change. Change subject: core: BindingXMLRPC - exporting logic out from do_PUT. ......................................................................
Patch Set 3: (4 comments) http://gerrit.ovirt.org/#/c/26740/3/vdsm/BindingXMLRPC.py File vdsm/BindingXMLRPC.py: Line 127: Line 128: def do_PUT(self): Line 129: contentLength = self._getIntHeader(self.HEADER_CONTENT_LENGTH, Line 130: httplib.LENGTH_REQUIRED) Line 131: image = self._constructImageFromRequestInfo() should return here if not image - i'll add it. Line 132: methodArgs = {'fileObj': self.rfile, Line 133: 'length': contentLength} Line 134: # Optional header Line 135: volUUID = self.headers.getheader(self.HEADER_VOLUME) Line 150: self._performSocketOp(operation) Line 151: Line 152: def _performSocketOp(self, execFunc): Line 153: try: Line 154: execFunc() > Avoiding duplication is nice but is not enough. The code must be clear and I'm against it, i don't see reason to duplicate the exact same code to another functions. I think that this code is clear enough, if you think that it's not - let's improve it's clarity rather than adding another 100 lines of code here, having need to maintain both..etc :) we can always improve clarity, but there's no reason to duplicate things in that scnario. Line 155: Line 156: except socket.timeout: Line 157: self.send_error(httplib.REQUEST_TIMEOUT, Line 158: "request timeout") Line 176: return API.Image(imgUUID, spUUID, sdUUID) Line 177: Line 178: @staticmethod Line 179: def _createEventWithCallback(self): Line 180: opFinishedEvent = threading.Event() > Currently this module seem to be the right place, but not as a nested class If we do add this event decorator, it should be in a general place so we can in further patches replace the other event users with it. Line 181: Line 182: def setCallback(): Line 183: opFinishedEvent.set() Line 184: Line 210: return Line 211: Line 212: return value Line 213: Line 214: def handle_response(self, headers, body=None): > Repeating the self.wfile.write() line *is* good. This help you to understan having a method that gets headers and body and returns a response made of them is clear enough imo, but let's agree to hear other opinions on that. Line 215: self.send_response(httplib.OK) Line 216: Line 217: for k, v in headers.iteritems(): Line 218: self.send_header(k, v) -- 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: Liron Ar <[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
