[pacman-dev] [PATCH 1/6] dload: move curl option setting to static func
Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- lib/libalpm/dload.c | 112 ++++++++++++++++++++++++++++----------------------- 1 files changed, 62 insertions(+), 50 deletions(-) diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index 2a1fb41..16b2206 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -177,18 +177,74 @@ static size_t parse_headers(void *ptr, size_t size, size_t nmemb, void *user) return realsize; } +static int curl_set_handle_opts(struct dload_payload *payload, + const char *destfile, const char *tempfile, FILE **localf, + const char **open_mode, char *error_buffer) +{ + char *useragent = getenv("HTTP_USER_AGENT"); + alpm_handle_t *handle = payload->handle; + struct stat st; + + /* the curl_easy handle is initialized with the alpm handle, so we only need + * to reset the curl handle set parameters for each time it's used. */ + curl_easy_reset(handle->curl); + curl_easy_setopt(handle->curl, CURLOPT_URL, payload->fileurl); + curl_easy_setopt(handle->curl, CURLOPT_FAILONERROR, 1L); + curl_easy_setopt(handle->curl, CURLOPT_ERRORBUFFER, error_buffer); + curl_easy_setopt(handle->curl, CURLOPT_CONNECTTIMEOUT, 10L); + curl_easy_setopt(handle->curl, CURLOPT_FILETIME, 1L); + curl_easy_setopt(handle->curl, CURLOPT_NOPROGRESS, 0L); + curl_easy_setopt(handle->curl, CURLOPT_FOLLOWLOCATION, 1L); + curl_easy_setopt(handle->curl, CURLOPT_PROGRESSFUNCTION, curl_progress); + curl_easy_setopt(handle->curl, CURLOPT_PROGRESSDATA, (void *)payload); + curl_easy_setopt(handle->curl, CURLOPT_LOW_SPEED_LIMIT, 1024L); + curl_easy_setopt(handle->curl, CURLOPT_LOW_SPEED_TIME, 10L); + curl_easy_setopt(handle->curl, CURLOPT_HEADERFUNCTION, parse_headers); + curl_easy_setopt(handle->curl, CURLOPT_WRITEHEADER, (void *)payload); + + if(payload->max_size) { + curl_easy_setopt(handle->curl, CURLOPT_MAXFILESIZE, payload->max_size); + } + + if(useragent != NULL) { + curl_easy_setopt(handle->curl, CURLOPT_USERAGENT, useragent); + } + + if(!payload->allow_resume && !payload->force && destfile && stat(destfile, &st) == 0) { + /* start from scratch, but only download if our local is out of date. */ + curl_easy_setopt(handle->curl, CURLOPT_TIMECONDITION, CURL_TIMECOND_IFMODSINCE); + curl_easy_setopt(handle->curl, CURLOPT_TIMEVALUE, (long)st.st_mtime); + } else if(stat(tempfile, &st) == 0 && payload->allow_resume) { + /* a previous partial download exists, resume from end of file. */ + *open_mode = "ab"; + curl_easy_setopt(handle->curl, CURLOPT_RESUME_FROM, (long)st.st_size); + _alpm_log(handle, ALPM_LOG_DEBUG, "tempfile found, attempting continuation\n"); + payload->initial_size = (double)st.st_size; + } + + if(*localf == NULL) { + *localf = fopen(tempfile, *open_mode); + if(*localf == NULL) { + return 1; + } + } + + curl_easy_setopt(handle->curl, CURLOPT_WRITEDATA, *localf); + + return 0; +} + + static int curl_download_internal(struct dload_payload *payload, const char *localpath, char **final_file) { int ret = -1; FILE *localf = NULL; - const char *useragent; const char *open_mode = "wb"; char *destfile = NULL, *tempfile = NULL, *effective_url; /* RFC1123 states applications should support this length */ char hostname[256]; - char error_buffer[CURL_ERROR_SIZE]; - struct stat st; + char error_buffer[CURL_ERROR_SIZE] = { 0 }; long timecond, remote_time = -1; double remote_size, bytes_dl; struct sigaction sig_pipe[2], sig_int[2]; @@ -233,55 +289,11 @@ static int curl_download_internal(struct dload_payload *payload, payload->filename = strrchr(randpath, '/') + 1; } - error_buffer[0] = '\0'; - - /* the curl_easy handle is initialized with the alpm handle, so we only need - * to reset the curl handle set parameters for each time it's used. */ - curl_easy_reset(handle->curl); - curl_easy_setopt(handle->curl, CURLOPT_URL, payload->fileurl); - curl_easy_setopt(handle->curl, CURLOPT_FAILONERROR, 1L); - curl_easy_setopt(handle->curl, CURLOPT_ERRORBUFFER, error_buffer); - curl_easy_setopt(handle->curl, CURLOPT_CONNECTTIMEOUT, 10L); - curl_easy_setopt(handle->curl, CURLOPT_FILETIME, 1L); - curl_easy_setopt(handle->curl, CURLOPT_NOPROGRESS, 0L); - curl_easy_setopt(handle->curl, CURLOPT_FOLLOWLOCATION, 1L); - curl_easy_setopt(handle->curl, CURLOPT_PROGRESSFUNCTION, curl_progress); - curl_easy_setopt(handle->curl, CURLOPT_PROGRESSDATA, (void *)payload); - curl_easy_setopt(handle->curl, CURLOPT_LOW_SPEED_LIMIT, 1024L); - curl_easy_setopt(handle->curl, CURLOPT_LOW_SPEED_TIME, 10L); - curl_easy_setopt(handle->curl, CURLOPT_HEADERFUNCTION, parse_headers); - curl_easy_setopt(handle->curl, CURLOPT_WRITEHEADER, (void *)payload); - - if(payload->max_size) { - curl_easy_setopt(handle->curl, CURLOPT_MAXFILESIZE, payload->max_size); - } - - useragent = getenv("HTTP_USER_AGENT"); - if(useragent != NULL) { - curl_easy_setopt(handle->curl, CURLOPT_USERAGENT, useragent); - } - - if(!payload->allow_resume && !payload->force && destfile && stat(destfile, &st) == 0) { - /* start from scratch, but only download if our local is out of date. */ - curl_easy_setopt(handle->curl, CURLOPT_TIMECONDITION, CURL_TIMECOND_IFMODSINCE); - curl_easy_setopt(handle->curl, CURLOPT_TIMEVALUE, (long)st.st_mtime); - } else if(stat(tempfile, &st) == 0 && payload->allow_resume) { - /* a previous partial download exists, resume from end of file. */ - open_mode = "ab"; - curl_easy_setopt(handle->curl, CURLOPT_RESUME_FROM, (long)st.st_size); - _alpm_log(handle, ALPM_LOG_DEBUG, "tempfile found, attempting continuation\n"); - payload->initial_size = (double)st.st_size; - } - - if(localf == NULL) { - localf = fopen(tempfile, open_mode); - if(localf == NULL) { - goto cleanup; - } + if(curl_set_handle_opts(payload, destfile, tempfile, &localf, &open_mode, + error_buffer) != 0) { + goto cleanup; } - curl_easy_setopt(handle->curl, CURLOPT_WRITEDATA, localf); - /* ignore any SIGPIPE signals- these may occur if our FTP socket dies or * something along those lines. Store the old signal handler first. */ sig_pipe[NEW].sa_handler = SIG_IGN; -- 1.7.6
Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- mask_signal doesn't get inlined here, but unmask does. lib/libalpm/dload.c | 43 +++++++++++++++++++++++++------------------ 1 files changed, 25 insertions(+), 18 deletions(-) diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index 16b2206..f8c10fa 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -67,7 +67,6 @@ static char *get_fullpath(const char *path, const char *filename, } #define check_stop() if(dload_interrupted) { ret = -1; goto cleanup; } -enum sighandlers { OLD = 0, NEW = 1 }; static int dload_interrupted; static void inthandler(int UNUSED signum) @@ -234,6 +233,25 @@ static int curl_set_handle_opts(struct dload_payload *payload, return 0; } +/* set new sighandler, and return the original */ +static struct sigaction mask_signal(int signal, void (*handler)(int)) +{ + struct sigaction newaction, origaction; + + newaction.sa_handler = handler; + sigemptyset(&newaction.sa_mask); + newaction.sa_flags = 0; + + sigaction(signal, NULL, &origaction); + sigaction(signal, &newaction, NULL); + + return origaction; +} + +static void unmask_signal(int signal, struct sigaction sa) +{ + sigaction(signal, &sa, NULL); +} static int curl_download_internal(struct dload_payload *payload, const char *localpath, char **final_file) @@ -247,7 +265,7 @@ static int curl_download_internal(struct dload_payload *payload, char error_buffer[CURL_ERROR_SIZE] = { 0 }; long timecond, remote_time = -1; double remote_size, bytes_dl; - struct sigaction sig_pipe[2], sig_int[2]; + struct sigaction sig_pipe, sig_int; /* shortcut to our handle within the payload */ alpm_handle_t *handle = payload->handle; handle->pm_errno = 0; @@ -294,20 +312,9 @@ static int curl_download_internal(struct dload_payload *payload, goto cleanup; } - /* ignore any SIGPIPE signals- these may occur if our FTP socket dies or - * something along those lines. Store the old signal handler first. */ - sig_pipe[NEW].sa_handler = SIG_IGN; - sigemptyset(&sig_pipe[NEW].sa_mask); - sig_pipe[NEW].sa_flags = 0; - sigaction(SIGPIPE, NULL, &sig_pipe[OLD]); - sigaction(SIGPIPE, &sig_pipe[NEW], NULL); - - dload_interrupted = 0; - sig_int[NEW].sa_handler = &inthandler; - sigemptyset(&sig_int[NEW].sa_mask); - sig_int[NEW].sa_flags = 0; - sigaction(SIGINT, NULL, &sig_int[OLD]); - sigaction(SIGINT, &sig_int[NEW], NULL); + /* ignore SIGPIPE and SIGINT signals while we're transferring */ + sig_pipe = mask_signal(SIGPIPE, SIG_IGN); + sig_int = mask_signal(SIGINT, &inthandler); /* Progress 0 - initialize */ prevprogress = 0; @@ -414,8 +421,8 @@ cleanup: FREE(destfile); /* restore the old signal handlers */ - sigaction(SIGINT, &sig_int[OLD], NULL); - sigaction(SIGPIPE, &sig_pipe[OLD], NULL); + unmask_signal(SIGINT, sig_int); + unmask_signal(SIGPIPE, sig_pipe); /* if we were interrupted, trip the old handler */ if(dload_interrupted) { raise(SIGINT); -- 1.7.6
On Thu, Aug 18, 2011 at 8:09 PM, Dave Reisner <d@falconindy.com> wrote:
Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- mask_signal doesn't get inlined here, but unmask does.
lib/libalpm/dload.c | 43 +++++++++++++++++++++++++------------------ 1 files changed, 25 insertions(+), 18 deletions(-)
diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index 16b2206..f8c10fa 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -67,7 +67,6 @@ static char *get_fullpath(const char *path, const char *filename, }
#define check_stop() if(dload_interrupted) { ret = -1; goto cleanup; } -enum sighandlers { OLD = 0, NEW = 1 };
static int dload_interrupted; static void inthandler(int UNUSED signum) @@ -234,6 +233,25 @@ static int curl_set_handle_opts(struct dload_payload *payload, return 0; }
+/* set new sighandler, and return the original */ +static struct sigaction mask_signal(int signal, void (*handler)(int)) +{ + struct sigaction newaction, origaction; + + newaction.sa_handler = handler; + sigemptyset(&newaction.sa_mask); + newaction.sa_flags = 0; + + sigaction(signal, NULL, &origaction); + sigaction(signal, &newaction, NULL); + + return origaction; +} + +static void unmask_signal(int signal, struct sigaction sa) +{ + sigaction(signal, &sa, NULL); +}
static int curl_download_internal(struct dload_payload *payload, const char *localpath, char **final_file) @@ -247,7 +265,7 @@ static int curl_download_internal(struct dload_payload *payload, char error_buffer[CURL_ERROR_SIZE] = { 0 }; long timecond, remote_time = -1; double remote_size, bytes_dl; - struct sigaction sig_pipe[2], sig_int[2]; + struct sigaction sig_pipe, sig_int; /* shortcut to our handle within the payload */ alpm_handle_t *handle = payload->handle; handle->pm_errno = 0; @@ -294,20 +312,9 @@ static int curl_download_internal(struct dload_payload *payload, goto cleanup; }
- /* ignore any SIGPIPE signals- these may occur if our FTP socket dies or - * something along those lines. Store the old signal handler first. */ - sig_pipe[NEW].sa_handler = SIG_IGN; - sigemptyset(&sig_pipe[NEW].sa_mask); - sig_pipe[NEW].sa_flags = 0; - sigaction(SIGPIPE, NULL, &sig_pipe[OLD]); - sigaction(SIGPIPE, &sig_pipe[NEW], NULL); - - dload_interrupted = 0; - sig_int[NEW].sa_handler = &inthandler; - sigemptyset(&sig_int[NEW].sa_mask); - sig_int[NEW].sa_flags = 0; - sigaction(SIGINT, NULL, &sig_int[OLD]); - sigaction(SIGINT, &sig_int[NEW], NULL); + /* ignore SIGPIPE and SIGINT signals while we're transferring */ + sig_pipe = mask_signal(SIGPIPE, SIG_IGN); + sig_int = mask_signal(SIGINT, &inthandler); Now that we don't have the enum, it would be clearer if these variables were named "orig_sigpipe" or something- it took me a while to figure out why the second var wasn't needed here anymore.
/* Progress 0 - initialize */ prevprogress = 0; @@ -414,8 +421,8 @@ cleanup: FREE(destfile);
/* restore the old signal handlers */ - sigaction(SIGINT, &sig_int[OLD], NULL); - sigaction(SIGPIPE, &sig_pipe[OLD], NULL); + unmask_signal(SIGINT, sig_int); + unmask_signal(SIGPIPE, sig_pipe);
Should we be doing this sooner? e.g. as soon as we are done downloading? Or does this location make sense so that we are atomic in all our download stuff before tripping the original handler right before we return from the function. A question more than knowing the right answer here.
/* if we were interrupted, trip the old handler */ if(dload_interrupted) { raise(SIGINT); -- 1.7.6
Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- lib/libalpm/dload.c | 10 +++++----- 1 files changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index f8c10fa..a7a13b6 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -398,17 +398,17 @@ cleanup: } if(ret == 0) { + const char *realname = tempfile; if (destfile) { + realname = destfile; if(rename(tempfile, destfile)) { _alpm_log(handle, ALPM_LOG_ERROR, _("could not rename %s to %s (%s)\n"), tempfile, destfile, strerror(errno)); ret = -1; - } else if(final_file) { - STRDUP(*final_file, strrchr(destfile, '/') + 1, - RET_ERR(handle, ALPM_ERR_MEMORY, -1)); } - } else if(final_file) { - STRDUP(*final_file, strrchr(tempfile, '/') + 1, + } + if(ret != -1 && final_file) { + STRDUP(*final_file, strrchr(realname, '/') + 1, RET_ERR(handle, ALPM_ERR_MEMORY, -1)); } } -- 1.7.6
ftp and http both define >=400 as being "something bad happened" Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- Fewer .part files left behind... lib/libalpm/dload.c | 8 +++++++- 1 files changed, 7 insertions(+), 1 deletions(-) diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index a7a13b6..f03fabd 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -263,7 +263,7 @@ static int curl_download_internal(struct dload_payload *payload, /* RFC1123 states applications should support this length */ char hostname[256]; char error_buffer[CURL_ERROR_SIZE] = { 0 }; - long timecond, remote_time = -1; + long timecond, respcode = 0, remote_time = -1; double remote_size, bytes_dl; struct sigaction sig_pipe, sig_int; /* shortcut to our handle within the payload */ @@ -328,6 +328,12 @@ static int curl_download_internal(struct dload_payload *payload, /* was it a success? */ switch(handle->curlerr) { case CURLE_OK: + /* get http/ftp response code */ + curl_easy_getinfo(handle->curl, CURLINFO_RESPONSE_CODE, &respcode); + if(respcode >=400) { + payload->unlink_on_fail = 1; + goto cleanup; + } break; case CURLE_ABORTED_BY_CALLBACK: goto cleanup; -- 1.7.6
Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- Fewer .part files left behind... lib/libalpm/dload.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-) diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index f03fabd..4b6ce74 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -266,6 +266,7 @@ static int curl_download_internal(struct dload_payload *payload, long timecond, respcode = 0, remote_time = -1; double remote_size, bytes_dl; struct sigaction sig_pipe, sig_int; + struct stat st; /* shortcut to our handle within the payload */ alpm_handle_t *handle = payload->handle; handle->pm_errno = 0; @@ -338,6 +339,10 @@ static int curl_download_internal(struct dload_payload *payload, case CURLE_ABORTED_BY_CALLBACK: goto cleanup; default: + /* delete zero length downloads */ + if(stat(tempfile, &st) == 0 && st.st_size == 0) { + payload->unlink_on_fail = 1; + } if(!payload->errors_ok) { handle->pm_errno = ALPM_ERR_LIBCURL; _alpm_log(handle, ALPM_LOG_ERROR, _("failed retrieving file '%s' from %s : %s\n"), -- 1.7.6
On Thu, Aug 18, 2011 at 8:09 PM, Dave Reisner <d@falconindy.com> wrote: Commit message explanation would be nice, as until I actually looked at the code, I didn't realize this only applied if we were not in the CURLE_OK case (e.g. some sort of download failure). A 200 OK with 0-byte response will not get deleted, correct?
Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- Fewer .part files left behind...
lib/libalpm/dload.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index f03fabd..4b6ce74 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -266,6 +266,7 @@ static int curl_download_internal(struct dload_payload *payload, long timecond, respcode = 0, remote_time = -1; double remote_size, bytes_dl; struct sigaction sig_pipe, sig_int; + struct stat st; /* shortcut to our handle within the payload */ alpm_handle_t *handle = payload->handle; handle->pm_errno = 0; @@ -338,6 +339,10 @@ static int curl_download_internal(struct dload_payload *payload, case CURLE_ABORTED_BY_CALLBACK: goto cleanup; default: + /* delete zero length downloads */ + if(stat(tempfile, &st) == 0 && st.st_size == 0) { + payload->unlink_on_fail = 1; + } if(!payload->errors_ok) { handle->pm_errno = ALPM_ERR_LIBCURL; _alpm_log(handle, ALPM_LOG_ERROR, _("failed retrieving file '%s' from %s : %s\n"), -- 1.7.6
Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- lib/libalpm/dload.c | 46 +++++++++++++++++++++++++++++----------------- 1 files changed, 29 insertions(+), 17 deletions(-) diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index 4b6ce74..7df66c0 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -253,6 +253,31 @@ static void unmask_signal(int signal, struct sigaction sa) sigaction(signal, &sa, NULL); } +static FILE *create_tempfile(struct dload_payload *payload, + const char *localpath, const char *open_mode, char **filename) +{ + int fd; + FILE *fp; + char randpath[PATH_MAX]; + alpm_handle_t *handle = payload->handle; + + /* create a random filename, which is opened with O_EXCL */ + snprintf(randpath, PATH_MAX, "%salpmtmp.XXXXXX", localpath); + if((fd = mkstemp(randpath)) == -1 || !(fp = fdopen(fd, open_mode))) { + unlink(randpath); + close(fd); + _alpm_log(handle, ALPM_LOG_ERROR, + _("failed to create temporary file for download\n")); + return NULL; + } + + /* localf now points to our alpmtmp.XXXXXX */ + STRDUP(*filename, randpath, RET_ERR(handle, ALPM_ERR_MEMORY, NULL)); + payload->filename = strrchr(randpath, '/') + 1; + + return fp; +} + static int curl_download_internal(struct dload_payload *payload, const char *localpath, char **final_file) { @@ -286,26 +311,13 @@ static int curl_download_internal(struct dload_payload *payload, goto cleanup; } } else { - /* URL isn't to a file and ended with a slash */ - int fd; - char randpath[PATH_MAX]; - - /* we can't support resuming this kind of download, so a partial transfer - * will be destroyed */ + /* URL doesn't contain a filename, so make a temp file. We can't support + * resuming this kind of download; partial transfers will be destroyed */ payload->unlink_on_fail = 1; - - /* create a random filename, which is opened with O_EXCL */ - snprintf(randpath, PATH_MAX, "%salpmtmp.XXXXXX", localpath); - if((fd = mkstemp(randpath)) == -1 || !(localf = fdopen(fd, open_mode))) { - unlink(randpath); - close(fd); - _alpm_log(handle, ALPM_LOG_ERROR, - _("failed to create temporary file for download\n")); + localf = create_tempfile(payload, localpath, open_mode, &tempfile); + if(!localf) { goto cleanup; } - /* localf now points to our alpmtmp.XXXXXX */ - STRDUP(tempfile, randpath, RET_ERR(handle, ALPM_ERR_MEMORY, -1)); - payload->filename = strrchr(randpath, '/') + 1; } if(curl_set_handle_opts(payload, destfile, tempfile, &localf, &open_mode, -- 1.7.6
Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- lib/libalpm/dload.c | 112 ++++++++++++++++++++++++++++----------------------- 1 files changed, 62 insertions(+), 50 deletions(-)
diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index 2a1fb41..16b2206 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -177,18 +177,74 @@ static size_t parse_headers(void *ptr, size_t size, size_t nmemb, void *user) return realsize; }
+static int curl_set_handle_opts(struct dload_payload *payload, + const char *destfile, const char *tempfile, FILE **localf, + const char **open_mode, char *error_buffer) +{ + char *useragent = getenv("HTTP_USER_AGENT"); + alpm_handle_t *handle = payload->handle; + struct stat st; + + /* the curl_easy handle is initialized with the alpm handle, so we only need + * to reset the curl handle set parameters for each time it's used. */ + curl_easy_reset(handle->curl); + curl_easy_setopt(handle->curl, CURLOPT_URL, payload->fileurl); + curl_easy_setopt(handle->curl, CURLOPT_FAILONERROR, 1L); + curl_easy_setopt(handle->curl, CURLOPT_ERRORBUFFER, error_buffer); + curl_easy_setopt(handle->curl, CURLOPT_CONNECTTIMEOUT, 10L); + curl_easy_setopt(handle->curl, CURLOPT_FILETIME, 1L); + curl_easy_setopt(handle->curl, CURLOPT_NOPROGRESS, 0L); + curl_easy_setopt(handle->curl, CURLOPT_FOLLOWLOCATION, 1L); + curl_easy_setopt(handle->curl, CURLOPT_PROGRESSFUNCTION, curl_progress); + curl_easy_setopt(handle->curl, CURLOPT_PROGRESSDATA, (void *)payload); + curl_easy_setopt(handle->curl, CURLOPT_LOW_SPEED_LIMIT, 1024L); + curl_easy_setopt(handle->curl, CURLOPT_LOW_SPEED_TIME, 10L); + curl_easy_setopt(handle->curl, CURLOPT_HEADERFUNCTION, parse_headers); + curl_easy_setopt(handle->curl, CURLOPT_WRITEHEADER, (void *)payload); + + if(payload->max_size) { + curl_easy_setopt(handle->curl, CURLOPT_MAXFILESIZE, payload->max_size); + } + + if(useragent != NULL) { + curl_easy_setopt(handle->curl, CURLOPT_USERAGENT, useragent); + } + + if(!payload->allow_resume && !payload->force && destfile && stat(destfile, &st) == 0) { + /* start from scratch, but only download if our local is out of date. */ + curl_easy_setopt(handle->curl, CURLOPT_TIMECONDITION, CURL_TIMECOND_IFMODSINCE); + curl_easy_setopt(handle->curl, CURLOPT_TIMEVALUE, (long)st.st_mtime); + } else if(stat(tempfile, &st) == 0 && payload->allow_resume) { + /* a previous partial download exists, resume from end of file. */ + *open_mode = "ab"; + curl_easy_setopt(handle->curl, CURLOPT_RESUME_FROM, (long)st.st_size); + _alpm_log(handle, ALPM_LOG_DEBUG, "tempfile found, attempting continuation\n"); + payload->initial_size = (double)st.st_size; + } + + if(*localf == NULL) { + *localf = fopen(tempfile, *open_mode); + if(*localf == NULL) { + return 1; + } This is doing more than {subject} stated here. I'm not sure we should be opening files and such in this method. Is that only so the final setopt WRITEDATA call is moved here? I think it would make more sense to leave that one behind and greatly simplify the argument list of
On Thu, Aug 18, 2011 at 8:09 PM, Dave Reisner <d@falconindy.com> wrote: this method, rather than modifying stack variables of another function.
+ } + + curl_easy_setopt(handle->curl, CURLOPT_WRITEDATA, *localf); + + return 0; +} + + static int curl_download_internal(struct dload_payload *payload, const char *localpath, char **final_file) { int ret = -1; FILE *localf = NULL; - const char *useragent; const char *open_mode = "wb"; char *destfile = NULL, *tempfile = NULL, *effective_url; /* RFC1123 states applications should support this length */ char hostname[256]; - char error_buffer[CURL_ERROR_SIZE]; - struct stat st; + char error_buffer[CURL_ERROR_SIZE] = { 0 }; long timecond, remote_time = -1; double remote_size, bytes_dl; struct sigaction sig_pipe[2], sig_int[2]; @@ -233,55 +289,11 @@ static int curl_download_internal(struct dload_payload *payload, payload->filename = strrchr(randpath, '/') + 1; }
- error_buffer[0] = '\0'; - - /* the curl_easy handle is initialized with the alpm handle, so we only need - * to reset the curl handle set parameters for each time it's used. */ - curl_easy_reset(handle->curl); - curl_easy_setopt(handle->curl, CURLOPT_URL, payload->fileurl); - curl_easy_setopt(handle->curl, CURLOPT_FAILONERROR, 1L); - curl_easy_setopt(handle->curl, CURLOPT_ERRORBUFFER, error_buffer); - curl_easy_setopt(handle->curl, CURLOPT_CONNECTTIMEOUT, 10L); - curl_easy_setopt(handle->curl, CURLOPT_FILETIME, 1L); - curl_easy_setopt(handle->curl, CURLOPT_NOPROGRESS, 0L); - curl_easy_setopt(handle->curl, CURLOPT_FOLLOWLOCATION, 1L); - curl_easy_setopt(handle->curl, CURLOPT_PROGRESSFUNCTION, curl_progress); - curl_easy_setopt(handle->curl, CURLOPT_PROGRESSDATA, (void *)payload); - curl_easy_setopt(handle->curl, CURLOPT_LOW_SPEED_LIMIT, 1024L); - curl_easy_setopt(handle->curl, CURLOPT_LOW_SPEED_TIME, 10L); - curl_easy_setopt(handle->curl, CURLOPT_HEADERFUNCTION, parse_headers); - curl_easy_setopt(handle->curl, CURLOPT_WRITEHEADER, (void *)payload); - - if(payload->max_size) { - curl_easy_setopt(handle->curl, CURLOPT_MAXFILESIZE, payload->max_size); - } - - useragent = getenv("HTTP_USER_AGENT"); - if(useragent != NULL) { - curl_easy_setopt(handle->curl, CURLOPT_USERAGENT, useragent); - } - - if(!payload->allow_resume && !payload->force && destfile && stat(destfile, &st) == 0) { - /* start from scratch, but only download if our local is out of date. */ - curl_easy_setopt(handle->curl, CURLOPT_TIMECONDITION, CURL_TIMECOND_IFMODSINCE); - curl_easy_setopt(handle->curl, CURLOPT_TIMEVALUE, (long)st.st_mtime); - } else if(stat(tempfile, &st) == 0 && payload->allow_resume) { - /* a previous partial download exists, resume from end of file. */ - open_mode = "ab"; - curl_easy_setopt(handle->curl, CURLOPT_RESUME_FROM, (long)st.st_size); - _alpm_log(handle, ALPM_LOG_DEBUG, "tempfile found, attempting continuation\n"); - payload->initial_size = (double)st.st_size; - } - - if(localf == NULL) { - localf = fopen(tempfile, open_mode); - if(localf == NULL) { - goto cleanup; - } + if(curl_set_handle_opts(payload, destfile, tempfile, &localf, &open_mode, + error_buffer) != 0) { + goto cleanup; }
- curl_easy_setopt(handle->curl, CURLOPT_WRITEDATA, localf); - /* ignore any SIGPIPE signals- these may occur if our FTP socket dies or * something along those lines. Store the old signal handler first. */ sig_pipe[NEW].sa_handler = SIG_IGN; -- 1.7.6
On Fri, Aug 19, 2011 at 09:46:42AM -0500, Dan McGee wrote:
Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- lib/libalpm/dload.c | 112 ++++++++++++++++++++++++++++----------------------- 1 files changed, 62 insertions(+), 50 deletions(-)
diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index 2a1fb41..16b2206 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -177,18 +177,74 @@ static size_t parse_headers(void *ptr, size_t size, size_t nmemb, void *user) return realsize; }
+static int curl_set_handle_opts(struct dload_payload *payload, + const char *destfile, const char *tempfile, FILE **localf, + const char **open_mode, char *error_buffer) +{ + char *useragent = getenv("HTTP_USER_AGENT"); + alpm_handle_t *handle = payload->handle; + struct stat st; + + /* the curl_easy handle is initialized with the alpm handle, so we only need + * to reset the curl handle set parameters for each time it's used. */ + curl_easy_reset(handle->curl); + curl_easy_setopt(handle->curl, CURLOPT_URL, payload->fileurl); + curl_easy_setopt(handle->curl, CURLOPT_FAILONERROR, 1L); + curl_easy_setopt(handle->curl, CURLOPT_ERRORBUFFER, error_buffer); + curl_easy_setopt(handle->curl, CURLOPT_CONNECTTIMEOUT, 10L); + curl_easy_setopt(handle->curl, CURLOPT_FILETIME, 1L); + curl_easy_setopt(handle->curl, CURLOPT_NOPROGRESS, 0L); + curl_easy_setopt(handle->curl, CURLOPT_FOLLOWLOCATION, 1L); + curl_easy_setopt(handle->curl, CURLOPT_PROGRESSFUNCTION, curl_progress); + curl_easy_setopt(handle->curl, CURLOPT_PROGRESSDATA, (void *)payload); + curl_easy_setopt(handle->curl, CURLOPT_LOW_SPEED_LIMIT, 1024L); + curl_easy_setopt(handle->curl, CURLOPT_LOW_SPEED_TIME, 10L); + curl_easy_setopt(handle->curl, CURLOPT_HEADERFUNCTION, parse_headers); + curl_easy_setopt(handle->curl, CURLOPT_WRITEHEADER, (void *)payload); + + if(payload->max_size) { + curl_easy_setopt(handle->curl, CURLOPT_MAXFILESIZE, payload->max_size); + } + + if(useragent != NULL) { + curl_easy_setopt(handle->curl, CURLOPT_USERAGENT, useragent); + } + + if(!payload->allow_resume && !payload->force && destfile && stat(destfile, &st) == 0) { + /* start from scratch, but only download if our local is out of date. */ + curl_easy_setopt(handle->curl, CURLOPT_TIMECONDITION, CURL_TIMECOND_IFMODSINCE); + curl_easy_setopt(handle->curl, CURLOPT_TIMEVALUE, (long)st.st_mtime); + } else if(stat(tempfile, &st) == 0 && payload->allow_resume) { + /* a previous partial download exists, resume from end of file. */ + *open_mode = "ab"; + curl_easy_setopt(handle->curl, CURLOPT_RESUME_FROM, (long)st.st_size); + _alpm_log(handle, ALPM_LOG_DEBUG, "tempfile found, attempting continuation\n"); + payload->initial_size = (double)st.st_size; + } + + if(*localf == NULL) { + *localf = fopen(tempfile, *open_mode); + if(*localf == NULL) { + return 1; + } This is doing more than {subject} stated here. I'm not sure we should be opening files and such in this method. Is that only so the final setopt WRITEDATA call is moved here? I think it would make more sense to leave that one behind and greatly simplify the argument list of
On Thu, Aug 18, 2011 at 8:09 PM, Dave Reisner <d@falconindy.com> wrote: this method, rather than modifying stack variables of another function.
Fair point. Perhaps I went a bit overboard here, and I'm all for simplifying the param list a bit here. I've also considered globbing the tempfile, destfile, localf trifecta into a single struct (possibly attached to the payload), but I'm starting to run into variable name hell...
+ } + + curl_easy_setopt(handle->curl, CURLOPT_WRITEDATA, *localf); + + return 0; +} + + static int curl_download_internal(struct dload_payload *payload, const char *localpath, char **final_file) { int ret = -1; FILE *localf = NULL; - const char *useragent; const char *open_mode = "wb"; char *destfile = NULL, *tempfile = NULL, *effective_url; /* RFC1123 states applications should support this length */ char hostname[256]; - char error_buffer[CURL_ERROR_SIZE]; - struct stat st; + char error_buffer[CURL_ERROR_SIZE] = { 0 }; long timecond, remote_time = -1; double remote_size, bytes_dl; struct sigaction sig_pipe[2], sig_int[2]; @@ -233,55 +289,11 @@ static int curl_download_internal(struct dload_payload *payload, payload->filename = strrchr(randpath, '/') + 1; }
- error_buffer[0] = '\0'; - - /* the curl_easy handle is initialized with the alpm handle, so we only need - * to reset the curl handle set parameters for each time it's used. */ - curl_easy_reset(handle->curl); - curl_easy_setopt(handle->curl, CURLOPT_URL, payload->fileurl); - curl_easy_setopt(handle->curl, CURLOPT_FAILONERROR, 1L); - curl_easy_setopt(handle->curl, CURLOPT_ERRORBUFFER, error_buffer); - curl_easy_setopt(handle->curl, CURLOPT_CONNECTTIMEOUT, 10L); - curl_easy_setopt(handle->curl, CURLOPT_FILETIME, 1L); - curl_easy_setopt(handle->curl, CURLOPT_NOPROGRESS, 0L); - curl_easy_setopt(handle->curl, CURLOPT_FOLLOWLOCATION, 1L); - curl_easy_setopt(handle->curl, CURLOPT_PROGRESSFUNCTION, curl_progress); - curl_easy_setopt(handle->curl, CURLOPT_PROGRESSDATA, (void *)payload); - curl_easy_setopt(handle->curl, CURLOPT_LOW_SPEED_LIMIT, 1024L); - curl_easy_setopt(handle->curl, CURLOPT_LOW_SPEED_TIME, 10L); - curl_easy_setopt(handle->curl, CURLOPT_HEADERFUNCTION, parse_headers); - curl_easy_setopt(handle->curl, CURLOPT_WRITEHEADER, (void *)payload); - - if(payload->max_size) { - curl_easy_setopt(handle->curl, CURLOPT_MAXFILESIZE, payload->max_size); - } - - useragent = getenv("HTTP_USER_AGENT"); - if(useragent != NULL) { - curl_easy_setopt(handle->curl, CURLOPT_USERAGENT, useragent); - } - - if(!payload->allow_resume && !payload->force && destfile && stat(destfile, &st) == 0) { - /* start from scratch, but only download if our local is out of date. */ - curl_easy_setopt(handle->curl, CURLOPT_TIMECONDITION, CURL_TIMECOND_IFMODSINCE); - curl_easy_setopt(handle->curl, CURLOPT_TIMEVALUE, (long)st.st_mtime); - } else if(stat(tempfile, &st) == 0 && payload->allow_resume) { - /* a previous partial download exists, resume from end of file. */ - open_mode = "ab"; - curl_easy_setopt(handle->curl, CURLOPT_RESUME_FROM, (long)st.st_size); - _alpm_log(handle, ALPM_LOG_DEBUG, "tempfile found, attempting continuation\n"); - payload->initial_size = (double)st.st_size; - } - - if(localf == NULL) { - localf = fopen(tempfile, open_mode); - if(localf == NULL) { - goto cleanup; - } + if(curl_set_handle_opts(payload, destfile, tempfile, &localf, &open_mode, + error_buffer) != 0) { + goto cleanup; }
- curl_easy_setopt(handle->curl, CURLOPT_WRITEDATA, localf); - /* ignore any SIGPIPE signals- these may occur if our FTP socket dies or * something along those lines. Store the old signal handler first. */ sig_pipe[NEW].sa_handler = SIG_IGN; -- 1.7.6
On Fri, Aug 19, 2011 at 9:55 AM, Dave Reisner <d@falconindy.com> wrote:
On Fri, Aug 19, 2011 at 09:46:42AM -0500, Dan McGee wrote:
This is doing more than {subject} stated here. I'm not sure we should be opening files and such in this method. Is that only so the final setopt WRITEDATA call is moved here? I think it would make more sense to leave that one behind and greatly simplify the argument list of this method, rather than modifying stack variables of another function.
Fair point. Perhaps I went a bit overboard here, and I'm all for simplifying the param list a bit here. I've also considered globbing the tempfile, destfile, localf trifecta into a single struct (possibly attached to the payload), but I'm starting to run into variable name hell...
The payload is "private" anyway, so I don't think adding these fields to the payload is necessarily a bad thing. -Dan
participants (2)
-
Dan McGee
-
Dave Reisner