[pacman-dev] Move signature payload creation to download engine

Message ID 20200513221526.208663-1-anatol.pomozov@gmail.com
State Superseded, archived
Headers show
Series [pacman-dev] Move signature payload creation to download engine | expand

Commit Message

Anatol Pomozov May 13, 2020, 10:15 p.m. UTC
Until now callee of ALPM download functionality has been in charge of
payload creation both for the main file (e.g. *.pkg) and for the accompanied
*.sig file. One advantage of such solution is that all payloads are
independent and can be fetched in parallel thus exploiting the maximum
level of download parallelism.

To build *.sig file url we've been using a simple string concatenation:
$requested_url + ".sig". Unfortunately there are cases when it does not
work. For example an archlinux.org "Download From Mirror" link looks like
this https://www.archlinux.org/packages/core/x86_64/bash/download/ and
it gets redirected to some mirror. But if we append ".sig" to the end of
the link url and try to download it then archlinux.org returns 404 error.

To overcome this issue we need to follow redirects for the main payload
first, find the final url and only then append '.sig' suffix.
This implies 2 things:
 - the signature payload initialization need to be moved to dload.c
 as it is the place where we have access to the resolved url
 - *.sig is downloaded serially with the main payload and this reduces
 level of parallelism

Move *.sig payload creation to dload.c. Once the main payload is fetched
successfully we check if the callee asked to download the accompanied
signature. If yes - create a new payload and add it to mcurl.

*.sig payload does not use server list of the main payload and thus does
not support mirror failover. *.sig file comes from the same server as
the main payload.

Refactor event loop in curl_multi_download_internal() a bit. Instead of
relying on curl_multi_check_finished_download() to return number of new
payloads we simply rerun the loop iteration one more time to check if
there are any active downloads left.
---
 lib/libalpm/be_sync.c | 34 +++-------------
 lib/libalpm/dload.c   | 91 ++++++++++++++++++++++++++-----------------
 lib/libalpm/dload.h   |  4 +-
 3 files changed, 65 insertions(+), 64 deletions(-)

Comments

Allan McRae June 11, 2020, 4:32 a.m. UTC | #1
On 14/5/20 8:15 am, Anatol Pomozov wrote:
> Until now callee of ALPM download functionality has been in charge of
> payload creation both for the main file (e.g. *.pkg) and for the accompanied
> *.sig file. One advantage of such solution is that all payloads are
> independent and can be fetched in parallel thus exploiting the maximum
> level of download parallelism.
> 
> To build *.sig file url we've been using a simple string concatenation:
> $requested_url + ".sig". Unfortunately there are cases when it does not
> work. For example an archlinux.org "Download From Mirror" link looks like
> this https://www.archlinux.org/packages/core/x86_64/bash/download/ and
> it gets redirected to some mirror. But if we append ".sig" to the end of
> the link url and try to download it then archlinux.org returns 404 error.
> 
> To overcome this issue we need to follow redirects for the main payload
> first, find the final url and only then append '.sig' suffix.
> This implies 2 things:
>  - the signature payload initialization need to be moved to dload.c
>  as it is the place where we have access to the resolved url
>  - *.sig is downloaded serially with the main payload and this reduces
>  level of parallelism
> 
> Move *.sig payload creation to dload.c. Once the main payload is fetched
> successfully we check if the callee asked to download the accompanied
> signature. If yes - create a new payload and add it to mcurl.
> 
> *.sig payload does not use server list of the main payload and thus does
> not support mirror failover. *.sig file comes from the same server as
> the main payload.
> 
> Refactor event loop in curl_multi_download_internal() a bit. Instead of
> relying on curl_multi_check_finished_download() to return number of new
> payloads we simply rerun the loop iteration one more time to check if
> there are any active downloads left.
> ---
>  lib/libalpm/be_sync.c | 34 +++-------------
>  lib/libalpm/dload.c   | 91 ++++++++++++++++++++++++++-----------------
>  lib/libalpm/dload.h   |  4 +-
>  3 files changed, 65 insertions(+), 64 deletions(-)
> 
> diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c
> index 82018e15..41675d21 100644
> --- a/lib/libalpm/be_sync.c
> +++ b/lib/libalpm/be_sync.c
> @@ -180,12 +180,10 @@ int SYMEXPORT alpm_db_update(alpm_handle_t *handle, alpm_list_t *dbs, int force)
>  			dbforce = 1;
>  		}
>  
> -		CALLOC(payload, 1, sizeof(*payload), GOTO_ERR(handle, ALPM_ERR_MEMORY, cleanup));
> +		siglevel = alpm_db_get_siglevel(db);
>  
> -		/* set hard upper limit of 128 MiB */
> -		payload->max_size = 128 * 1024 * 1024;
> +		CALLOC(payload, 1, sizeof(*payload), GOTO_ERR(handle, ALPM_ERR_MEMORY, cleanup));
>  		payload->servers = db->servers;
> -
>  		/* print server + filename into a buffer */
>  		len = strlen(db->treename) + strlen(dbext) + 1;
>  		MALLOC(payload->filepath, len,
> @@ -194,31 +192,11 @@ int SYMEXPORT alpm_db_update(alpm_handle_t *handle, alpm_list_t *dbs, int force)
>  		payload->handle = handle;
>  		payload->force = dbforce;
>  		payload->unlink_on_fail = 1;
> -
> +		payload->download_signature = (siglevel & ALPM_SIG_DATABASE);
> +		payload->signature_optional = (siglevel & ALPM_SIG_DATABASE_OPTIONAL);
> +		/* set hard upper limit of 128 MiB */
> +		payload->max_size = 128 * 1024 * 1024;
>  		payloads = alpm_list_add(payloads, payload);

OK.

> -
> -		siglevel = alpm_db_get_siglevel(db);
> -		if(siglevel & ALPM_SIG_DATABASE) {
> -			struct dload_payload *sig_payload;
> -			CALLOC(sig_payload, 1, sizeof(*sig_payload), GOTO_ERR(handle, ALPM_ERR_MEMORY, cleanup));
> -			sig_payload->signature = 1;
> -
> -			/* print filename into a buffer (leave space for separator and .sig) */
> -			len = strlen(db->treename) + strlen(dbext) + 5;
> -			MALLOC(sig_payload->filepath, len,
> -				FREE(sig_payload); GOTO_ERR(handle, ALPM_ERR_MEMORY, cleanup));
> -			snprintf(sig_payload->filepath, len, "%s%s.sig", db->treename, dbext);
> -
> -			sig_payload->handle = handle;
> -			sig_payload->force = dbforce;
> -			sig_payload->errors_ok = (siglevel & ALPM_SIG_DATABASE_OPTIONAL);
> -
> -			/* set hard upper limit of 16 KiB */
> -			sig_payload->max_size = 16 * 1024;
> -			sig_payload->servers = db->servers;
> -
> -			payloads = alpm_list_add(payloads, sig_payload);
> -		}
>  	}
>  
>  	event.type = ALPM_EVENT_DB_RETRIEVE_START;

OK.

> diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c
> index 4dbb011f..b68dcf6d 100644
> --- a/lib/libalpm/dload.c
> +++ b/lib/libalpm/dload.c
> @@ -18,6 +18,7 @@
>   *  along with this program.  If not, see <http://www.gnu.org/licenses/>.
>   */
>  
> +#include <stdbool.h>

See below.

>  #include <stdlib.h>
>  #include <stdio.h>
>  #include <errno.h>
> @@ -49,6 +50,10 @@
>  #include "handle.h"
>  
>  #ifdef HAVE_LIBCURL
> +
> +static int curl_multi_add_payload(alpm_handle_t *handle, CURLM *curlm,
> +	struct dload_payload *payload, const char *localpath);
> +

