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
> };
>
>
