Skip to content

Extend supported range requests for large files on 64-bit hosts #59

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Extend supported range requests for large files on 64-bit hosts #59

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions src/legacy_http.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,9 +340,9 @@ static int fill_buffer(HTTP_FILE *file, size_t want, CURLM* multi_handle)
*
* Removes `want` bytes from the front of the buffer.
*/
static int use_buffer(HTTP_FILE *file, int want)
static int use_buffer(HTTP_FILE *file, size_t want)
{
if((file->buffer_pos - want) <= 0){
if(file->buffer_pos <= want){
Copy link
Member

@TheAssassin TheAssassin Apr 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the type of buffer_pos?

You should use a fixed 64-bit type like uint64_t, or unsigned long long. On 32-bit architectures, size_t is 4 bytes in size.

Copy link
Contributor Author

@jonathonf jonathonf Apr 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

buffer_pos is size_t: https://github.com/AppImage/zsync2/blob/86cfd3a1d6a27483ec40edd62c1a6bd409cbbe5d/src/legacy_http.c#L78

Copy link
Member

@TheAssassin TheAssassin Apr 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All these need to have a real 8-byte size type like uint64_t. I'm very certain size_t will be too small on 32-bit, same goes for off_t. I just tried with gcc -m32:

#include <stdio.h>
#include <sys/types.h>

int main() {
    printf("%lu\n", sizeof(off_t));
}

It's generally a good idea to use fixed-size types, as they're predictable and not compiler-dependent. And in this case, it's even a must, since we also have 32-bit binaries. You can't support big files on 64-bit only.

Copy link
Member

@TheAssassin TheAssassin Apr 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is, by the way, the big issue with the patch in #31.

Copy link
Contributor Author

@jonathonf jonathonf Apr 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears that just opening a 2GB+ file with a 32-bit build is currently broken, e.g. at

https://github.com/AppImage/zsync2/blob/86cfd3a1d6a27483ec40edd62c1a6bd409cbbe5d/src/zsclient.cpp#L986

so just increasing type size on those systems won't help.

Copy link
Member

@TheAssassin TheAssassin Apr 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be. But I don't see the point in accepting a pull request that doesn't completely solve an issue, while it claims to do so. Renaming the PR doesn't make things better, really.

Copy link
Contributor Author

@jonathonf jonathonf Apr 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR was renamed specifically because you pointed out that it doesn't actually solve the whole issue, yet does still improve things for 64-bit builds (i.e. a 64-bit build can download files larger than 2GB, which, for me at least, makes things better than when #31 was opened in 2018).

Copy link
Member

@TheAssassin TheAssassin Apr 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The argument is nonsense. There is no reason not to just swap the types you change anyway for ones that actually work on 32-bit as well. It doesn't have to make 32-bit work entirely... Nobody requested this.

But your PR is really off-putting in that its argument is "at least it's better than nothing". Of course, a full-featured 32-bit fix would be nice. But I'm certain you won't be willing to work on that. At least you could fix the types in the lines you touch anyway, permanently.

Copy link
Contributor Author

@jonathonf jonathonf Apr 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't have to make 32-bit work entirely... Nobody requested this

I may have misinterpreted this statement:

I don't see the point in accepting a pull request that doesn't completely solve an issue

Copy link
Member

@TheAssassin TheAssassin Apr 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point is, if you know an exact set of lines has to be touched in the future because the data types don't work on one platform properly, it really makes sense to just use the future proof data type. I basically had to re-do your changes to change the types to the future proof ones. This resulted in twice the work overall.

/* trash the buffer */
if(file->buffer){
free(file->buffer);
Expand Down Expand Up @@ -509,7 +509,7 @@ static void buflwr(char *s) {
int range_fetch_read_http_headers(struct range_fetch *rf) {
char buf[512];
int status;
int seen_location = 0;
size_t seen_location = 0;

{ /* read status line */
char *p;
Expand Down Expand Up @@ -571,7 +571,7 @@ int range_fetch_read_http_headers(struct range_fetch *rf) {
p += 2;
buflwr(buf);
{ /* Remove the trailing \r\n from the value */
int len = strcspn(p, "\r\n");
size_t len = strcspn(p, "\r\n");
p[len] = 0;
}
/* buf is the header name (lower-cased), p the value */
Expand All @@ -580,7 +580,7 @@ int range_fetch_read_http_headers(struct range_fetch *rf) {
if (status == 206 && !strcmp(buf, "content-range")) {
/* Okay, we're getting a non-MIME block from the remote. Get the
* range and set our state appropriately */
int from, to;
size_t from, to;
sscanf(p, "bytes " OFF_T_PF "-" OFF_T_PF "/", &from, &to);
if (from <= to) {
rf->block_left = to + 1 - from;
Expand Down