Liron Aravot has posted comments on this change. Change subject: core: GET requests - use Range header ......................................................................
Patch Set 6: (9 comments) 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! thanks 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. Done 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. Done 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. Done, although in that case it's unneeded, but let's have it. >> ^ is redundant in when using re.match - it matches only if the string starts >> >> with the expression. I added it intentionally for making it clearer and avoid possibly future mistakes caused by not having it there. i prefer to keep it. >>regrex should be compiled once, and the compiled regex should be used >>later >>- this improve performance. For example: I didn't do it at the first place as according to the python documentation - "Under the hood, these functions simply create a pattern object for you and call the appropriate method on it. They also store the compiled object in a cache, so future calls using the same RE are faster. Should you use these module-level functions, or should you get the pattern and call its methods yourself? That choice depends on how frequently the RE will be used, and on your personal coding style. If the RE is being used at only one point in the code, then the module functions are probably more convenient." so mainly, you need to do that in case you use the regex multiple times are if you are afraid that somehow it'll be removed from the cache. as we are in an upload image scenario, the cost here will ever be minimal - i prefer to keep the code shorter here and have the regex here as there is practically no cost and the regex is used only once and imo it's clearer that way..let's wait for Fedes opinion on that. >> 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) I intentionally didn't use that regular expression, i want to have clear errors to be understood better the users/clients and eliminate the need to compare you request with the supported regex to find what's wrong. The added overhead resides in a separate methods so i don't see reason to "shorten" things here. I prefer to be more informative in that case (which will be even more useful what's it'll be returned as part of the response). 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: Done Line 274: Line 275: min = m.group(1) Line 276: if min != '0': Line 277: raise self.RequestException( Line 271: raise self.RequestException( Line 272: httplib.BAD_REQUEST, Line 273: "range in an unsupported format") Line 274: Line 275: min = m.group(1) > Please use the same terms used in the rfc - first_byte Done Line 276: if min != '0': Line 277: raise self.RequestException( Line 278: httplib.BAD_REQUEST, Line 279: "range first byte position other than 0 isn't " Line 278: httplib.BAD_REQUEST, Line 279: "range first byte position other than 0 isn't " Line 280: "supported") Line 281: Line 282: max = m.group(2) > Please use the same terms used in the rfc - last_byte Done Line 283: if max == '': Line 284: raise self.RequestException( Line 285: httplib.BAD_REQUEST, Line 286: "unspecified range last byte position isn't 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: see the comment for your long comment earlier in that file. Line 284: raise self.RequestException( Line 285: httplib.BAD_REQUEST, Line 286: "unspecified range last byte position isn't supported") Line 287: Line 284: raise self.RequestException( Line 285: httplib.BAD_REQUEST, Line 286: "unspecified range last byte position isn't supported") Line 287: Line 288: return self._getInt(max) > There is a off-by-one error here. When the client send the range 0-499, the Done Line 289: Line 290: def send_error(self, error, message, exc_info=False): Line 291: try: Line 292: self.log.error(message, exc_info=exc_info) -- 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 Aravot <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Liron Aravot <[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
