[pacman-dev] Convert '-U pkg1 pkg2' codepath to parallel download

Message ID 20200511085010.117622-1-anatol.pomozov@gmail.com
State Superseded, archived
Headers show
Series [pacman-dev] Convert '-U pkg1 pkg2' codepath to parallel download | expand

Commit Message

Anatol Pomozov May 11, 2020, 8:50 a.m. UTC
Installing remote packages using its URL is an interesting case for ALPM
API. Unlike package sync ('pacman -S pkg1 pkg2') '-U' does not deal with
server mirror list. Thus _alpm_multi_download() should be able to
handle file download for payloads that either have 'fileurl' field
or pair of fields ('servers' and 'filepath') set.

Signature for alpm_fetch_pkgurl() has changed and it accepts an
output list that is populated with filepaths to fetched packages.

Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
---
 lib/libalpm/alpm.h   |  11 ++-
 lib/libalpm/dload.c  | 204 +++++++++++++++++++++++++------------------
 lib/libalpm/dload.h  |   7 +-
 src/pacman/upgrade.c | 102 ++++++++++------------
 4 files changed, 181 insertions(+), 143 deletions(-)

Comments

Allan McRae June 11, 2020, 2:56 a.m. UTC | #1
On 11/5/20 6:50 pm, Anatol Pomozov wrote:
> Installing remote packages using its URL is an interesting case for ALPM
> API. Unlike package sync ('pacman -S pkg1 pkg2') '-U' does not deal with
> server mirror list. Thus _alpm_multi_download() should be able to
> handle file download for payloads that either have 'fileurl' field
> or pair of fields ('servers' and 'filepath') set.
> 
> Signature for alpm_fetch_pkgurl() has changed and it accepts an
> output list that is populated with filepaths to fetched packages.
> 
> Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>

Thanks. Very minor comments belown.

> ---
>  lib/libalpm/alpm.h   |  11 ++-
>  lib/libalpm/dload.c  | 204 +++++++++++++++++++++++++------------------
>  lib/libalpm/dload.h  |   7 +-
>  src/pacman/upgrade.c | 102 ++++++++++------------
>  4 files changed, 181 insertions(+), 143 deletions(-)
> 
> diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h
> index 3ea66ccc..c6a13273 100644
> --- a/lib/libalpm/alpm.h
> +++ b/lib/libalpm/alpm.h
> @@ -755,12 +755,15 @@ typedef void (*alpm_cb_totaldl)(off_t total);
>  typedef int (*alpm_cb_fetch)(const char *url, const char *localpath,
>  		int force);
>  
> -/** Fetch a remote pkg.
> +/** Fetch a list of remote packages.
>   * @param handle the context handle
> - * @param url URL of the package to download
> - * @return the downloaded filepath on success, NULL on error
> + * @param urls list of package URLs to download
> + * @param fetched list of filepaths to the fetched packages, each item
> + *    corresponds to one in `urls` list

Can you make it clear that this is filled by the function, and not
something that should be passed.

> + * @return 0 on success or -1 on failure
>   */
> -char *alpm_fetch_pkgurl(alpm_handle_t *handle, const char *url);
> +int alpm_fetch_pkgurl(alpm_handle_t *handle, const alpm_list_t *urls,
> +	  alpm_list_t **fetched);
>  
>  /** @addtogroup alpm_api_options Options
>   * Libalpm option getters and setters
> diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c
> index 13aa4086..43fe9847 100644
> --- a/lib/libalpm/dload.c
> +++ b/lib/libalpm/dload.c
> @@ -613,7 +613,9 @@ static int curl_multi_retry_next_server(CURLM *curlm, CURL *curl, struct dload_p
>  	size_t len;
>  	alpm_handle_t *handle = payload->handle;
>  
> -	payload->servers = payload->servers->next;
> +	if(payload->servers) {
> +		payload->servers = payload->servers->next;
> +	}

OK.

>  	if(!payload->servers) {
>  		_alpm_log(payload->handle, ALPM_LOG_DEBUG,
>  				"%s: no more servers to retry\n", payload->remote_name);
> @@ -622,7 +624,7 @@ static int curl_multi_retry_next_server(CURLM *curlm, CURL *curl, struct dload_p
>  	server = payload->servers->data;
>  
>  	/* regenerate a new fileurl */
> -	free(payload->fileurl);
> +	FREE(payload->fileurl);

Change unrelated to this patch, but OK.

