Nir Soffer has posted comments on this change. Change subject: core: GET requests - use Range header ......................................................................
Patch Set 6: (7 comments) Good idea, can be simplified and needs some cleanup. http://gerrit.ovirt.org/#/c/28465/6//COMMIT_MSG Commit Message: Line 7: core: GET requests - use Range header Line 8: Line 9: This patch replaces the use of the custom Size header with use of http Line 10: Range header. Line 11: (http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.35) Nice! Line 12: Note that the use is similar to the use that was done with the Size header. Line 13: The motivation is to use the standard http header (instead of supporting a custom header) and not to fully comply with the spec (as there are existing gaps). Line 14: Line 15: After this change the Range header is mandatory to issue a get request for an image and support ranges between zero and specified last byte position. Line 9: This patch replaces the use of the custom Size header with use of http Line 10: Range header. Line 11: (http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.35) Line 12: Note that the use is similar to the use that was done with the Size header. Line 13: The motivation is to use the standard http header (instead of supporting a custom header) and not to fully comply with the spec (as there are existing gaps). Don't add random leading whitespace. Line 14: Line 15: After this change the Range header is mandatory to issue a get request for an image and support ranges between zero and specified last byte position. Line 16: Line 17: Change-Id: I8164867347b1cf800efd2a78cc98dbc10c02ee0d Line 11: (http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.35) Line 12: Note that the use is similar to the use that was done with the Size header. Line 13: The motivation is to use the standard http header (instead of supporting a custom header) and not to fully comply with the spec (as there are existing gaps). Line 14: Line 15: After this change the Range header is mandatory to issue a get request for an image and support ranges between zero and specified last byte position. Please keep lines shorter then 80 characters. This looks sloppy. Line 16: Line 17: Change-Id: I8164867347b1cf800efd2a78cc98dbc10c02ee0d http://gerrit.ovirt.org/#/c/28465/6/vdsm/BindingXMLRPC.py File vdsm/BindingXMLRPC.py: Line 265: def _getLength(self): Line 266: value = self._getRequiredHeader(self.HEADER_RANGE, Line 267: httplib.BAD_REQUEST) Line 268: Line 269: m = re.match('^bytes=(\d*)-(\d+)$', value) Few issues: - regex must use raw strings e.g. r'^bytes=(\d*)-(\d+)$'. If you don't use r'', special characters in the expression are not escaped, changing the semantics of the expression. - ^ is redundant in when using re.match - it matches only if the string starts with the expression. - regrex should be compiled once, and the compiled regex should be used later - this improve performance. For example: range_re = re.compile(r'^bytes=(\d*)-(\d+)$') def _getLength(self): ... m = self.range_re.match(value) - Since you want to support only some inputs, in particular, offset must be zero, the expression should be: m = re.match('^bytes=0-(\d+)$', value) Now every (valid) but unsupported input will never match. When we want to support all values, we will change the expression. We don't need to parse each value and return 20 different error messages here, this is a waste of our time at this point, and we will have to maintain this code for no benefit. Line 270: if m is None: Line 271: raise self.RequestException( Line 272: httplib.BAD_REQUEST, Line 273: "range in an unsupported format") Line 269: m = re.match('^bytes=(\d*)-(\d+)$', value) Line 270: if m is None: Line 271: raise self.RequestException( Line 272: httplib.BAD_REQUEST, Line 273: "range in an unsupported format") When you complain about such error, you should show the unmatched input: raise self.RequestException(httplib.BAD_REQUEST, "range in an unsupported format: %r", value) Line 274: Line 275: min = m.group(1) Line 276: if min != '0': Line 277: raise self.RequestException( Line 272: httplib.BAD_REQUEST, Line 273: "range in an unsupported format") Line 274: Line 275: min = m.group(1) Line 276: if min != '0': Cannot happen with the simpler regex Line 277: raise self.RequestException( Line 278: httplib.BAD_REQUEST, Line 279: "range first byte position other than 0 isn't " Line 280: "supported") Line 279: "range first byte position other than 0 isn't " Line 280: "supported") Line 281: Line 282: max = m.group(2) Line 283: if max == '': This cannot happen - (\d+) means: one more digits: >>> m = re.match('^bytes=(\d*)-(\d+)$', '-') >>> print m None Line 284: raise self.RequestException( Line 285: httplib.BAD_REQUEST, Line 286: "unspecified range last byte position isn't supported") Line 287: -- To view, visit http://gerrit.ovirt.org/28465 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8164867347b1cf800efd2a78cc98dbc10c02ee0d Gerrit-PatchSet: 6 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: [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
