Skip to content

Conversation

@brunoramoslu
Copy link
Contributor

I was trying some things with the mirror list and I hit this "missing" functionality.

This patch add the possibility to have a format host:port in the mirrors.lst file.

Any comments are welcome.

Copy link
Collaborator

@gvissers gvissers left a comment

Choose a reason for hiding this comment

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

Some comments:

First, not to shoot down this effort, but do we have a use case for this change? Is anyone running (or wanting to run) an (HTTP) update mirror on a port other than 80? Not saying it is impossible, but port 80 is rather traditional for HTTP requests.

Second, I believe the new line 601 is the only place where the port number is actually used as an integer. Would it not be possible to keep the entire setup as is, but check at that point if the server string contains a port number?

Some further suggestions:

lines 36-39:

static void http_threaded_get_file(struct update_server_struct *server, char *path, FILE *fp, Uint8 *md5, Uint32 event);
...
static int http_get_file(struct update_server_struct *update_server, char *path, FILE *fp);

You could make update_server const. path as well, probably.

Line 150:

safe_strncpy(server_without_port,buf,128);

Might be better to use sizeof(server_without_port), or use a constant. Same in the else branch, and for portbuf.

Line 153:

	safe_strncpy2(port_buf,ptr+1,6,strlen(buf)-index+1);

I think the last argument should be strlen(buf)-index-1.

Lines 155-158: I'm not sure defaulting to port 80 when a (invalid) port number is explicitly given is the right thing to do. Might be better to just bail out and not increase num_update_servers.

Line 184: I'm not too fond of the name cmp_xxx to determine of two xxx's are different. Perhaps invert the logic and make a update_server_equal() function. Would also save a few ! negations in the code that follows.

Lines 258-263: this could be simply update_server = update_servers[num].

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants