Re: [pacman-dev] [PATCH] Cleanup the old sequential download code
Hi On Wed, Jun 10, 2020 at 8:12 PM Allan McRae wrote: > > On 12/5/20 3:16 am, Anatol Pomozov wrote: > > All users of _alpm_download() have been refactored to the new API. > > It is time to remove the old _alpm_download() functionality now. > > > > This change also removes obsolete SIGPIPE signal handler functionality > > (this is a leftover from libfetch days). > > > > Signed-off-by: Anatol Pomozov > > --- > > lib/libalpm/dload.c | 323 +--- > > lib/libalpm/dload.h | 4 - > > 2 files changed, 3 insertions(+), 324 deletions(-) > > > > diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c > > index 43fe9847..4dbb011f 100644 > > --- a/lib/libalpm/dload.c > > +++ b/lib/libalpm/dload.c > > @@ -78,10 +78,6 @@ enum { > > }; > > > > static int dload_interrupted; > > -static void inthandler(int UNUSED signum) > > -{ > > - dload_interrupted = ABORT_SIGINT; > > -} > > > > static int dload_progress_cb(void *file, curl_off_t dltotal, curl_off_t > > dlnow, > > curl_off_t UNUSED ultotal, curl_off_t UNUSED ulnow) > > @@ -236,8 +232,7 @@ static size_t dload_parseheader_cb(void *ptr, size_t > > size, size_t nmemb, void *u > > return realsize; > > } > > > > -static void curl_set_handle_opts(struct dload_payload *payload, > > - CURL *curl, char *error_buffer) > > +static void curl_set_handle_opts(CURL *curl, struct dload_payload *payload) > > { > > This change seems beyond the scope for this patch. Can you split that > and the other related changes below into a separate patch. Inlining "error_buffer" is related to the "serial download" code cleanup. The old (serial) codepath has been using a stack variable error_buffer and was passing it to curl_set_handle_opts() function as a parameter. With the concurrent download we cannot share a stack variable between all payloads, instead we initialize a separate buffer for each payload. Now "error_buffer is part of dload_payload{}. With this serial codepath removed we don't have any local variable for error_buffer. And we don't need to pass "error_buffer" to curl_set_handle_opts() - "error_buffer" is already part of *payload parameter. This parameter inlining organically belongs to this refactoring IMO.
Re: [pacman-dev] [PATCH] Cleanup the old sequential download code
On 12/5/20 3:16 am, Anatol Pomozov wrote: > All users of _alpm_download() have been refactored to the new API. > It is time to remove the old _alpm_download() functionality now. > > This change also removes obsolete SIGPIPE signal handler functionality > (this is a leftover from libfetch days). > > Signed-off-by: Anatol Pomozov > --- > lib/libalpm/dload.c | 323 +--- > lib/libalpm/dload.h | 4 - > 2 files changed, 3 insertions(+), 324 deletions(-) > > diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c > index 43fe9847..4dbb011f 100644 > --- a/lib/libalpm/dload.c > +++ b/lib/libalpm/dload.c > @@ -78,10 +78,6 @@ enum { > }; > > static int dload_interrupted; > -static void inthandler(int UNUSED signum) > -{ > - dload_interrupted = ABORT_SIGINT; > -} > > static int dload_progress_cb(void *file, curl_off_t dltotal, curl_off_t > dlnow, > curl_off_t UNUSED ultotal, curl_off_t UNUSED ulnow) > @@ -236,8 +232,7 @@ static size_t dload_parseheader_cb(void *ptr, size_t > size, size_t nmemb, void *u > return realsize; > } > > -static void curl_set_handle_opts(struct dload_payload *payload, > - CURL *curl, char *error_buffer) > +static void curl_set_handle_opts(CURL *curl, struct dload_payload *payload) > { This change seems beyond the scope for this patch. Can you split that and the other related changes below into a separate patch. Otherwise fine. A
Re: [pacman-dev] [PATCH] Cleanup the old sequential download code
On 14/5/20 6:03 pm, Anatol Pomozov wrote: > Given that in other patches we rename new functions to its old name it > makes sense to rename _alpm_multi_download to _alpm_download. That would make sense. A
Re: [pacman-dev] [PATCH] Cleanup the old sequential download code
Hi On Mon, May 11, 2020 at 10:16 AM Anatol Pomozov wrote: > > All users of _alpm_download() have been refactored to the new API. > It is time to remove the old _alpm_download() functionality now. > > This change also removes obsolete SIGPIPE signal handler functionality > (this is a leftover from libfetch days). > > Signed-off-by: Anatol Pomozov > --- > lib/libalpm/dload.c | 323 +--- > lib/libalpm/dload.h | 4 - > 2 files changed, 3 insertions(+), 324 deletions(-) > > diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c > index 43fe9847..4dbb011f 100644 > --- a/lib/libalpm/dload.c > +++ b/lib/libalpm/dload.c > @@ -78,10 +78,6 @@ enum { > }; > > static int dload_interrupted; > -static void inthandler(int UNUSED signum) > -{ > - dload_interrupted = ABORT_SIGINT; > -} > > static int dload_progress_cb(void *file, curl_off_t dltotal, curl_off_t > dlnow, > curl_off_t UNUSED ultotal, curl_off_t UNUSED ulnow) > @@ -236,8 +232,7 @@ static size_t dload_parseheader_cb(void *ptr, size_t > size, size_t nmemb, void *u > return realsize; > } > > -static void curl_set_handle_opts(struct dload_payload *payload, > - CURL *curl, char *error_buffer) > +static void curl_set_handle_opts(CURL *curl, struct dload_payload *payload) > { > alpm_handle_t *handle = payload->handle; > const char *useragent = getenv("HTTP_USER_AGENT"); > @@ -247,7 +242,7 @@ static void curl_set_handle_opts(struct dload_payload > *payload, > * to reset the handle's parameters for each time it's used. */ > curl_easy_reset(curl); > curl_easy_setopt(curl, CURLOPT_URL, payload->fileurl); > - curl_easy_setopt(curl, CURLOPT_ERRORBUFFER, error_buffer); > + curl_easy_setopt(curl, CURLOPT_ERRORBUFFER, payload->error_buffer); > curl_easy_setopt(curl, CURLOPT_CONNECTTIMEOUT, 10L); > curl_easy_setopt(curl, CURLOPT_MAXREDIRS, 10L); > curl_easy_setopt(curl, CURLOPT_FILETIME, 1L); > @@ -301,24 +296,6 @@ static void curl_set_handle_opts(struct dload_payload > *payload, > } > } > > -static void mask_signal(int signum, void (*handler)(int), > - struct sigaction *origaction) > -{ > - struct sigaction newaction; > - > - newaction.sa_handler = handler; > - sigemptyset(&newaction.sa_mask); > - newaction.sa_flags = 0; > - > - sigaction(signum, NULL, origaction); > - sigaction(signum, &newaction, NULL); > -} > - > -static void unmask_signal(int signum, struct sigaction *sa) > -{ > - sigaction(signum, sa, NULL); > -} > - > static FILE *create_tempfile(struct dload_payload *payload, const char > *localpath) > { > int fd; > @@ -353,259 +330,6 @@ static FILE *create_tempfile(struct dload_payload > *payload, const char *localpat > /* RFC1123 states applications should support this length */ > #define HOSTNAME_SIZE 256 > > -static int curl_download_internal(struct dload_payload *payload, > - const char *localpath, char **final_file, const char > **final_url) > -{ > - int ret = -1; > - FILE *localf = NULL; > - char *effective_url; > - char hostname[HOSTNAME_SIZE]; > - char error_buffer[CURL_ERROR_SIZE] = {0}; > - struct stat st; > - long timecond, remote_time = -1; > - double remote_size, bytes_dl; > - struct sigaction orig_sig_pipe, orig_sig_int; > - CURLcode curlerr; > - alpm_download_event_init_t init_cb_data = {0}; > - alpm_download_event_completed_t completed_cb_data = {0}; > - /* shortcut to our handle within the payload */ > - alpm_handle_t *handle = payload->handle; > - CURL *curl = curl_easy_init(); > - handle->pm_errno = ALPM_ERR_OK; > - payload->curl = curl; > - > - /* make sure these are NULL */ > - FREE(payload->tempfile_name); > - FREE(payload->destfile_name); > - FREE(payload->content_disp_name); > - > - payload->tempfile_openmode = "wb"; > - if(!payload->remote_name) { > - STRDUP(payload->remote_name, get_filename(payload->fileurl), > - RET_ERR(handle, ALPM_ERR_MEMORY, -1)); > - } > - if(curl_gethost(payload->fileurl, hostname, sizeof(hostname)) != 0) { > - _alpm_log(handle, ALPM_LOG_ERROR, _("url '%s' is invalid\n"), > payload->fileurl); > - RET_ERR(handle, ALPM_ERR_SERVER_BAD_URL, -1); > - } > - > - if(payload->remote_name && strlen(payload->remote_name) > 0 && > - strcmp(payload->remote_name, ".sig") != 0) { > - payload->destfile_name = get_fullpath(localpath, > payload->remote_name, ""); > - payload->tempfile_name = get_fullpath(localpath, > payload->remote_name, ".part"); > - if(!payload->destfile_name || !payload->tempfile_name) { > - goto cleanup; > - } > -
[pacman-dev] [PATCH] Cleanup the old sequential download code
All users of _alpm_download() have been refactored to the new API. It is time to remove the old _alpm_download() functionality now. This change also removes obsolete SIGPIPE signal handler functionality (this is a leftover from libfetch days). Signed-off-by: Anatol Pomozov --- lib/libalpm/dload.c | 323 +--- lib/libalpm/dload.h | 4 - 2 files changed, 3 insertions(+), 324 deletions(-) diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index 43fe9847..4dbb011f 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -78,10 +78,6 @@ enum { }; static int dload_interrupted; -static void inthandler(int UNUSED signum) -{ - dload_interrupted = ABORT_SIGINT; -} static int dload_progress_cb(void *file, curl_off_t dltotal, curl_off_t dlnow, curl_off_t UNUSED ultotal, curl_off_t UNUSED ulnow) @@ -236,8 +232,7 @@ static size_t dload_parseheader_cb(void *ptr, size_t size, size_t nmemb, void *u return realsize; } -static void curl_set_handle_opts(struct dload_payload *payload, - CURL *curl, char *error_buffer) +static void curl_set_handle_opts(CURL *curl, struct dload_payload *payload) { alpm_handle_t *handle = payload->handle; const char *useragent = getenv("HTTP_USER_AGENT"); @@ -247,7 +242,7 @@ static void curl_set_handle_opts(struct dload_payload *payload, * to reset the handle's parameters for each time it's used. */ curl_easy_reset(curl); curl_easy_setopt(curl, CURLOPT_URL, payload->fileurl); - curl_easy_setopt(curl, CURLOPT_ERRORBUFFER, error_buffer); + curl_easy_setopt(curl, CURLOPT_ERRORBUFFER, payload->error_buffer); curl_easy_setopt(curl, CURLOPT_CONNECTTIMEOUT, 10L); curl_easy_setopt(curl, CURLOPT_MAXREDIRS, 10L); curl_easy_setopt(curl, CURLOPT_FILETIME, 1L); @@ -301,24 +296,6 @@ static void curl_set_handle_opts(struct dload_payload *payload, } } -static void mask_signal(int signum, void (*handler)(int), - struct sigaction *origaction) -{ - struct sigaction newaction; - - newaction.sa_handler = handler; - sigemptyset(&newaction.sa_mask); - newaction.sa_flags = 0; - - sigaction(signum, NULL, origaction); - sigaction(signum, &newaction, NULL); -} - -static void unmask_signal(int signum, struct sigaction *sa) -{ - sigaction(signum, sa, NULL); -} - static FILE *create_tempfile(struct dload_payload *payload, const char *localpath) { int fd; @@ -353,259 +330,6 @@ static FILE *create_tempfile(struct dload_payload *payload, const char *localpat /* RFC1123 states applications should support this length */ #define HOSTNAME_SIZE 256 -static int curl_download_internal(struct dload_payload *payload, - const char *localpath, char **final_file, const char **final_url) -{ - int ret = -1; - FILE *localf = NULL; - char *effective_url; - char hostname[HOSTNAME_SIZE]; - char error_buffer[CURL_ERROR_SIZE] = {0}; - struct stat st; - long timecond, remote_time = -1; - double remote_size, bytes_dl; - struct sigaction orig_sig_pipe, orig_sig_int; - CURLcode curlerr; - alpm_download_event_init_t init_cb_data = {0}; - alpm_download_event_completed_t completed_cb_data = {0}; - /* shortcut to our handle within the payload */ - alpm_handle_t *handle = payload->handle; - CURL *curl = curl_easy_init(); - handle->pm_errno = ALPM_ERR_OK; - payload->curl = curl; - - /* make sure these are NULL */ - FREE(payload->tempfile_name); - FREE(payload->destfile_name); - FREE(payload->content_disp_name); - - payload->tempfile_openmode = "wb"; - if(!payload->remote_name) { - STRDUP(payload->remote_name, get_filename(payload->fileurl), - RET_ERR(handle, ALPM_ERR_MEMORY, -1)); - } - if(curl_gethost(payload->fileurl, hostname, sizeof(hostname)) != 0) { - _alpm_log(handle, ALPM_LOG_ERROR, _("url '%s' is invalid\n"), payload->fileurl); - RET_ERR(handle, ALPM_ERR_SERVER_BAD_URL, -1); - } - - if(payload->remote_name && strlen(payload->remote_name) > 0 && - strcmp(payload->remote_name, ".sig") != 0) { - payload->destfile_name = get_fullpath(localpath, payload->remote_name, ""); - payload->tempfile_name = get_fullpath(localpath, payload->remote_name, ".part"); - if(!payload->destfile_name || !payload->tempfile_name) { - goto cleanup; - } - } else { - /* URL doesn't contain a filename, so make a tempfile. We can't support -* resuming this kind of download; partial transfers will be destroyed */ - payload->unlink_on_fail = 1; - - localf = create_tempfile(payload, localpath); -