OK.

>  static const char *get_filename(const char *url)
>  {
>  	char *filename = strrchr(url, '/');
> @@ -476,6 +481,25 @@ static int curl_multi_check_finished_download(CURLM *curlm, CURLMsg *msg,
>  	curl_easy_getinfo(curl, CURLINFO_CONDITION_UNMET, &timecond);
>  	curl_easy_getinfo(curl, CURLINFO_EFFECTIVE_URL, &effective_url);
>  
> +	/* Let's check if client requested downloading accompanion *.sig file */
> +	if(!payload->signature && payload->download_signature && curlerr == CURLE_OK && payload->respcode < 400) {
> +		struct dload_payload *sig = NULL;
> +
> +		int len = strlen(effective_url) + 5;
> +		CALLOC(sig, 1, sizeof(*sig), GOTO_ERR(handle, ALPM_ERR_MEMORY, cleanup));
> +		MALLOC(sig->fileurl, len, FREE(sig); GOTO_ERR(handle, ALPM_ERR_MEMORY, cleanup));
> +		snprintf(sig->fileurl, len, "%s.sig", effective_url);
> +		sig->signature = 1;
> +		sig->handle = handle;
> +		sig->force = payload->force;
> +		sig->unlink_on_fail = payload->unlink_on_fail;
> +		sig->errors_ok = payload->signature_optional;
> +		/* set hard upper limit of 16KiB */
> +		sig->max_size = 16 * 1024;
> +
> +		curl_multi_add_payload(handle, curlm, sig, localpath);
> +	}
> +

OK.

>  	/* time condition was met and we didn't download anything. we need to
>  	 * clean up the 0 byte .part file that's left behind. */
>  	if(timecond == 1 && DOUBLE_EQ(bytes_dl, 0)) {
> @@ -565,6 +589,13 @@ cleanup:
>  	if(ret == -1 && payload->errors_ok) {
>  		ret = -2;
>  	}
> +
> +	if(payload->signature) {
> +		/* free signature payload memory that was allocated earlier in dload.c */
> +		_alpm_dload_payload_reset(payload);
> +		FREE(payload);
> +	}
> +

OK.

>  	return ret;
>  }
>  
> @@ -669,12 +700,12 @@ static int curl_multi_download_internal(alpm_handle_t *handle,
>  	int still_running = 0;
>  	int err = 0;
>  	int parallel_downloads = handle->parallel_downloads;
> -
>  	CURLM *curlm = handle->curlm;
> -	CURLMsg *msg;
> +	bool msg_done = false;
>  

This is a weird mix of bool and int...  still_running is 0/1.  msg_done
is true/false.f.

I know stdbool started to be used in pacman with this patch series, but
I think it should be removed from here and a more comprehensive made
later on so not to have a mix.

> -	while(still_running || payloads) {
> -		int msgs_left = -1;
> +	while(still_running || payloads || msg_done) {
> +		int msgs_left = 0;
> +		CURLMcode mc;
>  
>  		for(; still_running < parallel_downloads && payloads; still_running++) {
>  			struct dload_payload *payload = payloads->data;
> @@ -687,15 +718,19 @@ static int curl_multi_download_internal(alpm_handle_t *handle,
>  
>  				payloads = payloads->next;
>  			} else {
> -				// the payload failed to start, do not start any new downloads just wait until
> -				// active one complete.
> +				/* The payload failed to start. Do not start any new downloads.
> +				 * Wait until all active downloads complete.
> +				 */

Not directly related to this patch, but OK...

>  				_alpm_log(handle, ALPM_LOG_ERROR, _("failed to setup a download payload for %s\n"), payload->remote_name);
>  				payloads = NULL;
>  				err = -1;
>  			}
>  		}
>  
> -		CURLMcode mc = curl_multi_perform(curlm, &still_running);
> +		mc = curl_multi_perform(curlm, &still_running);
> +		if(mc == CURLM_OK) {
> +			mc = curl_multi_wait(curlm, NULL, 0, 1000, NULL);
> +		}

OK.

>  
>  		if(mc != CURLM_OK) {
>  			_alpm_log(handle, ALPM_LOG_ERROR, _("curl returned error %d from transfer\n"), mc);
> @@ -703,28 +738,28 @@ static int curl_multi_download_internal(alpm_handle_t *handle,
>  			err = -1;
>  		}
>  
> -		while((msg = curl_multi_info_read(curlm, &msgs_left))) {
> +		msg_done = false;
> +		while(true) {
> +			CURLMsg *msg = curl_multi_info_read(curlm, &msgs_left);
> +			if(!msg) {
> +				break;
> +			}

OK.

>  			if(msg->msg == CURLMSG_DONE) {
>  				int ret = curl_multi_check_finished_download(curlm, msg, localpath);
>  				if(ret == -1) {
> -					/* if current payload failed to download then stop adding new payloads but wait for the
> -					 * current ones
> -					 */
> +					/* if current payload failed to download then stop adding new payloads */

Fine.

>  					payloads = NULL;
>  					err = -1;
> -				} else if(ret == 2) {
> -					/* in case of a retry increase the counter of active requests
> -					 * to avoid exiting the loop early
> -					 */
> -					still_running++;
>  				}

Where is this going?

> +				/* curl_multi_check_finished_download() might add more payloads e.g. in case of a retry
> +				 * from the next mirror. We need to execute curl_multi_perform() at least one more time
> +				 * to make sure new payload requests are processed.
> +				 */
> +				msg_done = true;

This is a weird variable name now I have figured out what it does.  My
understanding is that this is really a flag to actually check that
everything is done. I know it is related to the current CURLMsg being
complete.  Can it be renamed something like "recheck_downloads"?

>  			} else {
>  				_alpm_log(handle, ALPM_LOG_ERROR, _("curl transfer error: %d\n"), msg->msg);
>  			}
>  		}
> -		if(still_running) {
> -			curl_multi_wait(curlm, NULL, 0, 1000, NULL);
> -		}
>  	}
>  
>  	_alpm_log(handle, ALPM_LOG_DEBUG, "curl_multi_download_internal return code is %d\n", err);
> @@ -803,12 +838,10 @@ int SYMEXPORT alpm_fetch_pkgurl(alpm_handle_t *handle, const alpm_list_t *urls,
>  {
>  	const char *cachedir;
>  	alpm_list_t *payloads = NULL;
> -	int download_sigs;
>  	const alpm_list_t *i;
>  	alpm_event_t event;
>  
>  	CHECK_HANDLE(handle, return -1);
> -	download_sigs = handle->siglevel & ALPM_SIG_PACKAGE;
>  
>  	/* find a valid cache dir to download to */
>  	cachedir = _alpm_filecache_setup(handle);
> @@ -830,21 +863,9 @@ int SYMEXPORT alpm_fetch_pkgurl(alpm_handle_t *handle, const alpm_list_t *urls,
>  			payload->allow_resume = 1;
>  			payload->handle = handle;
>  			payload->trust_remote_name = 1;
> +			payload->download_signature = (handle->siglevel & ALPM_SIG_PACKAGE);
> +			payload->signature_optional = (handle->siglevel & ALPM_SIG_PACKAGE_OPTIONAL);
>  			payloads = alpm_list_add(payloads, payload);
> -
> -			if(download_sigs) {
> -				int len = strlen(url) + 5;
> -				CALLOC(payload, 1, sizeof(*payload), GOTO_ERR(handle, ALPM_ERR_MEMORY, err));
> -				payload->signature = 1;
> -				MALLOC(payload->fileurl, len, FREE(payload); GOTO_ERR(handle, ALPM_ERR_MEMORY, err));
> -				snprintf(payload->fileurl, len, "%s.sig", url);
> -				payload->handle = handle;
> -				payload->trust_remote_name = 1;
> -				payload->errors_ok = (handle->siglevel & ALPM_SIG_PACKAGE_OPTIONAL);
> -				/* set hard upper limit of 16KiB */
> -				payload->max_size = 16 * 1024;
> -				payloads = alpm_list_add(payloads, payload);
> -			}

OK.

>  		}
>  	}
>  
> diff --git a/lib/libalpm/dload.h b/lib/libalpm/dload.h
> index d13bc1b5..facafca2 100644
> --- a/lib/libalpm/dload.h
> +++ b/lib/libalpm/dload.h
> @@ -47,11 +47,13 @@ struct dload_payload {
>  	int errors_ok;
>  	int unlink_on_fail;
>  	int trust_remote_name;
> -	int signature; /* specifies if the payload is a signature file */
> +	int download_signature; /* specifies if an accompanion *.sig file need to be downloaded*/
> +	int signature_optional; /* *.sig file is optional */
>  #ifdef HAVE_LIBCURL
>  	CURL *curl;
>  	char error_buffer[CURL_ERROR_SIZE];
>  	FILE *localf; /* temp download file */
> +	int signature; /* specifies if this payload is for a signature file */
>  #endif
>  };
>  
>
Anatol Pomozov June 23, 2020, 6:35 a.m. UTC | #2
Hi

On Wed, Jun 10, 2020 at 9:33 PM Allan McRae <allan@archlinux.org> wrote:
>
> On 14/5/20 8:15 am, Anatol Pomozov wrote:
> > Until now callee of ALPM download functionality has been in charge of
> > payload creation both for the main file (e.g. *.pkg) and for the accompanied
> > *.sig file. One advantage of such solution is that all payloads are
> > independent and can be fetched in parallel thus exploiting the maximum
> > level of download parallelism.
> >
> > To build *.sig file url we've been using a simple string concatenation:
> > $requested_url + ".sig". Unfortunately there are cases when it does not
> > work. For example an archlinux.org "Download From Mirror" link looks like
> > this https://www.archlinux.org/packages/core/x86_64/bash/download/ and
> > it gets redirected to some mirror. But if we append ".sig" to the end of
> > the link url and try to download it then archlinux.org returns 404 error.
> >
> > To overcome this issue we need to follow redirects for the main payload
> > first, find the final url and only then append '.sig' suffix.
> > This implies 2 things:
> >  - the signature payload initialization need to be moved to dload.c
> >  as it is the place where we have access to the resolved url
> >  - *.sig is downloaded serially with the main payload and this reduces
> >  level of parallelism
> >
> > Move *.sig payload creation to dload.c. Once the main payload is fetched
> > successfully we check if the callee asked to download the accompanied
> > signature. If yes - create a new payload and add it to mcurl.
> >
> > *.sig payload does not use server list of the main payload and thus does
> > not support mirror failover. *.sig file comes from the same server as
> > the main payload.
> >
> > Refactor event loop in curl_multi_download_internal() a bit. Instead of
> > relying on curl_multi_check_finished_download() to return number of new
> > payloads we simply rerun the loop iteration one more time to check if
> > there are any active downloads left.
> > ---
> >  lib/libalpm/be_sync.c | 34 +++-------------
> >  lib/libalpm/dload.c   | 91 ++++++++++++++++++++++++++-----------------
> >  lib/libalpm/dload.h   |  4 +-
> >  3 files changed, 65 insertions(+), 64 deletions(-)
> >
> > diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c
> > index 82018e15..41675d21 100644
> > --- a/lib/libalpm/be_sync.c
> > +++ b/lib/libalpm/be_sync.c
> > @@ -180,12 +180,10 @@ int SYMEXPORT alpm_db_update(alpm_handle_t *handle, alpm_list_t *dbs, int force)
> >                       dbforce = 1;
> >               }
> >
> > -             CALLOC(payload, 1, sizeof(*payload), GOTO_ERR(handle, ALPM_ERR_MEMORY, cleanup));
> > +             siglevel = alpm_db_get_siglevel(db);
> >
> > -             /* set hard upper limit of 128 MiB */
> > -             payload->max_size = 128 * 1024 * 1024;
> > +             CALLOC(payload, 1, sizeof(*payload), GOTO_ERR(handle, ALPM_ERR_MEMORY, cleanup));
> >               payload->servers = db->servers;
> > -
> >               /* print server + filename into a buffer */
> >               len = strlen(db->treename) + strlen(dbext) + 1;
> >               MALLOC(payload->filepath, len,
> > @@ -194,31 +192,11 @@ int SYMEXPORT alpm_db_update(alpm_handle_t *handle, alpm_list_t *dbs, int force)
> >               payload->handle = handle;
> >               payload->force = dbforce;
> >               payload->unlink_on_fail = 1;
> > -
> > +             payload->download_signature = (siglevel & ALPM_SIG_DATABASE);
> > +             payload->signature_optional = (siglevel & ALPM_SIG_DATABASE_OPTIONAL);
> > +             /* set hard upper limit of 128 MiB */
> > +             payload->max_size = 128 * 1024 * 1024;
> >               payloads = alpm_list_add(payloads, payload);
>
> OK.
>
> > -
> > -             siglevel = alpm_db_get_siglevel(db);
> > -             if(siglevel & ALPM_SIG_DATABASE) {
> > -                     struct dload_payload *sig_payload;
> > -                     CALLOC(sig_payload, 1, sizeof(*sig_payload), GOTO_ERR(handle, ALPM_ERR_MEMORY, cleanup));
> > -                     sig_payload->signature = 1;
> > -
> > -                     /* print filename into a buffer (leave space for separator and .sig) */
> > -                     len = strlen(db->treename) + strlen(dbext) + 5;
> > -                     MALLOC(sig_payload->filepath, len,
> > -                             FREE(sig_payload); GOTO_ERR(handle, ALPM_ERR_MEMORY, cleanup));
> > -                     snprintf(sig_payload->filepath, len, "%s%s.sig", db->treename, dbext);
> > -
> > -                     sig_payload->handle = handle;
> > -                     sig_payload->force = dbforce;
> > -                     sig_payload->errors_ok = (siglevel & ALPM_SIG_DATABASE_OPTIONAL);
> > -
> > -                     /* set hard upper limit of 16 KiB */
> > -                     sig_payload->max_size = 16 * 1024;
> > -                     sig_payload->servers = db->servers;
> > -
> > -                     payloads = alpm_list_add(payloads, sig_payload);
> > -             }
> >       }
> >
> >       event.type = ALPM_EVENT_DB_RETRIEVE_START;
>
> OK.
>
> > diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c
> > index 4dbb011f..b68dcf6d 100644
> > --- a/lib/libalpm/dload.c
> > +++ b/lib/libalpm/dload.c
> > @@ -18,6 +18,7 @@
> >   *  along with this program.  If not, see <http://www.gnu.org/licenses/>.
> >   */
> >
> > +#include <stdbool.h>
>
> See below.
>
> >  #include <stdlib.h>
> >  #include <stdio.h>
> >  #include <errno.h>
> > @@ -49,6 +50,10 @@
> >  #include "handle.h"
> >
> >  #ifdef HAVE_LIBCURL
> > +
> > +static int curl_multi_add_payload(alpm_handle_t *handle, CURLM *curlm,
> > +     struct dload_payload *payload, const char *localpath);
> > +
>
> OK.
>
> >  static const char *get_filename(const char *url)
> >  {
> >       char *filename = strrchr(url, '/');
> > @@ -476,6 +481,25 @@ static int curl_multi_check_finished_download(CURLM *curlm, CURLMsg *msg,
> >       curl_easy_getinfo(curl, CURLINFO_CONDITION_UNMET, &timecond);
> >       curl_easy_getinfo(curl, CURLINFO_EFFECTIVE_URL, &effective_url);
> >
> > +     /* Let's check if client requested downloading accompanion *.sig file */
> > +     if(!payload->signature && payload->download_signature && curlerr == CURLE_OK && payload->respcode < 400) {
> > +             struct dload_payload *sig = NULL;
> > +
> > +             int len = strlen(effective_url) + 5;
> > +             CALLOC(sig, 1, sizeof(*sig), GOTO_ERR(handle, ALPM_ERR_MEMORY, cleanup));
> > +             MALLOC(sig->fileurl, len, FREE(sig); GOTO_ERR(handle, ALPM_ERR_MEMORY, cleanup));
> > +             snprintf(sig->fileurl, len, "%s.sig", effective_url);
> > +             sig->signature = 1;
> > +             sig->handle = handle;
> > +             sig->force = payload->force;
> > +             sig->unlink_on_fail = payload->unlink_on_fail;
> > +             sig->errors_ok = payload->signature_optional;
> > +             /* set hard upper limit of 16KiB */
> > +             sig->max_size = 16 * 1024;
> > +
> > +             curl_multi_add_payload(handle, curlm, sig, localpath);
> > +     }
> > +
>
> OK.
>
> >       /* time condition was met and we didn't download anything. we need to
> >        * clean up the 0 byte .part file that's left behind. */
> >       if(timecond == 1 && DOUBLE_EQ(bytes_dl, 0)) {
> > @@ -565,6 +589,13 @@ cleanup:
> >       if(ret == -1 && payload->errors_ok) {
> >               ret = -2;
> >       }
> > +
> > +     if(payload->signature) {
> > +             /* free signature payload memory that was allocated earlier in dload.c */
> > +             _alpm_dload_payload_reset(payload);
> > +             FREE(payload);
> > +     }
> > +
>
> OK.
>
> >       return ret;
> >  }
> >
> > @@ -669,12 +700,12 @@ static int curl_multi_download_internal(alpm_handle_t *handle,
> >       int still_running = 0;
> >       int err = 0;
> >       int parallel_downloads = handle->parallel_downloads;
> > -
> >       CURLM *curlm = handle->curlm;
> > -     CURLMsg *msg;
> > +     bool msg_done = false;
> >
>
> This is a weird mix of bool and int...  still_running is 0/1.

"still_running" is actually a positive int and represents a number of
active downloads.
We use it to make sure that the number of concurrent downloads does not overflow
"ParallelDownloads" configuration value.

Let me know if you have ideas for a better name for this variable.

>  msg_done
> is true/false.f.
>
> I know stdbool started to be used in pacman with this patch series, but
> I think it should be removed from here and a more comprehensive made
> later on so not to have a mix.
>
> > -     while(still_running || payloads) {
> > -             int msgs_left = -1;
> > +     while(still_running || payloads || msg_done) {
> > +             int msgs_left = 0;
> > +             CURLMcode mc;
> >
> >               for(; still_running < parallel_downloads && payloads; still_running++) {
> >                       struct dload_payload *payload = payloads->data;
> > @@ -687,15 +718,19 @@ static int curl_multi_download_internal(alpm_handle_t *handle,
> >
> >                               payloads = payloads->next;
> >                       } else {
> > -                             // the payload failed to start, do not start any new downloads just wait until
> > -                             // active one complete.
> > +                             /* The payload failed to start. Do not start any new downloads.
> > +                              * Wait until all active downloads complete.
> > +                              */
>
> Not directly related to this patch, but OK...
>
> >                               _alpm_log(handle, ALPM_LOG_ERROR, _("failed to setup a download payload for %s\n"), payload->remote_name);
> >                               payloads = NULL;
> >                               err = -1;
> >                       }
> >               }
> >
> > -             CURLMcode mc = curl_multi_perform(curlm, &still_running);
> > +             mc = curl_multi_perform(curlm, &still_running);
> > +             if(mc == CURLM_OK) {
> > +                     mc = curl_multi_wait(curlm, NULL, 0, 1000, NULL);
> > +             }
>
> OK.
>
> >
> >               if(mc != CURLM_OK) {
> >                       _alpm_log(handle, ALPM_LOG_ERROR, _("curl returned error %d from transfer\n"), mc);
> > @@ -703,28 +738,28 @@ static int curl_multi_download_internal(alpm_handle_t *handle,
> >                       err = -1;
> >               }
> >
> > -             while((msg = curl_multi_info_read(curlm, &msgs_left))) {
> > +             msg_done = false;
> > +             while(true) {
> > +                     CURLMsg *msg = curl_multi_info_read(curlm, &msgs_left);
> > +                     if(!msg) {
> > +                             break;
> > +                     }
>
> OK.
>
> >                       if(msg->msg == CURLMSG_DONE) {
> >                               int ret = curl_multi_check_finished_download(curlm, msg, localpath);

One more thing that can be cleaned up is return value for
curl_multi_check_finished_download().
Currently the function returns 5 different error codes but in fact now
we only care if download had any
fatal errors or not. i.e. it can return 0/-1 codes only and it will
simplify curl_multi_check_finished_download() a bit.

> >                               if(ret == -1) {
> > -                                     /* if current payload failed to download then stop adding new payloads but wait for the
> > -                                      * current ones
> > -                                      */
> > +                                     /* if current payload failed to download then stop adding new payloads */
>
> Fine.
>
> >                                       payloads = NULL;
> >                                       err = -1;
> > -                             } else if(ret == 2) {
> > -                                     /* in case of a retry increase the counter of active requests
> > -                                      * to avoid exiting the loop early
> > -                                      */
> > -                                     still_running++;
> >                               }
>
> Where is this going?
>
> > +                             /* curl_multi_check_finished_download() might add more payloads e.g. in case of a retry
> > +                              * from the next mirror. We need to execute curl_multi_perform() at least one more time
> > +                              * to make sure new payload requests are processed.
> > +                              */
> > +                             msg_done = true;
>
> This is a weird variable name now I have figured out what it does.  My
> understanding is that this is really a flag to actually check that
> everything is done. I know it is related to the current CURLMsg being
> complete.  Can it be renamed something like "recheck_downloads"?

Done.

> >                       } else {
> >                               _alpm_log(handle, ALPM_LOG_ERROR, _("curl transfer error: %d\n"), msg->msg);
> >                       }
> >               }
> > -             if(still_running) {
> > -                     curl_multi_wait(curlm, NULL, 0, 1000, NULL);
> > -             }
> >       }
> >
> >       _alpm_log(handle, ALPM_LOG_DEBUG, "curl_multi_download_internal return code is %d\n", err);
> > @@ -803,12 +838,10 @@ int SYMEXPORT alpm_fetch_pkgurl(alpm_handle_t *handle, const alpm_list_t *urls,
> >  {
> >       const char *cachedir;
> >       alpm_list_t *payloads = NULL;
> > -     int download_sigs;
> >       const alpm_list_t *i;
> >       alpm_event_t event;
> >
> >       CHECK_HANDLE(handle, return -1);
> > -     download_sigs = handle->siglevel & ALPM_SIG_PACKAGE;
> >
> >       /* find a valid cache dir to download to */
> >       cachedir = _alpm_filecache_setup(handle);
> > @@ -830,21 +863,9 @@ int SYMEXPORT alpm_fetch_pkgurl(alpm_handle_t *handle, const alpm_list_t *urls,
> >                       payload->allow_resume = 1;
> >                       payload->handle = handle;
> >                       payload->trust_remote_name = 1;
> > +                     payload->download_signature = (handle->siglevel & ALPM_SIG_PACKAGE);
> > +                     payload->signature_optional = (handle->siglevel & ALPM_SIG_PACKAGE_OPTIONAL);
> >                       payloads = alpm_list_add(payloads, payload);
> > -
> > -                     if(download_sigs) {
> > -                             int len = strlen(url) + 5;
> > -                             CALLOC(payload, 1, sizeof(*payload), GOTO_ERR(handle, ALPM_ERR_MEMORY, err));
> > -                             payload->signature = 1;
> > -                             MALLOC(payload->fileurl, len, FREE(payload); GOTO_ERR(handle, ALPM_ERR_MEMORY, err));
> > -                             snprintf(payload->fileurl, len, "%s.sig", url);
> > -                             payload->handle = handle;
> > -                             payload->trust_remote_name = 1;
> > -                             payload->errors_ok = (handle->siglevel & ALPM_SIG_PACKAGE_OPTIONAL);
> > -                             /* set hard upper limit of 16KiB */
> > -                             payload->max_size = 16 * 1024;
> > -                             payloads = alpm_list_add(payloads, payload);
> > -                     }
>
> OK.
>
> >               }
> >       }
> >
> > diff --git a/lib/libalpm/dload.h b/lib/libalpm/dload.h
> > index d13bc1b5..facafca2 100644
> > --- a/lib/libalpm/dload.h
> > +++ b/lib/libalpm/dload.h
> > @@ -47,11 +47,13 @@ struct dload_payload {
> >       int errors_ok;
> >       int unlink_on_fail;
> >       int trust_remote_name;
> > -     int signature; /* specifies if the payload is a signature file */
> > +     int download_signature; /* specifies if an accompanion *.sig file need to be downloaded*/
> > +     int signature_optional; /* *.sig file is optional */
> >  #ifdef HAVE_LIBCURL
> >       CURL *curl;
> >       char error_buffer[CURL_ERROR_SIZE];
> >       FILE *localf; /* temp download file */
> > +     int signature; /* specifies if this payload is for a signature file */
> >  #endif
> >  };
> >
> >
Anatol Pomozov June 23, 2020, 5:54 p.m. UTC | #3
Hi

On Mon, Jun 22, 2020 at 11:35 PM Anatol Pomozov
<anatol.pomozov@gmail.com> wrote:
>
> Hi
>
> On Wed, Jun 10, 2020 at 9:33 PM Allan McRae <allan@archlinux.org> wrote:
> >
> > On 14/5/20 8:15 am, Anatol Pomozov wrote:
> > > Until now callee of ALPM download functionality has been in charge of
> > > payload creation both for the main file (e.g. *.pkg) and for the accompanied
> > > *.sig file. One advantage of such solution is that all payloads are
> > > independent and can be fetched in parallel thus exploiting the maximum
> > > level of download parallelism.
> > >
> > > To build *.sig file url we've been using a simple string concatenation:
> > > $requested_url + ".sig". Unfortunately there are cases when it does not
> > > work. For example an archlinux.org "Download From Mirror" link looks like
> > > this https://www.archlinux.org/packages/core/x86_64/bash/download/ and
> > > it gets redirected to some mirror. But if we append ".sig" to the end of
> > > the link url and try to download it then archlinux.org returns 404 error.
> > >
> > > To overcome this issue we need to follow redirects for the main payload
> > > first, find the final url and only then append '.sig' suffix.
> > > This implies 2 things:
> > >  - the signature payload initialization need to be moved to dload.c
> > >  as it is the place where we have access to the resolved url
> > >  - *.sig is downloaded serially with the main payload and this reduces
> > >  level of parallelism
> > >
> > > Move *.sig payload creation to dload.c. Once the main payload is fetched
> > > successfully we check if the callee asked to download the accompanied
> > > signature. If yes - create a new payload and add it to mcurl.
> > >
> > > *.sig payload does not use server list of the main payload and thus does
> > > not support mirror failover. *.sig file comes from the same server as
> > > the main payload.
> > >
> > > Refactor event loop in curl_multi_download_internal() a bit. Instead of
> > > relying on curl_multi_check_finished_download() to return number of new
> > > payloads we simply rerun the loop iteration one more time to check if
> > > there are any active downloads left.
> > > ---
> > >  lib/libalpm/be_sync.c | 34 +++-------------
> > >  lib/libalpm/dload.c   | 91 ++++++++++++++++++++++++++-----------------
> > >  lib/libalpm/dload.h   |  4 +-
> > >  3 files changed, 65 insertions(+), 64 deletions(-)
> > >
> > > diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c
> > > index 82018e15..41675d21 100644
> > > --- a/lib/libalpm/be_sync.c
> > > +++ b/lib/libalpm/be_sync.c
> > > @@ -180,12 +180,10 @@ int SYMEXPORT alpm_db_update(alpm_handle_t *handle, alpm_list_t *dbs, int force)
> > >                       dbforce = 1;
> > >               }
> > >
> > > -             CALLOC(payload, 1, sizeof(*payload), GOTO_ERR(handle, ALPM_ERR_MEMORY, cleanup));
> > > +             siglevel = alpm_db_get_siglevel(db);
> > >
> > > -             /* set hard upper limit of 128 MiB */
> > > -             payload->max_size = 128 * 1024 * 1024;
> > > +             CALLOC(payload, 1, sizeof(*payload), GOTO_ERR(handle, ALPM_ERR_MEMORY, cleanup));
> > >               payload->servers = db->servers;
> > > -
> > >               /* print server + filename into a buffer */
> > >               len = strlen(db->treename) + strlen(dbext) + 1;
> > >               MALLOC(payload->filepath, len,
> > > @@ -194,31 +192,11 @@ int SYMEXPORT alpm_db_update(alpm_handle_t *handle, alpm_list_t *dbs, int force)
> > >               payload->handle = handle;
> > >               payload->force = dbforce;
> > >               payload->unlink_on_fail = 1;
> > > -
> > > +             payload->download_signature = (siglevel & ALPM_SIG_DATABASE);
> > > +             payload->signature_optional = (siglevel & ALPM_SIG_DATABASE_OPTIONAL);
> > > +             /* set hard upper limit of 128 MiB */
> > > +             payload->max_size = 128 * 1024 * 1024;
> > >               payloads = alpm_list_add(payloads, payload);
> >
> > OK.
> >
> > > -
> > > -             siglevel = alpm_db_get_siglevel(db);
> > > -             if(siglevel & ALPM_SIG_DATABASE) {
> > > -                     struct dload_payload *sig_payload;
> > > -                     CALLOC(sig_payload, 1, sizeof(*sig_payload), GOTO_ERR(handle, ALPM_ERR_MEMORY, cleanup));
> > > -                     sig_payload->signature = 1;
> > > -
> > > -                     /* print filename into a buffer (leave space for separator and .sig) */
> > > -                     len = strlen(db->treename) + strlen(dbext) + 5;
> > > -                     MALLOC(sig_payload->filepath, len,
> > > -                             FREE(sig_payload); GOTO_ERR(handle, ALPM_ERR_MEMORY, cleanup));
> > > -                     snprintf(sig_payload->filepath, len, "%s%s.sig", db->treename, dbext);
> > > -
> > > -                     sig_payload->handle = handle;
> > > -                     sig_payload->force = dbforce;
> > > -                     sig_payload->errors_ok = (siglevel & ALPM_SIG_DATABASE_OPTIONAL);
> > > -
> > > -                     /* set hard upper limit of 16 KiB */
> > > -                     sig_payload->max_size = 16 * 1024;
> > > -                     sig_payload->servers = db->servers;
> > > -
> > > -                     payloads = alpm_list_add(payloads, sig_payload);
> > > -             }
> > >       }
> > >
> > >       event.type = ALPM_EVENT_DB_RETRIEVE_START;
> >
> > OK.
> >
> > > diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c
> > > index 4dbb011f..b68dcf6d 100644
> > > --- a/lib/libalpm/dload.c
> > > +++ b/lib/libalpm/dload.c
> > > @@ -18,6 +18,7 @@
> > >   *  along with this program.  If not, see <http://www.gnu.org/licenses/>.
> > >   */
> > >
> > > +#include <stdbool.h>
> >
> > See below.
> >
> > >  #include <stdlib.h>
> > >  #include <stdio.h>
> > >  #include <errno.h>
> > > @@ -49,6 +50,10 @@
> > >  #include "handle.h"
> > >
> > >  #ifdef HAVE_LIBCURL
> > > +
> > > +static int curl_multi_add_payload(alpm_handle_t *handle, CURLM *curlm,
> > > +     struct dload_payload *payload, const char *localpath);
> > > +
> >
> > OK.
> >
> > >  static const char *get_filename(const char *url)
> > >  {
> > >       char *filename = strrchr(url, '/');
> > > @@ -476,6 +481,25 @@ static int curl_multi_check_finished_download(CURLM *curlm, CURLMsg *msg,
> > >       curl_easy_getinfo(curl, CURLINFO_CONDITION_UNMET, &timecond);
> > >       curl_easy_getinfo(curl, CURLINFO_EFFECTIVE_URL, &effective_url);
> > >
> > > +     /* Let's check if client requested downloading accompanion *.sig file */
> > > +     if(!payload->signature && payload->download_signature && curlerr == CURLE_OK && payload->respcode < 400) {
> > > +             struct dload_payload *sig = NULL;
> > > +
> > > +             int len = strlen(effective_url) + 5;
> > > +             CALLOC(sig, 1, sizeof(*sig), GOTO_ERR(handle, ALPM_ERR_MEMORY, cleanup));
> > > +             MALLOC(sig->fileurl, len, FREE(sig); GOTO_ERR(handle, ALPM_ERR_MEMORY, cleanup));
> > > +             snprintf(sig->fileurl, len, "%s.sig", effective_url);
> > > +             sig->signature = 1;
> > > +             sig->handle = handle;
> > > +             sig->force = payload->force;
> > > +             sig->unlink_on_fail = payload->unlink_on_fail;
> > > +             sig->errors_ok = payload->signature_optional;
> > > +             /* set hard upper limit of 16KiB */
> > > +             sig->max_size = 16 * 1024;
> > > +
> > > +             curl_multi_add_payload(handle, curlm, sig, localpath);
> > > +     }
> > > +
> >
> > OK.
> >
> > >       /* time condition was met and we didn't download anything. we need to
> > >        * clean up the 0 byte .part file that's left behind. */
> > >       if(timecond == 1 && DOUBLE_EQ(bytes_dl, 0)) {
> > > @@ -565,6 +589,13 @@ cleanup:
> > >       if(ret == -1 && payload->errors_ok) {
> > >               ret = -2;
> > >       }
> > > +
> > > +     if(payload->signature) {
> > > +             /* free signature payload memory that was allocated earlier in dload.c */
> > > +             _alpm_dload_payload_reset(payload);
> > > +             FREE(payload);
> > > +     }
> > > +
> >
> > OK.
> >
> > >       return ret;
> > >  }
> > >
> > > @@ -669,12 +700,12 @@ static int curl_multi_download_internal(alpm_handle_t *handle,
> > >       int still_running = 0;
> > >       int err = 0;
> > >       int parallel_downloads = handle->parallel_downloads;
> > > -
> > >       CURLM *curlm = handle->curlm;
> > > -     CURLMsg *msg;
> > > +     bool msg_done = false;
> > >
> >
> > This is a weird mix of bool and int...  still_running is 0/1.
>
> "still_running" is actually a positive int and represents a number of
> active downloads.
> We use it to make sure that the number of concurrent downloads does not overflow
> "ParallelDownloads" configuration value.
>
> Let me know if you have ideas for a better name for this variable.

I am thinking of renaming variables
"still_running"->"active_downloads_num" and
"parallel_downloads"->"max_downloads". Hopefully it will make the code
a bit more readable.

>
> >  msg_done
> > is true/false.f.
> >
> > I know stdbool started to be used in pacman with this patch series, but
> > I think it should be removed from here and a more comprehensive made
> > later on so not to have a mix.
> >
> > > -     while(still_running || payloads) {
> > > -             int msgs_left = -1;
> > > +     while(still_running || payloads || msg_done) {
> > > +             int msgs_left = 0;
> > > +             CURLMcode mc;
> > >
> > >               for(; still_running < parallel_downloads && payloads; still_running++) {
> > >                       struct dload_payload *payload = payloads->data;
> > > @@ -687,15 +718,19 @@ static int curl_multi_download_internal(alpm_handle_t *handle,
> > >
> > >                               payloads = payloads->next;
> > >                       } else {
> > > -                             // the payload failed to start, do not start any new downloads just wait until
> > > -                             // active one complete.
> > > +                             /* The payload failed to start. Do not start any new downloads.
> > > +                              * Wait until all active downloads complete.
> > > +                              */
> >
> > Not directly related to this patch, but OK...
> >
> > >                               _alpm_log(handle, ALPM_LOG_ERROR, _("failed to setup a download payload for %s\n"), payload->remote_name);
> > >                               payloads = NULL;
> > >                               err = -1;
> > >                       }
> > >               }
> > >
> > > -             CURLMcode mc = curl_multi_perform(curlm, &still_running);
> > > +             mc = curl_multi_perform(curlm, &still_running);
> > > +             if(mc == CURLM_OK) {
> > > +                     mc = curl_multi_wait(curlm, NULL, 0, 1000, NULL);
> > > +             }
> >
> > OK.
> >
> > >
> > >               if(mc != CURLM_OK) {
> > >                       _alpm_log(handle, ALPM_LOG_ERROR, _("curl returned error %d from transfer\n"), mc);
> > > @@ -703,28 +738,28 @@ static int curl_multi_download_internal(alpm_handle_t *handle,
> > >                       err = -1;
> > >               }
> > >
> > > -             while((msg = curl_multi_info_read(curlm, &msgs_left))) {
> > > +             msg_done = false;
> > > +             while(true) {
> > > +                     CURLMsg *msg = curl_multi_info_read(curlm, &msgs_left);
> > > +                     if(!msg) {
> > > +                             break;
> > > +                     }
> >
> > OK.
> >
> > >                       if(msg->msg == CURLMSG_DONE) {
> > >                               int ret = curl_multi_check_finished_download(curlm, msg, localpath);
>
> One more thing that can be cleaned up is return value for
> curl_multi_check_finished_download().
> Currently the function returns 5 different error codes but in fact now
> we only care if download had any
> fatal errors or not. i.e. it can return 0/-1 codes only and it will
> simplify curl_multi_check_finished_download() a bit.
>
> > >                               if(ret == -1) {
> > > -                                     /* if current payload failed to download then stop adding new payloads but wait for the
> > > -                                      * current ones
> > > -                                      */
> > > +                                     /* if current payload failed to download then stop adding new payloads */
> >
> > Fine.
> >
> > >                                       payloads = NULL;
> > >                                       err = -1;
> > > -                             } else if(ret == 2) {
> > > -                                     /* in case of a retry increase the counter of active requests
> > > -                                      * to avoid exiting the loop early
> > > -                                      */
> > > -                                     still_running++;
> > >                               }
> >
> > Where is this going?
> >
> > > +                             /* curl_multi_check_finished_download() might add more payloads e.g. in case of a retry
> > > +                              * from the next mirror. We need to execute curl_multi_perform() at least one more time
> > > +                              * to make sure new payload requests are processed.
> > > +                              */
> > > +                             msg_done = true;
> >
> > This is a weird variable name now I have figured out what it does.  My
> > understanding is that this is really a flag to actually check that
> > everything is done. I know it is related to the current CURLMsg being
> > complete.  Can it be renamed something like "recheck_downloads"?
>
> Done.
>
> > >                       } else {
> > >                               _alpm_log(handle, ALPM_LOG_ERROR, _("curl transfer error: %d\n"), msg->msg);
> > >                       }
> > >               }
> > > -             if(still_running) {
> > > -                     curl_multi_wait(curlm, NULL, 0, 1000, NULL);
> > > -             }
> > >       }
> > >
> > >       _alpm_log(handle, ALPM_LOG_DEBUG, "curl_multi_download_internal return code is %d\n", err);
> > > @@ -803,12 +838,10 @@ int SYMEXPORT alpm_fetch_pkgurl(alpm_handle_t *handle, const alpm_list_t *urls,
> > >  {
> > >       const char *cachedir;
> > >       alpm_list_t *payloads = NULL;
> > > -     int download_sigs;
> > >       const alpm_list_t *i;
> > >       alpm_event_t event;
> > >
> > >       CHECK_HANDLE(handle, return -1);
> > > -     download_sigs = handle->siglevel & ALPM_SIG_PACKAGE;
> > >
> > >       /* find a valid cache dir to download to */
> > >       cachedir = _alpm_filecache_setup(handle);
> > > @@ -830,21 +863,9 @@ int SYMEXPORT alpm_fetch_pkgurl(alpm_handle_t *handle, const alpm_list_t *urls,
> > >                       payload->allow_resume = 1;
> > >                       payload->handle = handle;
> > >                       payload->trust_remote_name = 1;
> > > +                     payload->download_signature = (handle->siglevel & ALPM_SIG_PACKAGE);
> > > +                     payload->signature_optional = (handle->siglevel & ALPM_SIG_PACKAGE_OPTIONAL);
> > >                       payloads = alpm_list_add(payloads, payload);
> > > -
> > > -                     if(download_sigs) {
> > > -                             int len = strlen(url) + 5;
> > > -                             CALLOC(payload, 1, sizeof(*payload), GOTO_ERR(handle, ALPM_ERR_MEMORY, err));
> > > -                             payload->signature = 1;
> > > -                             MALLOC(payload->fileurl, len, FREE(payload); GOTO_ERR(handle, ALPM_ERR_MEMORY, err));
> > > -                             snprintf(payload->fileurl, len, "%s.sig", url);
> > > -                             payload->handle = handle;
> > > -                             payload->trust_remote_name = 1;
> > > -                             payload->errors_ok = (handle->siglevel & ALPM_SIG_PACKAGE_OPTIONAL);
> > > -                             /* set hard upper limit of 16KiB */
> > > -                             payload->max_size = 16 * 1024;
> > > -                             payloads = alpm_list_add(payloads, payload);
> > > -                     }
> >
> > OK.
> >
> > >               }
> > >       }
> > >
> > > diff --git a/lib/libalpm/dload.h b/lib/libalpm/dload.h
> > > index d13bc1b5..facafca2 100644
> > > --- a/lib/libalpm/dload.h
> > > +++ b/lib/libalpm/dload.h
> > > @@ -47,11 +47,13 @@ struct dload_payload {
> > >       int errors_ok;
> > >       int unlink_on_fail;
> > >       int trust_remote_name;
> > > -     int signature; /* specifies if the payload is a signature file */
> > > +     int download_signature; /* specifies if an accompanion *.sig file need to be downloaded*/
> > > +     int signature_optional; /* *.sig file is optional */
> > >  #ifdef HAVE_LIBCURL
> > >       CURL *curl;
> > >       char error_buffer[CURL_ERROR_SIZE];
> > >       FILE *localf; /* temp download file */
> > > +     int signature; /* specifies if this payload is for a signature file */
> > >  #endif
> > >  };
> > >
> > >

Patch

diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c
index 82018e15..41675d21 100644
--- a/lib/libalpm/be_sync.c
+++ b/lib/libalpm/be_sync.c
@@ -180,12 +180,10 @@  int SYMEXPORT alpm_db_update(alpm_handle_t *handle, alpm_list_t *dbs, int force)
 			dbforce = 1;
 		}
 
-		CALLOC(payload, 1, sizeof(*payload), GOTO_ERR(handle, ALPM_ERR_MEMORY, cleanup));
+		siglevel = alpm_db_get_siglevel(db);
 
-		/* set hard upper limit of 128 MiB */
-		payload->max_size = 128 * 1024 * 1024;
+		CALLOC(payload, 1, sizeof(*payload), GOTO_ERR(handle, ALPM_ERR_MEMORY, cleanup));
 		payload->servers = db->servers;
-
 		/* print server + filename into a buffer */
 		len = strlen(db->treename) + strlen(dbext) + 1;
 		MALLOC(payload->filepath, len,
@@ -194,31 +192,11 @@  int SYMEXPORT alpm_db_update(alpm_handle_t *handle, alpm_list_t *dbs, int force)
 		payload->handle = handle;
 		payload->force = dbforce;
 		payload->unlink_on_fail = 1;
-
+		payload->download_signature = (siglevel & ALPM_SIG_DATABASE);
+		payload->signature_optional = (siglevel & ALPM_SIG_DATABASE_OPTIONAL);
+		/* set hard upper limit of 128 MiB */
+		payload->max_size = 128 * 1024 * 1024;
 		payloads = alpm_list_add(payloads, payload);
-
-		siglevel = alpm_db_get_siglevel(db);
-		if(siglevel & ALPM_SIG_DATABASE) {
-			struct dload_payload *sig_payload;
-			CALLOC(sig_payload, 1, sizeof(*sig_payload), GOTO_ERR(handle, ALPM_ERR_MEMORY, cleanup));
-			sig_payload->signature = 1;
-
-			/* print filename into a buffer (leave space for separator and .sig) */
-			len = strlen(db->treename) + strlen(dbext) + 5;
-			MALLOC(sig_payload->filepath, len,
-				FREE(sig_payload); GOTO_ERR(handle, ALPM_ERR_MEMORY, cleanup));
-			snprintf(sig_payload->filepath, len, "%s%s.sig", db->treename, dbext);
-
-			sig_payload->handle = handle;
-			sig_payload->force = dbforce;
-			sig_payload->errors_ok = (siglevel & ALPM_SIG_DATABASE_OPTIONAL);
-
-			/* set hard upper limit of 16 KiB */
-			sig_payload->max_size = 16 * 1024;
-			sig_payload->servers = db->servers;
-
-			payloads = alpm_list_add(payloads, sig_payload);
-		}
 	}
 
 	event.type = ALPM_EVENT_DB_RETRIEVE_START;
diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c
index 4dbb011f..b68dcf6d 100644
--- a/lib/libalpm/dload.c
+++ b/lib/libalpm/dload.c
@@ -18,6 +18,7 @@ 
  *  along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <stdbool.h>
 #include <stdlib.h>
 #include <stdio.h>
 #include <errno.h>
@@ -49,6 +50,10 @@ 
 #include "handle.h"
 
 #ifdef HAVE_LIBCURL
+
+static int curl_multi_add_payload(alpm_handle_t *handle, CURLM *curlm,
+	struct dload_payload *payload, const char *localpath);
+
 static const char *get_filename(const char *url)
 {
 	char *filename = strrchr(url, '/');
@@ -476,6 +481,25 @@  static int curl_multi_check_finished_download(CURLM *curlm, CURLMsg *msg,
 	curl_easy_getinfo(curl, CURLINFO_CONDITION_UNMET, &timecond);
 	curl_easy_getinfo(curl, CURLINFO_EFFECTIVE_URL, &effective_url);
 
+	/* Let's check if client requested downloading accompanion *.sig file */
+	if(!payload->signature && payload->download_signature && curlerr == CURLE_OK && payload->respcode < 400) {
+		struct dload_payload *sig = NULL;
+
+		int len = strlen(effective_url) + 5;
+		CALLOC(sig, 1, sizeof(*sig), GOTO_ERR(handle, ALPM_ERR_MEMORY, cleanup));
+		MALLOC(sig->fileurl, len, FREE(sig); GOTO_ERR(handle, ALPM_ERR_MEMORY, cleanup));
+		snprintf(sig->fileurl, len, "%s.sig", effective_url);
+		sig->signature = 1;
+		sig->handle = handle;
+		sig->force = payload->force;
+		sig->unlink_on_fail = payload->unlink_on_fail;
+		sig->errors_ok = payload->signature_optional;
+		/* set hard upper limit of 16KiB */
+		sig->max_size = 16 * 1024;
+
+		curl_multi_add_payload(handle, curlm, sig, localpath);
+	}
+
 	/* time condition was met and we didn't download anything. we need to
 	 * clean up the 0 byte .part file that's left behind. */
 	if(timecond == 1 && DOUBLE_EQ(bytes_dl, 0)) {
@@ -565,6 +589,13 @@  cleanup:
 	if(ret == -1 && payload->errors_ok) {
 		ret = -2;
 	}
+
+	if(payload->signature) {
+		/* free signature payload memory that was allocated earlier in dload.c */
+		_alpm_dload_payload_reset(payload);
+		FREE(payload);
+	}
+
 	return ret;
 }
 
@@ -669,12 +700,12 @@  static int curl_multi_download_internal(alpm_handle_t *handle,
 	int still_running = 0;
 	int err = 0;
 	int parallel_downloads = handle->parallel_downloads;
-
 	CURLM *curlm = handle->curlm;
-	CURLMsg *msg;
+	bool msg_done = false;
 
-	while(still_running || payloads) {
-		int msgs_left = -1;
+	while(still_running || payloads || msg_done) {
+		int msgs_left = 0;
+		CURLMcode mc;
 
 		for(; still_running < parallel_downloads && payloads; still_running++) {
 			struct dload_payload *payload = payloads->data;
@@ -687,15 +718,19 @@  static int curl_multi_download_internal(alpm_handle_t *handle,
 
 				payloads = payloads->next;
 			} else {
-				// the payload failed to start, do not start any new downloads just wait until
-				// active one complete.
+				/* The payload failed to start. Do not start any new downloads.
+				 * Wait until all active downloads complete.
+				 */
 				_alpm_log(handle, ALPM_LOG_ERROR, _("failed to setup a download payload for %s\n"), payload->remote_name);
 				payloads = NULL;
 				err = -1;
 			}
 		}
 
-		CURLMcode mc = curl_multi_perform(curlm, &still_running);
+		mc = curl_multi_perform(curlm, &still_running);
+		if(mc == CURLM_OK) {
+			mc = curl_multi_wait(curlm, NULL, 0, 1000, NULL);
+		}
 
 		if(mc != CURLM_OK) {
 			_alpm_log(handle, ALPM_LOG_ERROR, _("curl returned error %d from transfer\n"), mc);
@@ -703,28 +738,28 @@  static int curl_multi_download_internal(alpm_handle_t *handle,
 			err = -1;
 		}
 
-		while((msg = curl_multi_info_read(curlm, &msgs_left))) {
+		msg_done = false;
+		while(true) {
+			CURLMsg *msg = curl_multi_info_read(curlm, &msgs_left);
+			if(!msg) {
+				break;
+			}
 			if(msg->msg == CURLMSG_DONE) {
 				int ret = curl_multi_check_finished_download(curlm, msg, localpath);
 				if(ret == -1) {
-					/* if current payload failed to download then stop adding new payloads but wait for the
-					 * current ones
-					 */
+					/* if current payload failed to download then stop adding new payloads */
 					payloads = NULL;
 					err = -1;
-				} else if(ret == 2) {
-					/* in case of a retry increase the counter of active requests
-					 * to avoid exiting the loop early
-					 */
-					still_running++;
 				}
+				/* curl_multi_check_finished_download() might add more payloads e.g. in case of a retry
+				 * from the next mirror. We need to execute curl_multi_perform() at least one more time
+				 * to make sure new payload requests are processed.
+				 */
+				msg_done = true;
 			} else {
 				_alpm_log(handle, ALPM_LOG_ERROR, _("curl transfer error: %d\n"), msg->msg);
 			}
 		}
-		if(still_running) {
-			curl_multi_wait(curlm, NULL, 0, 1000, NULL);
-		}
 	}
 
 	_alpm_log(handle, ALPM_LOG_DEBUG, "curl_multi_download_internal return code is %d\n", err);
@@ -803,12 +838,10 @@  int SYMEXPORT alpm_fetch_pkgurl(alpm_handle_t *handle, const alpm_list_t *urls,
 {
 	const char *cachedir;
 	alpm_list_t *payloads = NULL;
-	int download_sigs;
 	const alpm_list_t *i;
 	alpm_event_t event;
 
 	CHECK_HANDLE(handle, return -1);
-	download_sigs = handle->siglevel & ALPM_SIG_PACKAGE;
 
 	/* find a valid cache dir to download to */
 	cachedir = _alpm_filecache_setup(handle);
@@ -830,21 +863,9 @@  int SYMEXPORT alpm_fetch_pkgurl(alpm_handle_t *handle, const alpm_list_t *urls,
 			payload->allow_resume = 1;
 			payload->handle = handle;
 			payload->trust_remote_name = 1;
+			payload->download_signature = (handle->siglevel & ALPM_SIG_PACKAGE);
+			payload->signature_optional = (handle->siglevel & ALPM_SIG_PACKAGE_OPTIONAL);
 			payloads = alpm_list_add(payloads, payload);
-
-			if(download_sigs) {
-				int len = strlen(url) + 5;
-				CALLOC(payload, 1, sizeof(*payload), GOTO_ERR(handle, ALPM_ERR_MEMORY, err));
-				payload->signature = 1;
-				MALLOC(payload->fileurl, len, FREE(payload); GOTO_ERR(handle, ALPM_ERR_MEMORY, err));
-				snprintf(payload->fileurl, len, "%s.sig", url);
-				payload->handle = handle;
-				payload->trust_remote_name = 1;
-				payload->errors_ok = (handle->siglevel & ALPM_SIG_PACKAGE_OPTIONAL);
-				/* set hard upper limit of 16KiB */
-				payload->max_size = 16 * 1024;
-				payloads = alpm_list_add(payloads, payload);
-			}
 		}
 	}
 
diff --git a/lib/libalpm/dload.h b/lib/libalpm/dload.h
index d13bc1b5..facafca2 100644
--- a/lib/libalpm/dload.h
+++ b/lib/libalpm/dload.h
@@ -47,11 +47,13 @@  struct dload_payload {
 	int errors_ok;
 	int unlink_on_fail;
 	int trust_remote_name;
-	int signature; /* specifies if the payload is a signature file */
+	int download_signature; /* specifies if an accompanion *.sig file need to be downloaded*/
+	int signature_optional; /* *.sig file is optional */
 #ifdef HAVE_LIBCURL
 	CURL *curl;
 	char error_buffer[CURL_ERROR_SIZE];
 	FILE *localf; /* temp download file */
+	int signature; /* specifies if this payload is for a signature file */
 #endif
 };