>  	len = strlen(server) + strlen(payload->filepath) + 2;
>  	MALLOC(payload->fileurl, len, RET_ERR(handle, ALPM_ERR_MEMORY, -1));
>  	snprintf(payload->fileurl, len, "%s/%s", server, payload->filepath);
> @@ -849,20 +851,28 @@ static int curl_multi_add_payload(alpm_handle_t *handle, CURLM *curlm,
>  		struct dload_payload *payload, const char *localpath)
>  {
>  	size_t len;
> -	const char *server;
>  	CURL *curl = NULL;
>  	char hostname[HOSTNAME_SIZE];
>  	int ret = -1;
>  
> -	ASSERT(payload->servers, RET_ERR(handle, ALPM_ERR_SERVER_NONE, -1));
> -	server = payload->servers->data;
> -
>  	curl = curl_easy_init();
>  	payload->curl = curl;
>  
> -	len = strlen(server) + strlen(payload->filepath) + 2;
> -	MALLOC(payload->fileurl, len, GOTO_ERR(handle, ALPM_ERR_MEMORY, cleanup));
> -	snprintf(payload->fileurl, len, "%s/%s", server, payload->filepath);
> +	if(payload->fileurl) {
> +		ASSERT(!payload->servers, GOTO_ERR(handle, ALPM_ERR_WRONG_ARGS, cleanup));
> +		ASSERT(!payload->filepath, GOTO_ERR(handle, ALPM_ERR_WRONG_ARGS, cleanup));

OK.

> +	} else {
> +		const char *server;
> +
> +		ASSERT(payload->servers, GOTO_ERR(handle, ALPM_ERR_SERVER_NONE, cleanup));
> +		ASSERT(payload->filepath, GOTO_ERR(handle, ALPM_ERR_WRONG_ARGS, cleanup));
> +
> +		server = payload->servers->data;
> +
> +		len = strlen(server) + strlen(payload->filepath) + 2;
> +		MALLOC(payload->fileurl, len, GOTO_ERR(handle, ALPM_ERR_MEMORY, cleanup));
> +		snprintf(payload->fileurl, len, "%s/%s", server, payload->filepath);
> +	}
>  

OK.

>  	payload->tempfile_openmode = "wb";
>  	if(!payload->remote_name) {
> @@ -1046,22 +1056,29 @@ int _alpm_multi_download(alpm_handle_t *handle,
>  			alpm_list_t *s;
>  			int success = 0;
>  
> -			for(s = payload->servers; s; s = s->next) {
> -				const char *server = s->data;
> -				char *fileurl;
> -				int ret;
> -
> -				size_t len = strlen(server) + strlen(payload->filepath) + 2;
> -				MALLOC(fileurl, len, RET_ERR(handle, ALPM_ERR_MEMORY, -1));
> -				snprintf(fileurl, len, "%s/%s", server, payload->filepath);
> -
> -				ret = handle->fetchcb(fileurl, localpath, payload->force);
> -				free(fileurl);
> -
> -				if (ret != -1) {
> +			if(payload->fileurl) {
> +				if (handle->fetchcb(payload->fileurl, localpath, payload->force) != -1) {
>  					success = 1;
>  					break;
>  				}
> +			} else {
> +				for(s = payload->servers; s; s = s->next) {
> +					const char *server = s->data;
> +					char *fileurl;
> +					int ret;
> +
> +					size_t len = strlen(server) + strlen(payload->filepath) + 2;
> +					MALLOC(fileurl, len, RET_ERR(handle, ALPM_ERR_MEMORY, -1));
> +					snprintf(fileurl, len, "%s/%s", server, payload->filepath);
> +
> +					ret = handle->fetchcb(fileurl, localpath, payload->force);
> +					free(fileurl);
> +
> +					if (ret != -1) {
> +						success = 1;
> +						break;
> +					}
> +				}
>  			}

OK.

>  			if(!success && !payload->errors_ok) {
>  				RET_ERR(handle, ALPM_ERR_EXTERNAL_DOWNLOAD, -1);
> @@ -1087,82 +1104,101 @@ static char *filecache_find_url(alpm_handle_t *handle, const char *url)
>  	return _alpm_filecache_find(handle, filebase);
>  }
>  
> -char SYMEXPORT *alpm_fetch_pkgurl(alpm_handle_t *handle, const char *url)
> +int SYMEXPORT alpm_fetch_pkgurl(alpm_handle_t *handle, const alpm_list_t *urls,
> +	  alpm_list_t **fetched)
>  {
> -	char *filepath;
> -	const char *cachedir, *final_pkg_url = NULL;
> -	char *final_file = NULL;
> -	struct dload_payload payload = {0};
> -	int ret = 0;
> +	const char *cachedir;
> +	alpm_list_t *payloads = NULL;
> +	int download_sigs;
> +	const alpm_list_t *i;
> +	alpm_event_t event;
>  
> -	CHECK_HANDLE(handle, return NULL);
> -	ASSERT(url, RET_ERR(handle, ALPM_ERR_WRONG_ARGS, NULL));
> +	CHECK_HANDLE(handle, return -1);
> +	download_sigs = handle->siglevel & ALPM_SIG_PACKAGE;
>  

Should we check *fetched is NULL?

>  	/* find a valid cache dir to download to */
>  	cachedir = _alpm_filecache_setup(handle);
>  
> -	/* attempt to find the file in our pkgcache */
> -	filepath = filecache_find_url(handle, url);
> -	if(filepath == NULL) {
> -		STRDUP(payload.fileurl, url, RET_ERR(handle, ALPM_ERR_MEMORY, NULL));
> -		payload.allow_resume = 1;
> -		payload.handle = handle;
> -		payload.trust_remote_name = 1;
> -
> -		/* download the file */
> -		ret = _alpm_download(&payload, cachedir, &final_file, &final_pkg_url);
> -		_alpm_dload_payload_reset(&payload);
> -		if(ret == -1) {
> -			_alpm_log(handle, ALPM_LOG_WARNING, _("failed to download %s\n"), url);
> -			free(final_file);
> -			return NULL;
> +	for(i = urls; i; i = i->next) {
> +		char *url = i->data;
> +
> +		/* attempt to find the file in our pkgcache */
> +		char *filepath = filecache_find_url(handle, url);
> +		if(filepath) {
> +			/* the file is locally cached so add it to the output right away */
> +			alpm_list_append(fetched, filepath);
> +		} else {
> +			struct dload_payload *payload = NULL;
> +
> +			ASSERT(url, GOTO_ERR(handle, ALPM_ERR_WRONG_ARGS, err));
> +			CALLOC(payload, 1, sizeof(*payload), GOTO_ERR(handle, ALPM_ERR_MEMORY, err));
> +			STRDUP(payload->fileurl, url, FREE(payload); GOTO_ERR(handle, ALPM_ERR_MEMORY, err));
> +			payload->allow_resume = 1;
> +			payload->handle = handle;
> +			payload->trust_remote_name = 1;
> +			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 (realising this will be updated in a couple of patches time to handle
redircting stuff)

>  		}
> -		_alpm_log(handle, ALPM_LOG_DEBUG, "successfully downloaded %s\n", url);
>  	}
>  
> -	/* attempt to download the signature */
> -	if(ret == 0 && final_pkg_url && (handle->siglevel & ALPM_SIG_PACKAGE)) {
> -		char *sig_filepath, *sig_final_file = NULL;
> -		size_t len;
> -
> -		len = strlen(final_pkg_url) + 5;
> -		MALLOC(payload.fileurl, len, free(final_file); RET_ERR(handle, ALPM_ERR_MEMORY, NULL));
> -		snprintf(payload.fileurl, len, "%s.sig", final_pkg_url);
> -
> -		sig_filepath = filecache_find_url(handle, payload.fileurl);
> -		if(sig_filepath == NULL) {
> -			payload.signature = 1;
> -			payload.handle = handle;
> -			payload.trust_remote_name = 1;
> -			payload.force = 1;
> -			payload.errors_ok = (handle->siglevel & ALPM_SIG_PACKAGE_OPTIONAL);
> -
> -			/* set hard upper limit of 16KiB */
> -			payload.max_size = 16 * 1024;
> -
> -			ret = _alpm_download(&payload, cachedir, &sig_final_file, NULL);
> -			if(ret == -1 && !payload.errors_ok) {
> -				_alpm_log(handle, ALPM_LOG_WARNING,
> -						_("failed to download %s\n"), payload.fileurl);
> -				/* Warn now, but don't return NULL. We will fail later during package
> -				 * load time. */
> -			} else if(ret == 0) {
> -				_alpm_log(handle, ALPM_LOG_DEBUG,
> -						"successfully downloaded %s\n", payload.fileurl);

OK.

> +	if(payloads) {
> +		event.type = ALPM_EVENT_PKG_RETRIEVE_START;
> +		EVENT(handle, &event);
> +		if(_alpm_multi_download(handle, payloads, cachedir) == -1) {
> +			_alpm_log(handle, ALPM_LOG_WARNING, _("failed to retrieve some files\n"));
> +			event.type = ALPM_EVENT_PKG_RETRIEVE_FAILED;
> +			EVENT(handle, &event);
> +
> +			GOTO_ERR(handle, ALPM_ERR_RETRIEVE, err);
> +		} else {
> +			event.type = ALPM_EVENT_PKG_RETRIEVE_DONE;
> +			EVENT(handle, &event);
> +		}
> +
> +		for(i = payloads; i; i = i->next) {
> +			struct dload_payload *payload = i->data;
> +			const char *filename;
> +			char *filepath;
> +
> +			if(payload->signature) {
> +				continue;
> +			}
> +
> +			filename = mbasename(payload->destfile_name);
> +			filepath = _alpm_filecache_find(handle, filename);
> +			if(filepath) {
> +				alpm_list_append(fetched, filepath);
> +			} else {
> +				_alpm_log(handle, ALPM_LOG_WARNING, _("download completed successfully but no file in the cache\n"));
> +				GOTO_ERR(handle, ALPM_ERR_RETRIEVE, err);
>  			}
> -			FREE(sig_final_file);
>  		}

OK.

> -		free(sig_filepath);
> -		_alpm_dload_payload_reset(&payload);
> -	}
>  
> -	/* we should be able to find the file the second time around */
> -	if(filepath == NULL) {
> -		filepath = _alpm_filecache_find(handle, final_file);
> +		alpm_list_free_inner(payloads, (alpm_list_fn_free)_alpm_dload_payload_reset);
> +		FREELIST(payloads);
>  	}
> -	free(final_file);
>  
> -	return filepath;
> +	return 0;
> +
> +err:
> +	alpm_list_free_inner(payloads, (alpm_list_fn_free)_alpm_dload_payload_reset);
> +	FREELIST(payloads);
> +	FREELIST(*fetched);
> +
> +	return -1;
>  }
>  

OK.

>  void _alpm_dload_payload_reset(struct dload_payload *payload)
> diff --git a/lib/libalpm/dload.h b/lib/libalpm/dload.h
> index 1e4af755..f119fc2e 100644
> --- a/lib/libalpm/dload.h
> +++ b/lib/libalpm/dload.h
> @@ -30,9 +30,14 @@ struct dload_payload {
>  	char *tempfile_name;
>  	char *destfile_name;
>  	char *content_disp_name;
> +	/* client has to provide either
> +	 *  1) fileurl - full URL to the file
> +	 *  2) pair of (servers, filepath), in this case ALPM iterates over the
> +	 *     server list and tries to download "$server/$filepath"
> +	 */

OK.

>  	char *fileurl;
> -	char *filepath; /* download URL path */
>  	alpm_list_t *servers;
> +	char *filepath; /* download URL path */

Random churn.

>  	long respcode;
>  	off_t initial_size;
>  	off_t max_size;
> diff --git a/src/pacman/upgrade.c b/src/pacman/upgrade.c
> index 5f984e44..840e3a31 100644
> --- a/src/pacman/upgrade.c
> +++ b/src/pacman/upgrade.c
> @@ -30,6 +30,34 @@
>  #include "conf.h"
>  #include "util.h"
>  
> +/* add targets to the created transaction */
> +static int load_packages(alpm_list_t *targets, int siglevel)
> +{
> +	alpm_list_t *i;
> +	int retval = 0;
> +
> +	for(i = targets; i; i = alpm_list_next(i)) {
> +		const char *targ = i->data;
> +		alpm_pkg_t *pkg;
> +
> +		if(alpm_pkg_load(config->handle, targ, 1, siglevel, &pkg) != 0) {
> +			pm_printf(ALPM_LOG_ERROR, "'%s': %s\n",
> +					targ, alpm_strerror(alpm_errno(config->handle)));
> +			retval = 1;
> +			continue;
> +		}
> +		if(alpm_add_pkg(config->handle, pkg) == -1) {
> +			pm_printf(ALPM_LOG_ERROR, "'%s': %s\n",
> +					targ, alpm_strerror(alpm_errno(config->handle)));
> +			alpm_pkg_free(pkg);
> +			retval = 1;
> +			continue;
> +		}
> +		config->explicit_adds = alpm_list_add(config->explicit_adds, pkg);
> +	}
> +	return retval;
> +}
> +
>  /**
>   * @brief Upgrade a specified list of packages.
>   *
> @@ -39,43 +67,30 @@
>   */
>  int pacman_upgrade(alpm_list_t *targets)
>  {
> -	int retval = 0, *file_is_remote;
> +	int retval = 0;
> +	alpm_list_t *remote_targets = NULL, *fetched_files = NULL;
> +	alpm_list_t *local_targets = NULL;
>  	alpm_list_t *i;
> -	unsigned int n, num_targets;
>  
>  	if(targets == NULL) {
>  		pm_printf(ALPM_LOG_ERROR, _("no targets specified (use -h for help)\n"));
>  		return 1;
>  	}
>  
> -	num_targets = alpm_list_count(targets);
> -
> -	/* Check for URL targets and process them */
> -	file_is_remote = malloc(num_targets * sizeof(int));
> -	if(file_is_remote == NULL) {
> -		pm_printf(ALPM_LOG_ERROR, _("memory exhausted\n"));
> -		return 1;
> -	}
> -
> -	for(i = targets, n = 0; i; i = alpm_list_next(i), n++) {
> +	/* carve out remote targets and move it into a separate list */
> +	for(i = targets; i; i = alpm_list_next(i)) {
>  		if(strstr(i->data, "://")) {
> -			char *str = alpm_fetch_pkgurl(config->handle, i->data);
> -			if(str == NULL) {
> -				pm_printf(ALPM_LOG_ERROR, "'%s': %s\n",
> -						(char *)i->data, alpm_strerror(alpm_errno(config->handle)));
> -				retval = 1;
> -			} else {
> -				free(i->data);
> -				i->data = str;
> -				file_is_remote[n] = 1;
> -			}
> +			remote_targets = alpm_list_add(remote_targets, i->data);
>  		} else {
> -			file_is_remote[n] = 0;
> +			local_targets = alpm_list_add(local_targets, i->data);
>  		}
>  	}
>  
> -	if(retval) {
> -		goto fail_free;
> +	if(remote_targets) {
> +		retval = alpm_fetch_pkgurl(config->handle, remote_targets, &fetched_files);
> +		if(retval) {
> +			goto fail_free;
> +		}
>  	}
>  

OK.

>  	/* Step 1: create a new transaction */
> @@ -85,39 +100,16 @@ int pacman_upgrade(alpm_list_t *targets)
>  	}
>  
>  	printf(_("loading packages...\n"));
> -	/* add targets to the created transaction */
> -	for(i = targets, n = 0; i; i = alpm_list_next(i), n++) {
> -		const char *targ = i->data;
> -		alpm_pkg_t *pkg;
> -		int siglevel;
> -
> -		if(file_is_remote[n]) {
> -			siglevel = alpm_option_get_remote_file_siglevel(config->handle);
> -		} else {
> -			siglevel = alpm_option_get_local_file_siglevel(config->handle);
> -		}
> -
> -		if(alpm_pkg_load(config->handle, targ, 1, siglevel, &pkg) != 0) {
> -			pm_printf(ALPM_LOG_ERROR, "'%s': %s\n",
> -					targ, alpm_strerror(alpm_errno(config->handle)));
> -			retval = 1;
> -			continue;
> -		}
> -		if(alpm_add_pkg(config->handle, pkg) == -1) {
> -			pm_printf(ALPM_LOG_ERROR, "'%s': %s\n",
> -					targ, alpm_strerror(alpm_errno(config->handle)));
> -			alpm_pkg_free(pkg);
> -			retval = 1;
> -			continue;
> -		}
> -		config->explicit_adds = alpm_list_add(config->explicit_adds, pkg);
> -	}
> +	retval |= load_packages(local_targets, alpm_option_get_local_file_siglevel(config->handle));
> +	retval |= load_packages(fetched_files, alpm_option_get_remote_file_siglevel(config->handle));
>  

OK.

>  	if(retval) {
>  		goto fail_release;
>  	}
>  
> -	free(file_is_remote);
> +	alpm_list_free(remote_targets);
> +	alpm_list_free(local_targets);
> +	FREELIST(fetched_files);
>  
>  	/* now that targets are resolved, we can hand it all off to the sync code */
>  	return sync_prepare_execute();
> @@ -125,7 +117,9 @@ int pacman_upgrade(alpm_list_t *targets)
>  fail_release:
>  	trans_release();
>  fail_free:
> -	free(file_is_remote);
> +	alpm_list_free(remote_targets);
> +	alpm_list_free(local_targets);
> +	FREELIST(fetched_files);
>  
>  	return retval;
>  }
> 

OK.
Anatol Pomozov June 23, 2020, 5:57 a.m. UTC | #2
Hi

On Wed, Jun 10, 2020 at 7:57 PM Allan McRae <allan@archlinux.org> wrote:
>
> On 11/5/20 6:50 pm, Anatol Pomozov wrote:
> > Installing remote packages using its URL is an interesting case for ALPM
> > API. Unlike package sync ('pacman -S pkg1 pkg2') '-U' does not deal with
> > server mirror list. Thus _alpm_multi_download() should be able to
> > handle file download for payloads that either have 'fileurl' field
> > or pair of fields ('servers' and 'filepath') set.
> >
> > Signature for alpm_fetch_pkgurl() has changed and it accepts an
> > output list that is populated with filepaths to fetched packages.
> >
> > Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
>
> Thanks. Very minor comments belown.
>
> > ---
> >  lib/libalpm/alpm.h   |  11 ++-
> >  lib/libalpm/dload.c  | 204 +++++++++++++++++++++++++------------------
> >  lib/libalpm/dload.h  |   7 +-
> >  src/pacman/upgrade.c | 102 ++++++++++------------
> >  4 files changed, 181 insertions(+), 143 deletions(-)
> >
> > diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h
> > index 3ea66ccc..c6a13273 100644
> > --- a/lib/libalpm/alpm.h
> > +++ b/lib/libalpm/alpm.h
> > @@ -755,12 +755,15 @@ typedef void (*alpm_cb_totaldl)(off_t total);
> >  typedef int (*alpm_cb_fetch)(const char *url, const char *localpath,
> >               int force);
> >
> > -/** Fetch a remote pkg.
> > +/** Fetch a list of remote packages.
> >   * @param handle the context handle
> > - * @param url URL of the package to download
> > - * @return the downloaded filepath on success, NULL on error
> > + * @param urls list of package URLs to download
> > + * @param fetched list of filepaths to the fetched packages, each item
> > + *    corresponds to one in `urls` list
>
> Can you make it clear that this is filled by the function, and not
> something that should be passed.

Done.

>
> > + * @return 0 on success or -1 on failure
> >   */
> > -char *alpm_fetch_pkgurl(alpm_handle_t *handle, const char *url);
> > +int alpm_fetch_pkgurl(alpm_handle_t *handle, const alpm_list_t *urls,
> > +       alpm_list_t **fetched);
> >
> >  /** @addtogroup alpm_api_options Options
> >   * Libalpm option getters and setters
> > diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c
> > index 13aa4086..43fe9847 100644
> > --- a/lib/libalpm/dload.c
> > +++ b/lib/libalpm/dload.c
> > @@ -613,7 +613,9 @@ static int curl_multi_retry_next_server(CURLM *curlm, CURL *curl, struct dload_p
> >       size_t len;
> >       alpm_handle_t *handle = payload->handle;
> >
> > -     payload->servers = payload->servers->next;
> > +     if(payload->servers) {
> > +             payload->servers = payload->servers->next;
> > +     }
>
> OK.
>
> >       if(!payload->servers) {
> >               _alpm_log(payload->handle, ALPM_LOG_DEBUG,
> >                               "%s: no more servers to retry\n", payload->remote_name);
> > @@ -622,7 +624,7 @@ static int curl_multi_retry_next_server(CURLM *curlm, CURL *curl, struct dload_p
> >       server = payload->servers->data;
> >
> >       /* regenerate a new fileurl */
> > -     free(payload->fileurl);
> > +     FREE(payload->fileurl);
>
> Change unrelated to this patch, but OK.
>
> >       len = strlen(server) + strlen(payload->filepath) + 2;
> >       MALLOC(payload->fileurl, len, RET_ERR(handle, ALPM_ERR_MEMORY, -1));
> >       snprintf(payload->fileurl, len, "%s/%s", server, payload->filepath);
> > @@ -849,20 +851,28 @@ static int curl_multi_add_payload(alpm_handle_t *handle, CURLM *curlm,
> >               struct dload_payload *payload, const char *localpath)
> >  {
> >       size_t len;
> > -     const char *server;
> >       CURL *curl = NULL;
> >       char hostname[HOSTNAME_SIZE];
> >       int ret = -1;
> >
> > -     ASSERT(payload->servers, RET_ERR(handle, ALPM_ERR_SERVER_NONE, -1));
> > -     server = payload->servers->data;
> > -
> >       curl = curl_easy_init();
> >       payload->curl = curl;
> >
> > -     len = strlen(server) + strlen(payload->filepath) + 2;
> > -     MALLOC(payload->fileurl, len, GOTO_ERR(handle, ALPM_ERR_MEMORY, cleanup));
> > -     snprintf(payload->fileurl, len, "%s/%s", server, payload->filepath);
> > +     if(payload->fileurl) {
> > +             ASSERT(!payload->servers, GOTO_ERR(handle, ALPM_ERR_WRONG_ARGS, cleanup));
> > +             ASSERT(!payload->filepath, GOTO_ERR(handle, ALPM_ERR_WRONG_ARGS, cleanup));
>
> OK.
>
> > +     } else {
> > +             const char *server;
> > +
> > +             ASSERT(payload->servers, GOTO_ERR(handle, ALPM_ERR_SERVER_NONE, cleanup));
> > +             ASSERT(payload->filepath, GOTO_ERR(handle, ALPM_ERR_WRONG_ARGS, cleanup));
> > +
> > +             server = payload->servers->data;
> > +
> > +             len = strlen(server) + strlen(payload->filepath) + 2;
> > +             MALLOC(payload->fileurl, len, GOTO_ERR(handle, ALPM_ERR_MEMORY, cleanup));
> > +             snprintf(payload->fileurl, len, "%s/%s", server, payload->filepath);
> > +     }
> >
>
> OK.
>
> >       payload->tempfile_openmode = "wb";
> >       if(!payload->remote_name) {
> > @@ -1046,22 +1056,29 @@ int _alpm_multi_download(alpm_handle_t *handle,
> >                       alpm_list_t *s;
> >                       int success = 0;
> >
> > -                     for(s = payload->servers; s; s = s->next) {
> > -                             const char *server = s->data;
> > -                             char *fileurl;
> > -                             int ret;
> > -
> > -                             size_t len = strlen(server) + strlen(payload->filepath) + 2;
> > -                             MALLOC(fileurl, len, RET_ERR(handle, ALPM_ERR_MEMORY, -1));
> > -                             snprintf(fileurl, len, "%s/%s", server, payload->filepath);
> > -
> > -                             ret = handle->fetchcb(fileurl, localpath, payload->force);
> > -                             free(fileurl);
> > -
> > -                             if (ret != -1) {
> > +                     if(payload->fileurl) {
> > +                             if (handle->fetchcb(payload->fileurl, localpath, payload->force) != -1) {
> >                                       success = 1;
> >                                       break;
> >                               }
> > +                     } else {
> > +                             for(s = payload->servers; s; s = s->next) {
> > +                                     const char *server = s->data;
> > +                                     char *fileurl;
> > +                                     int ret;
> > +
> > +                                     size_t len = strlen(server) + strlen(payload->filepath) + 2;
> > +                                     MALLOC(fileurl, len, RET_ERR(handle, ALPM_ERR_MEMORY, -1));
> > +                                     snprintf(fileurl, len, "%s/%s", server, payload->filepath);
> > +
> > +                                     ret = handle->fetchcb(fileurl, localpath, payload->force);
> > +                                     free(fileurl);
> > +
> > +                                     if (ret != -1) {
> > +                                             success = 1;
> > +                                             break;
> > +                                     }
> > +                             }
> >                       }
>
> OK.
>
> >                       if(!success && !payload->errors_ok) {
> >                               RET_ERR(handle, ALPM_ERR_EXTERNAL_DOWNLOAD, -1);
> > @@ -1087,82 +1104,101 @@ static char *filecache_find_url(alpm_handle_t *handle, const char *url)
> >       return _alpm_filecache_find(handle, filebase);
> >  }
> >
> > -char SYMEXPORT *alpm_fetch_pkgurl(alpm_handle_t *handle, const char *url)
> > +int SYMEXPORT alpm_fetch_pkgurl(alpm_handle_t *handle, const alpm_list_t *urls,
> > +       alpm_list_t **fetched)
> >  {
> > -     char *filepath;
> > -     const char *cachedir, *final_pkg_url = NULL;
> > -     char *final_file = NULL;
> > -     struct dload_payload payload = {0};
> > -     int ret = 0;
> > +     const char *cachedir;
> > +     alpm_list_t *payloads = NULL;
> > +     int download_sigs;
> > +     const alpm_list_t *i;
> > +     alpm_event_t event;
> >
> > -     CHECK_HANDLE(handle, return NULL);
> > -     ASSERT(url, RET_ERR(handle, ALPM_ERR_WRONG_ARGS, NULL));
> > +     CHECK_HANDLE(handle, return -1);
> > +     download_sigs = handle->siglevel & ALPM_SIG_PACKAGE;
> >
>
> Should we check *fetched is NULL?

It is a good idea. I also updated the function documentation and
specified that callee expects
*fetched is NULL.

>
> >       /* find a valid cache dir to download to */
> >       cachedir = _alpm_filecache_setup(handle);
> >
> > -     /* attempt to find the file in our pkgcache */
> > -     filepath = filecache_find_url(handle, url);
> > -     if(filepath == NULL) {
> > -             STRDUP(payload.fileurl, url, RET_ERR(handle, ALPM_ERR_MEMORY, NULL));
> > -             payload.allow_resume = 1;
> > -             payload.handle = handle;
> > -             payload.trust_remote_name = 1;
> > -
> > -             /* download the file */
> > -             ret = _alpm_download(&payload, cachedir, &final_file, &final_pkg_url);
> > -             _alpm_dload_payload_reset(&payload);
> > -             if(ret == -1) {
> > -                     _alpm_log(handle, ALPM_LOG_WARNING, _("failed to download %s\n"), url);
> > -                     free(final_file);
> > -                     return NULL;
> > +     for(i = urls; i; i = i->next) {
> > +             char *url = i->data;
> > +
> > +             /* attempt to find the file in our pkgcache */
> > +             char *filepath = filecache_find_url(handle, url);
> > +             if(filepath) {
> > +                     /* the file is locally cached so add it to the output right away */
> > +                     alpm_list_append(fetched, filepath);
> > +             } else {
> > +                     struct dload_payload *payload = NULL;
> > +
> > +                     ASSERT(url, GOTO_ERR(handle, ALPM_ERR_WRONG_ARGS, err));
> > +                     CALLOC(payload, 1, sizeof(*payload), GOTO_ERR(handle, ALPM_ERR_MEMORY, err));
> > +                     STRDUP(payload->fileurl, url, FREE(payload); GOTO_ERR(handle, ALPM_ERR_MEMORY, err));
> > +                     payload->allow_resume = 1;
> > +                     payload->handle = handle;
> > +                     payload->trust_remote_name = 1;
> > +                     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 (realising this will be updated in a couple of patches time to handle
> redircting stuff)

Yes, there is an upcoming patch that refactors this and a few other
places to make sure that
*.sig file is downloaded from the same mirror as the main file.

> >               }
> > -             _alpm_log(handle, ALPM_LOG_DEBUG, "successfully downloaded %s\n", url);
> >       }
> >
> > -     /* attempt to download the signature */
> > -     if(ret == 0 && final_pkg_url && (handle->siglevel & ALPM_SIG_PACKAGE)) {
> > -             char *sig_filepath, *sig_final_file = NULL;
> > -             size_t len;
> > -
> > -             len = strlen(final_pkg_url) + 5;
> > -             MALLOC(payload.fileurl, len, free(final_file); RET_ERR(handle, ALPM_ERR_MEMORY, NULL));
> > -             snprintf(payload.fileurl, len, "%s.sig", final_pkg_url);
> > -
> > -             sig_filepath = filecache_find_url(handle, payload.fileurl);
> > -             if(sig_filepath == NULL) {
> > -                     payload.signature = 1;
> > -                     payload.handle = handle;
> > -                     payload.trust_remote_name = 1;
> > -                     payload.force = 1;
> > -                     payload.errors_ok = (handle->siglevel & ALPM_SIG_PACKAGE_OPTIONAL);
> > -
> > -                     /* set hard upper limit of 16KiB */
> > -                     payload.max_size = 16 * 1024;
> > -
> > -                     ret = _alpm_download(&payload, cachedir, &sig_final_file, NULL);
> > -                     if(ret == -1 && !payload.errors_ok) {
> > -                             _alpm_log(handle, ALPM_LOG_WARNING,
> > -                                             _("failed to download %s\n"), payload.fileurl);
> > -                             /* Warn now, but don't return NULL. We will fail later during package
> > -                              * load time. */
> > -                     } else if(ret == 0) {
> > -                             _alpm_log(handle, ALPM_LOG_DEBUG,
> > -                                             "successfully downloaded %s\n", payload.fileurl);
>
> OK.
>
> > +     if(payloads) {
> > +             event.type = ALPM_EVENT_PKG_RETRIEVE_START;
> > +             EVENT(handle, &event);
> > +             if(_alpm_multi_download(handle, payloads, cachedir) == -1) {
> > +                     _alpm_log(handle, ALPM_LOG_WARNING, _("failed to retrieve some files\n"));
> > +                     event.type = ALPM_EVENT_PKG_RETRIEVE_FAILED;
> > +                     EVENT(handle, &event);
> > +
> > +                     GOTO_ERR(handle, ALPM_ERR_RETRIEVE, err);
> > +             } else {
> > +                     event.type = ALPM_EVENT_PKG_RETRIEVE_DONE;
> > +                     EVENT(handle, &event);
> > +             }
> > +
> > +             for(i = payloads; i; i = i->next) {
> > +                     struct dload_payload *payload = i->data;
> > +                     const char *filename;
> > +                     char *filepath;
> > +
> > +                     if(payload->signature) {
> > +                             continue;
> > +                     }
> > +
> > +                     filename = mbasename(payload->destfile_name);
> > +                     filepath = _alpm_filecache_find(handle, filename);
> > +                     if(filepath) {
> > +                             alpm_list_append(fetched, filepath);
> > +                     } else {
> > +                             _alpm_log(handle, ALPM_LOG_WARNING, _("download completed successfully but no file in the cache\n"));
> > +                             GOTO_ERR(handle, ALPM_ERR_RETRIEVE, err);
> >                       }
> > -                     FREE(sig_final_file);
> >               }
>
> OK.
>
> > -             free(sig_filepath);
> > -             _alpm_dload_payload_reset(&payload);
> > -     }
> >
> > -     /* we should be able to find the file the second time around */
> > -     if(filepath == NULL) {
> > -             filepath = _alpm_filecache_find(handle, final_file);
> > +             alpm_list_free_inner(payloads, (alpm_list_fn_free)_alpm_dload_payload_reset);
> > +             FREELIST(payloads);
> >       }
> > -     free(final_file);
> >
> > -     return filepath;
> > +     return 0;
> > +
> > +err:
> > +     alpm_list_free_inner(payloads, (alpm_list_fn_free)_alpm_dload_payload_reset);
> > +     FREELIST(payloads);
> > +     FREELIST(*fetched);
> > +
> > +     return -1;
> >  }
> >
>
> OK.
>
> >  void _alpm_dload_payload_reset(struct dload_payload *payload)
> > diff --git a/lib/libalpm/dload.h b/lib/libalpm/dload.h
> > index 1e4af755..f119fc2e 100644
> > --- a/lib/libalpm/dload.h
> > +++ b/lib/libalpm/dload.h
> > @@ -30,9 +30,14 @@ struct dload_payload {
> >       char *tempfile_name;
> >       char *destfile_name;
> >       char *content_disp_name;
> > +     /* client has to provide either
> > +      *  1) fileurl - full URL to the file
> > +      *  2) pair of (servers, filepath), in this case ALPM iterates over the
> > +      *     server list and tries to download "$server/$filepath"
> > +      */
>
> OK.
>
> >       char *fileurl;
> > -     char *filepath; /* download URL path */
> >       alpm_list_t *servers;
> > +     char *filepath; /* download URL path */
>
> Random churn.

reverted

>
> >       long respcode;
> >       off_t initial_size;
> >       off_t max_size;
> > diff --git a/src/pacman/upgrade.c b/src/pacman/upgrade.c
> > index 5f984e44..840e3a31 100644
> > --- a/src/pacman/upgrade.c
> > +++ b/src/pacman/upgrade.c
> > @@ -30,6 +30,34 @@
> >  #include "conf.h"
> >  #include "util.h"
> >
> > +/* add targets to the created transaction */
> > +static int load_packages(alpm_list_t *targets, int siglevel)
> > +{
> > +     alpm_list_t *i;
> > +     int retval = 0;
> > +
> > +     for(i = targets; i; i = alpm_list_next(i)) {
> > +             const char *targ = i->data;
> > +             alpm_pkg_t *pkg;
> > +
> > +             if(alpm_pkg_load(config->handle, targ, 1, siglevel, &pkg) != 0) {
> > +                     pm_printf(ALPM_LOG_ERROR, "'%s': %s\n",
> > +                                     targ, alpm_strerror(alpm_errno(config->handle)));
> > +                     retval = 1;
> > +                     continue;
> > +             }
> > +             if(alpm_add_pkg(config->handle, pkg) == -1) {
> > +                     pm_printf(ALPM_LOG_ERROR, "'%s': %s\n",
> > +                                     targ, alpm_strerror(alpm_errno(config->handle)));
> > +                     alpm_pkg_free(pkg);
> > +                     retval = 1;
> > +                     continue;
> > +             }
> > +             config->explicit_adds = alpm_list_add(config->explicit_adds, pkg);
> > +     }
> > +     return retval;
> > +}
> > +
> >  /**
> >   * @brief Upgrade a specified list of packages.
> >   *
> > @@ -39,43 +67,30 @@
> >   */
> >  int pacman_upgrade(alpm_list_t *targets)
> >  {
> > -     int retval = 0, *file_is_remote;
> > +     int retval = 0;
> > +     alpm_list_t *remote_targets = NULL, *fetched_files = NULL;
> > +     alpm_list_t *local_targets = NULL;
> >       alpm_list_t *i;
> > -     unsigned int n, num_targets;
> >
> >       if(targets == NULL) {
> >               pm_printf(ALPM_LOG_ERROR, _("no targets specified (use -h for help)\n"));
> >               return 1;
> >       }
> >
> > -     num_targets = alpm_list_count(targets);
> > -
> > -     /* Check for URL targets and process them */
> > -     file_is_remote = malloc(num_targets * sizeof(int));
> > -     if(file_is_remote == NULL) {
> > -             pm_printf(ALPM_LOG_ERROR, _("memory exhausted\n"));
> > -             return 1;
> > -     }
> > -
> > -     for(i = targets, n = 0; i; i = alpm_list_next(i), n++) {
> > +     /* carve out remote targets and move it into a separate list */
> > +     for(i = targets; i; i = alpm_list_next(i)) {
> >               if(strstr(i->data, "://")) {
> > -                     char *str = alpm_fetch_pkgurl(config->handle, i->data);
> > -                     if(str == NULL) {
> > -                             pm_printf(ALPM_LOG_ERROR, "'%s': %s\n",
> > -                                             (char *)i->data, alpm_strerror(alpm_errno(config->handle)));
> > -                             retval = 1;
> > -                     } else {
> > -                             free(i->data);
> > -                             i->data = str;
> > -                             file_is_remote[n] = 1;
> > -                     }
> > +                     remote_targets = alpm_list_add(remote_targets, i->data);
> >               } else {
> > -                     file_is_remote[n] = 0;
> > +                     local_targets = alpm_list_add(local_targets, i->data);
> >               }
> >       }
> >
> > -     if(retval) {
> > -             goto fail_free;
> > +     if(remote_targets) {
> > +             retval = alpm_fetch_pkgurl(config->handle, remote_targets, &fetched_files);
> > +             if(retval) {
> > +                     goto fail_free;
> > +             }
> >       }
> >
>
> OK.
>
> >       /* Step 1: create a new transaction */
> > @@ -85,39 +100,16 @@ int pacman_upgrade(alpm_list_t *targets)
> >       }
> >
> >       printf(_("loading packages...\n"));
> > -     /* add targets to the created transaction */
> > -     for(i = targets, n = 0; i; i = alpm_list_next(i), n++) {
> > -             const char *targ = i->data;
> > -             alpm_pkg_t *pkg;
> > -             int siglevel;
> > -
> > -             if(file_is_remote[n]) {
> > -                     siglevel = alpm_option_get_remote_file_siglevel(config->handle);
> > -             } else {
> > -                     siglevel = alpm_option_get_local_file_siglevel(config->handle);
> > -             }
> > -
> > -             if(alpm_pkg_load(config->handle, targ, 1, siglevel, &pkg) != 0) {
> > -                     pm_printf(ALPM_LOG_ERROR, "'%s': %s\n",
> > -                                     targ, alpm_strerror(alpm_errno(config->handle)));
> > -                     retval = 1;
> > -                     continue;
> > -             }
> > -             if(alpm_add_pkg(config->handle, pkg) == -1) {
> > -                     pm_printf(ALPM_LOG_ERROR, "'%s': %s\n",
> > -                                     targ, alpm_strerror(alpm_errno(config->handle)));
> > -                     alpm_pkg_free(pkg);
> > -                     retval = 1;
> > -                     continue;
> > -             }
> > -             config->explicit_adds = alpm_list_add(config->explicit_adds, pkg);
> > -     }
> > +     retval |= load_packages(local_targets, alpm_option_get_local_file_siglevel(config->handle));
> > +     retval |= load_packages(fetched_files, alpm_option_get_remote_file_siglevel(config->handle));
> >
>
> OK.
>
> >       if(retval) {
> >               goto fail_release;
> >       }
> >
> > -     free(file_is_remote);
> > +     alpm_list_free(remote_targets);
> > +     alpm_list_free(local_targets);
> > +     FREELIST(fetched_files);
> >
> >       /* now that targets are resolved, we can hand it all off to the sync code */
> >       return sync_prepare_execute();
> > @@ -125,7 +117,9 @@ int pacman_upgrade(alpm_list_t *targets)
> >  fail_release:
> >       trans_release();
> >  fail_free:
> > -     free(file_is_remote);
> > +     alpm_list_free(remote_targets);
> > +     alpm_list_free(local_targets);
> > +     FREELIST(fetched_files);
> >
> >       return retval;
> >  }
> >
>
> OK.

Patch

diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h
index 3ea66ccc..c6a13273 100644
--- a/lib/libalpm/alpm.h
+++ b/lib/libalpm/alpm.h
@@ -755,12 +755,15 @@  typedef void (*alpm_cb_totaldl)(off_t total);
 typedef int (*alpm_cb_fetch)(const char *url, const char *localpath,
 		int force);
 
-/** Fetch a remote pkg.
+/** Fetch a list of remote packages.
  * @param handle the context handle
- * @param url URL of the package to download
- * @return the downloaded filepath on success, NULL on error
+ * @param urls list of package URLs to download
+ * @param fetched list of filepaths to the fetched packages, each item
+ *    corresponds to one in `urls` list
+ * @return 0 on success or -1 on failure
  */
-char *alpm_fetch_pkgurl(alpm_handle_t *handle, const char *url);
+int alpm_fetch_pkgurl(alpm_handle_t *handle, const alpm_list_t *urls,
+	  alpm_list_t **fetched);
 
 /** @addtogroup alpm_api_options Options
  * Libalpm option getters and setters
diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c
index 13aa4086..43fe9847 100644
--- a/lib/libalpm/dload.c
+++ b/lib/libalpm/dload.c
@@ -613,7 +613,9 @@  static int curl_multi_retry_next_server(CURLM *curlm, CURL *curl, struct dload_p
 	size_t len;
 	alpm_handle_t *handle = payload->handle;
 
-	payload->servers = payload->servers->next;
+	if(payload->servers) {
+		payload->servers = payload->servers->next;
+	}
 	if(!payload->servers) {
 		_alpm_log(payload->handle, ALPM_LOG_DEBUG,
 				"%s: no more servers to retry\n", payload->remote_name);
@@ -622,7 +624,7 @@  static int curl_multi_retry_next_server(CURLM *curlm, CURL *curl, struct dload_p
 	server = payload->servers->data;
 
 	/* regenerate a new fileurl */
-	free(payload->fileurl);
+	FREE(payload->fileurl);
 	len = strlen(server) + strlen(payload->filepath) + 2;
 	MALLOC(payload->fileurl, len, RET_ERR(handle, ALPM_ERR_MEMORY, -1));
 	snprintf(payload->fileurl, len, "%s/%s", server, payload->filepath);
@@ -849,20 +851,28 @@  static int curl_multi_add_payload(alpm_handle_t *handle, CURLM *curlm,
 		struct dload_payload *payload, const char *localpath)
 {
 	size_t len;
-	const char *server;
 	CURL *curl = NULL;
 	char hostname[HOSTNAME_SIZE];
 	int ret = -1;
 
-	ASSERT(payload->servers, RET_ERR(handle, ALPM_ERR_SERVER_NONE, -1));
-	server = payload->servers->data;
-
 	curl = curl_easy_init();
 	payload->curl = curl;
 
-	len = strlen(server) + strlen(payload->filepath) + 2;
-	MALLOC(payload->fileurl, len, GOTO_ERR(handle, ALPM_ERR_MEMORY, cleanup));
-	snprintf(payload->fileurl, len, "%s/%s", server, payload->filepath);
+	if(payload->fileurl) {
+		ASSERT(!payload->servers, GOTO_ERR(handle, ALPM_ERR_WRONG_ARGS, cleanup));
+		ASSERT(!payload->filepath, GOTO_ERR(handle, ALPM_ERR_WRONG_ARGS, cleanup));
+	} else {
+		const char *server;
+
+		ASSERT(payload->servers, GOTO_ERR(handle, ALPM_ERR_SERVER_NONE, cleanup));
+		ASSERT(payload->filepath, GOTO_ERR(handle, ALPM_ERR_WRONG_ARGS, cleanup));
+
+		server = payload->servers->data;
+
+		len = strlen(server) + strlen(payload->filepath) + 2;
+		MALLOC(payload->fileurl, len, GOTO_ERR(handle, ALPM_ERR_MEMORY, cleanup));
+		snprintf(payload->fileurl, len, "%s/%s", server, payload->filepath);
+	}
 
 	payload->tempfile_openmode = "wb";
 	if(!payload->remote_name) {
@@ -1046,22 +1056,29 @@  int _alpm_multi_download(alpm_handle_t *handle,
 			alpm_list_t *s;
 			int success = 0;
 
-			for(s = payload->servers; s; s = s->next) {
-				const char *server = s->data;
-				char *fileurl;
-				int ret;
-
-				size_t len = strlen(server) + strlen(payload->filepath) + 2;
-				MALLOC(fileurl, len, RET_ERR(handle, ALPM_ERR_MEMORY, -1));
-				snprintf(fileurl, len, "%s/%s", server, payload->filepath);
-
-				ret = handle->fetchcb(fileurl, localpath, payload->force);
-				free(fileurl);
-
-				if (ret != -1) {
+			if(payload->fileurl) {
+				if (handle->fetchcb(payload->fileurl, localpath, payload->force) != -1) {
 					success = 1;
 					break;
 				}
+			} else {
+				for(s = payload->servers; s; s = s->next) {
+					const char *server = s->data;
+					char *fileurl;
+					int ret;
+
+					size_t len = strlen(server) + strlen(payload->filepath) + 2;
+					MALLOC(fileurl, len, RET_ERR(handle, ALPM_ERR_MEMORY, -1));
+					snprintf(fileurl, len, "%s/%s", server, payload->filepath);
+
+					ret = handle->fetchcb(fileurl, localpath, payload->force);
+					free(fileurl);
+
+					if (ret != -1) {
+						success = 1;
+						break;
+					}
+				}
 			}
 			if(!success && !payload->errors_ok) {
 				RET_ERR(handle, ALPM_ERR_EXTERNAL_DOWNLOAD, -1);
@@ -1087,82 +1104,101 @@  static char *filecache_find_url(alpm_handle_t *handle, const char *url)
 	return _alpm_filecache_find(handle, filebase);
 }
 
-char SYMEXPORT *alpm_fetch_pkgurl(alpm_handle_t *handle, const char *url)
+int SYMEXPORT alpm_fetch_pkgurl(alpm_handle_t *handle, const alpm_list_t *urls,
+	  alpm_list_t **fetched)
 {
-	char *filepath;
-	const char *cachedir, *final_pkg_url = NULL;
-	char *final_file = NULL;
-	struct dload_payload payload = {0};
-	int ret = 0;
+	const char *cachedir;
+	alpm_list_t *payloads = NULL;
+	int download_sigs;
+	const alpm_list_t *i;
+	alpm_event_t event;
 
-	CHECK_HANDLE(handle, return NULL);
-	ASSERT(url, RET_ERR(handle, ALPM_ERR_WRONG_ARGS, NULL));
+	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);
 
-	/* attempt to find the file in our pkgcache */
-	filepath = filecache_find_url(handle, url);
-	if(filepath == NULL) {
-		STRDUP(payload.fileurl, url, RET_ERR(handle, ALPM_ERR_MEMORY, NULL));
-		payload.allow_resume = 1;
-		payload.handle = handle;
-		payload.trust_remote_name = 1;
-
-		/* download the file */
-		ret = _alpm_download(&payload, cachedir, &final_file, &final_pkg_url);
-		_alpm_dload_payload_reset(&payload);
-		if(ret == -1) {
-			_alpm_log(handle, ALPM_LOG_WARNING, _("failed to download %s\n"), url);
-			free(final_file);
-			return NULL;
+	for(i = urls; i; i = i->next) {
+		char *url = i->data;
+
+		/* attempt to find the file in our pkgcache */
+		char *filepath = filecache_find_url(handle, url);
+		if(filepath) {
+			/* the file is locally cached so add it to the output right away */
+			alpm_list_append(fetched, filepath);
+		} else {
+			struct dload_payload *payload = NULL;
+
+			ASSERT(url, GOTO_ERR(handle, ALPM_ERR_WRONG_ARGS, err));
+			CALLOC(payload, 1, sizeof(*payload), GOTO_ERR(handle, ALPM_ERR_MEMORY, err));
+			STRDUP(payload->fileurl, url, FREE(payload); GOTO_ERR(handle, ALPM_ERR_MEMORY, err));
+			payload->allow_resume = 1;
+			payload->handle = handle;
+			payload->trust_remote_name = 1;
+			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);
+			}
 		}
-		_alpm_log(handle, ALPM_LOG_DEBUG, "successfully downloaded %s\n", url);
 	}
 
-	/* attempt to download the signature */
-	if(ret == 0 && final_pkg_url && (handle->siglevel & ALPM_SIG_PACKAGE)) {
-		char *sig_filepath, *sig_final_file = NULL;
-		size_t len;
-
-		len = strlen(final_pkg_url) + 5;
-		MALLOC(payload.fileurl, len, free(final_file); RET_ERR(handle, ALPM_ERR_MEMORY, NULL));
-		snprintf(payload.fileurl, len, "%s.sig", final_pkg_url);
-
-		sig_filepath = filecache_find_url(handle, payload.fileurl);
-		if(sig_filepath == NULL) {
-			payload.signature = 1;
-			payload.handle = handle;
-			payload.trust_remote_name = 1;
-			payload.force = 1;
-			payload.errors_ok = (handle->siglevel & ALPM_SIG_PACKAGE_OPTIONAL);
-
-			/* set hard upper limit of 16KiB */
-			payload.max_size = 16 * 1024;
-
-			ret = _alpm_download(&payload, cachedir, &sig_final_file, NULL);
-			if(ret == -1 && !payload.errors_ok) {
-				_alpm_log(handle, ALPM_LOG_WARNING,
-						_("failed to download %s\n"), payload.fileurl);
-				/* Warn now, but don't return NULL. We will fail later during package
-				 * load time. */
-			} else if(ret == 0) {
-				_alpm_log(handle, ALPM_LOG_DEBUG,
-						"successfully downloaded %s\n", payload.fileurl);
+	if(payloads) {
+		event.type = ALPM_EVENT_PKG_RETRIEVE_START;
+		EVENT(handle, &event);
+		if(_alpm_multi_download(handle, payloads, cachedir) == -1) {
+			_alpm_log(handle, ALPM_LOG_WARNING, _("failed to retrieve some files\n"));
+			event.type = ALPM_EVENT_PKG_RETRIEVE_FAILED;
+			EVENT(handle, &event);
+
+			GOTO_ERR(handle, ALPM_ERR_RETRIEVE, err);
+		} else {
+			event.type = ALPM_EVENT_PKG_RETRIEVE_DONE;
+			EVENT(handle, &event);
+		}
+
+		for(i = payloads; i; i = i->next) {
+			struct dload_payload *payload = i->data;
+			const char *filename;
+			char *filepath;
+
+			if(payload->signature) {
+				continue;
+			}
+
+			filename = mbasename(payload->destfile_name);
+			filepath = _alpm_filecache_find(handle, filename);
+			if(filepath) {
+				alpm_list_append(fetched, filepath);
+			} else {
+				_alpm_log(handle, ALPM_LOG_WARNING, _("download completed successfully but no file in the cache\n"));
+				GOTO_ERR(handle, ALPM_ERR_RETRIEVE, err);
 			}
-			FREE(sig_final_file);
 		}
-		free(sig_filepath);
-		_alpm_dload_payload_reset(&payload);
-	}
 
-	/* we should be able to find the file the second time around */
-	if(filepath == NULL) {
-		filepath = _alpm_filecache_find(handle, final_file);
+		alpm_list_free_inner(payloads, (alpm_list_fn_free)_alpm_dload_payload_reset);
+		FREELIST(payloads);
 	}
-	free(final_file);
 
-	return filepath;
+	return 0;
+
+err:
+	alpm_list_free_inner(payloads, (alpm_list_fn_free)_alpm_dload_payload_reset);
+	FREELIST(payloads);
+	FREELIST(*fetched);
+
+	return -1;
 }
 
 void _alpm_dload_payload_reset(struct dload_payload *payload)
diff --git a/lib/libalpm/dload.h b/lib/libalpm/dload.h
index 1e4af755..f119fc2e 100644
--- a/lib/libalpm/dload.h
+++ b/lib/libalpm/dload.h
@@ -30,9 +30,14 @@  struct dload_payload {
 	char *tempfile_name;
 	char *destfile_name;
 	char *content_disp_name;
+	/* client has to provide either
+	 *  1) fileurl - full URL to the file
+	 *  2) pair of (servers, filepath), in this case ALPM iterates over the
+	 *     server list and tries to download "$server/$filepath"
+	 */
 	char *fileurl;
-	char *filepath; /* download URL path */
 	alpm_list_t *servers;
+	char *filepath; /* download URL path */
 	long respcode;
 	off_t initial_size;
 	off_t max_size;
diff --git a/src/pacman/upgrade.c b/src/pacman/upgrade.c
index 5f984e44..840e3a31 100644
--- a/src/pacman/upgrade.c
+++ b/src/pacman/upgrade.c
@@ -30,6 +30,34 @@ 
 #include "conf.h"
 #include "util.h"
 
+/* add targets to the created transaction */
+static int load_packages(alpm_list_t *targets, int siglevel)
+{
+	alpm_list_t *i;
+	int retval = 0;
+
+	for(i = targets; i; i = alpm_list_next(i)) {
+		const char *targ = i->data;
+		alpm_pkg_t *pkg;
+
+		if(alpm_pkg_load(config->handle, targ, 1, siglevel, &pkg) != 0) {
+			pm_printf(ALPM_LOG_ERROR, "'%s': %s\n",
+					targ, alpm_strerror(alpm_errno(config->handle)));
+			retval = 1;
+			continue;
+		}
+		if(alpm_add_pkg(config->handle, pkg) == -1) {
+			pm_printf(ALPM_LOG_ERROR, "'%s': %s\n",
+					targ, alpm_strerror(alpm_errno(config->handle)));
+			alpm_pkg_free(pkg);
+			retval = 1;
+			continue;
+		}
+		config->explicit_adds = alpm_list_add(config->explicit_adds, pkg);
+	}
+	return retval;
+}
+
 /**
  * @brief Upgrade a specified list of packages.
  *
@@ -39,43 +67,30 @@ 
  */
 int pacman_upgrade(alpm_list_t *targets)
 {
-	int retval = 0, *file_is_remote;
+	int retval = 0;
+	alpm_list_t *remote_targets = NULL, *fetched_files = NULL;
+	alpm_list_t *local_targets = NULL;
 	alpm_list_t *i;
-	unsigned int n, num_targets;
 
 	if(targets == NULL) {
 		pm_printf(ALPM_LOG_ERROR, _("no targets specified (use -h for help)\n"));
 		return 1;
 	}
 
-	num_targets = alpm_list_count(targets);
-
-	/* Check for URL targets and process them */
-	file_is_remote = malloc(num_targets * sizeof(int));
-	if(file_is_remote == NULL) {
-		pm_printf(ALPM_LOG_ERROR, _("memory exhausted\n"));
-		return 1;
-	}
-
-	for(i = targets, n = 0; i; i = alpm_list_next(i), n++) {
+	/* carve out remote targets and move it into a separate list */
+	for(i = targets; i; i = alpm_list_next(i)) {
 		if(strstr(i->data, "://")) {
-			char *str = alpm_fetch_pkgurl(config->handle, i->data);
-			if(str == NULL) {
-				pm_printf(ALPM_LOG_ERROR, "'%s': %s\n",
-						(char *)i->data, alpm_strerror(alpm_errno(config->handle)));
-				retval = 1;
-			} else {
-				free(i->data);
-				i->data = str;
-				file_is_remote[n] = 1;
-			}
+			remote_targets = alpm_list_add(remote_targets, i->data);
 		} else {
-			file_is_remote[n] = 0;
+			local_targets = alpm_list_add(local_targets, i->data);
 		}
 	}
 
-	if(retval) {
-		goto fail_free;
+	if(remote_targets) {
+		retval = alpm_fetch_pkgurl(config->handle, remote_targets, &fetched_files);
+		if(retval) {
+			goto fail_free;
+		}
 	}
 
 	/* Step 1: create a new transaction */
@@ -85,39 +100,16 @@  int pacman_upgrade(alpm_list_t *targets)
 	}
 
 	printf(_("loading packages...\n"));
-	/* add targets to the created transaction */
-	for(i = targets, n = 0; i; i = alpm_list_next(i), n++) {
-		const char *targ = i->data;
-		alpm_pkg_t *pkg;
-		int siglevel;
-
-		if(file_is_remote[n]) {
-			siglevel = alpm_option_get_remote_file_siglevel(config->handle);
-		} else {
-			siglevel = alpm_option_get_local_file_siglevel(config->handle);
-		}
-
-		if(alpm_pkg_load(config->handle, targ, 1, siglevel, &pkg) != 0) {
-			pm_printf(ALPM_LOG_ERROR, "'%s': %s\n",
-					targ, alpm_strerror(alpm_errno(config->handle)));
-			retval = 1;
-			continue;
-		}
-		if(alpm_add_pkg(config->handle, pkg) == -1) {
-			pm_printf(ALPM_LOG_ERROR, "'%s': %s\n",
-					targ, alpm_strerror(alpm_errno(config->handle)));
-			alpm_pkg_free(pkg);
-			retval = 1;
-			continue;
-		}
-		config->explicit_adds = alpm_list_add(config->explicit_adds, pkg);
-	}
+	retval |= load_packages(local_targets, alpm_option_get_local_file_siglevel(config->handle));
+	retval |= load_packages(fetched_files, alpm_option_get_remote_file_siglevel(config->handle));
 
 	if(retval) {
 		goto fail_release;
 	}
 
-	free(file_is_remote);
+	alpm_list_free(remote_targets);
+	alpm_list_free(local_targets);
+	FREELIST(fetched_files);
 
 	/* now that targets are resolved, we can hand it all off to the sync code */
 	return sync_prepare_execute();
@@ -125,7 +117,9 @@  int pacman_upgrade(alpm_list_t *targets)
 fail_release:
 	trans_release();
 fail_free:
-	free(file_is_remote);
+	alpm_list_free(remote_targets);
+	alpm_list_free(local_targets);
+	FREELIST(fetched_files);
 
 	return retval;
 }