GNOME Bugzilla – Bug 705925
Add partial file support to trivial-httpd
Last modified: 2013-08-14 21:49:44 UTC
Created attachment 251525 [details] [review] Partial download support and range request support in trivial-httpd I've been tasked with adding support to ostree to resume download after partial file has been downloaded or network error, etc. In order to do that I thought I'd first add support for having ostree trivial-httpd cut off files at a certain length specified on the command line. It will also need support for http range requests. The attached patch adds both of these features to the ot-builtin-trivial-httpd.c file. I'm not sure if -c for 'chop' is the best argument for specifying the serving of files should quit after X megabytes, any better ideas are welcome. I've tested this patch with a file of 11M size and ostree trivial-httpd -c 5 -p ./html_port then I do wget -c localhost:`cat test/html_port`/Testfile a few times. The first time it downloads the first 5M of the file, then the next 5, and finally the last 1M of the file is downloaded the third time. thanks
Review of attachment 251525 [details] [review]: Might be good to split this into two patches; one to add Content-Range support, the second to add a mode to truncate responses. ::: src/ostree/ot-builtin-trivial-httpd.c @@ +216,3 @@ + buffer_length = file_size; + else + buffer_length = opt_chop_size * 1000000; Indentation looks strange here; tabs instead of spaces? I guess I should add the vim modelines too...the indentation setting should be no tabs. @@ +217,3 @@ + else + buffer_length = opt_chop_size * 1000000; + if (soup_message_headers_get_ranges(msg->request_headers, 0, &ranges, &ranges_length)) We know the buffer length, so we could pass it here instead of 0 and have libsoup do cleanup/sorting. @@ +222,3 @@ + gsize size; + start = ranges[0].start; + end = ranges[0].end; We're only handling one range? That's OK for debug code, but it seems like it wouldn't be too hard to add a loop here. @@ +225,3 @@ + if (end == -1) + end = file_size; + if (start > file_size || start > end || end > file_size) This code would be cleaner if we just did: if (start > end) start = end; if (end > file_size) { ... } (Yes, we'll return an empty response if someone does start > end, but that's fine).
Created attachment 251655 [details] [review] -r --force-range-requests argument to only serve half of requested files
As discussed on irc this patch improves the following: Libsoup handles all range requests automatically. We only serve half of files when no range request is given. We serve half of all files, not just limiting by megabytes, this makes trivial-httpd serve half of even small files that would have been served completely with -c 1 because they are under 1 megabyte.
Review of attachment 251655 [details] [review]: ::: src/ostree/ot-builtin-trivial-httpd.c @@ +223,3 @@ + } + + if (ranges_length > 0) This will be reading uninitialized memory in the case where opt_force_ranges is TRUE, but the request has no ranges. Should be enough to initialize ranges_length = 0, or you could do: gboolean have_ranges; ... have_ranges = soup_message_headers_get_ranges(msg->request_headers, file_size, &ranges, &ranges_length) if (opt_force_ranges && have_ranges) ... if (have_ranges) @@ +225,3 @@ + if (ranges_length > 0) + { + soup_message_headers_free_ranges (msg->request_headers, ranges); This will also be freeing potentially unallocated memory.
Created attachment 251658 [details] [review] Updated after review
Review of attachment 251658 [details] [review]: Ok, there were only style issues remaining, and rather than bounce them to you again, I just fixed them and pushed. https://git.gnome.org/browse/ostree/commit/?id=71f6f10cd282118b41516260dedce9b550e257b3 The most important fix is to the commit message. Three things: 1) You should say *why* you're making this change. Concretely in the commit, I wrote: "This will be used to test resuming interrupted downloads for ostree-pull." 2) The first line is a summary; don't put everything on one line. 3) The commit message should include a link back to this bug for extended rationale, previous patch iterations, etc. All of the above are described in: https://live.gnome.org/GnomeLove/SubmittingPatches Just run "git log" in ostree to see other people's commit messages; they aren't perfect, but even for my tiny small commits I try to at least very briefly say *why*, not just "what". It makes a big difference coming back to code 5 years later trying to figure out why some line of code is there. Further changes: * I dropped the short option '-r'; the option is specialized enough that it seems reasonable to have consumers spell it out. * There was one indentation region using tabs; I changed it to spaces. * Dropped the braces for one-liners in conditionals (this is very minor, I don't mind them there, but here it just makes the code more readable to omit them). Thanks for the patch, and keep them coming!