Re: [pacman-dev] [PATCH] Convert download packages logic to multiplexed API

Hello On Tue, Apr 28, 2020 at 9:36 PM Allan McRae wrote:

On 19/4/20 12:04 pm, Anatol Pomozov wrote:

...

Create a list of dload_payloads and pass it to the new _alpm_multi_* interface.

Signed-off-by: Anatol Pomozov --- lib/libalpm/sync.c | 74 ++++++++++++++++++---------------------------- 1 file changed, 28 insertions(+), 46 deletions(-)

Deletions!

:D and more deletions are coming...

...

diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c index 8a9dcae8..aa417bca 100644 --- a/lib/libalpm/sync.c +++ b/lib/libalpm/sync.c @@ -726,47 +726,13 @@ static int find_dl_candidates(alpm_handle_t *handle, alpm_list_t **files) return 0; }

-static int download_single_file(alpm_handle_t *handle, struct dload_payload *payload, - const char *cachedir) -{ - alpm_event_pkgdownload_t event = { - .type = ALPM_EVENT_PKGDOWNLOAD_START, - .file = payload->remote_name - }; - const alpm_list_t *server; - - payload->handle = handle; - payload->allow_resume = 1; - - EVENT(handle, &event); - for(server = payload->servers; server; server = server->next) { - const char *server_url = server->data; - size_t len; - - /* print server + filename into a buffer */ - len = strlen(server_url) + strlen(payload->remote_name) + 2; - MALLOC(payload->fileurl, len, RET_ERR(handle, ALPM_ERR_MEMORY, -1)); - snprintf(payload->fileurl, len, "%s/%s", server_url, payload->remote_name); - - if(_alpm_download(payload, cachedir, NULL, NULL) != -1) { - event.type = ALPM_EVENT_PKGDOWNLOAD_DONE; - EVENT(handle, &event); - return 0; - } - _alpm_dload_payload_reset_for_retry(payload); - } - - event.type = ALPM_EVENT_PKGDOWNLOAD_FAILED; - EVENT(handle, &event); - return -1; -} - static int download_files(alpm_handle_t *handle) { const char *cachedir; alpm_list_t *i, *files = NULL; int errors = 0; alpm_event_t event; + alpm_list_t *payloads = NULL;

cachedir = _alpm_filecache_setup(handle); handle->trans->state = STATE_DOWNLOADING; @@ -814,26 +780,42 @@ static int download_files(alpm_handle_t *handle)

event.type = ALPM_EVENT_RETRIEVE_START; EVENT(handle, &event); - event.type = ALPM_EVENT_RETRIEVE_DONE; for(i = files; i; i = i->next) { const alpm_pkg_t *pkg = i->data; - struct dload_payload payload = {0}; + struct dload_payload *payload = NULL;

- STRDUP(payload.remote_name, pkg->filename, GOTO_ERR(handle, ALPM_ERR_MEMORY, finish)); - payload.servers = pkg->origin_data.db->servers; - payload.max_size = pkg->size; + CALLOC(payload, 1, sizeof(*payload), GOTO_ERR(handle, ALPM_ERR_MEMORY, finish)); + STRDUP(payload->remote_name, pkg->filename, GOTO_ERR(handle, ALPM_ERR_MEMORY, finish)); + STRDUP(payload->filepath, pkg->filename, GOTO_ERR(handle, ALPM_ERR_MEMORY, finish));

My suspicion that the payload struct now has redundant information increases... But as I said at the time, we need to set up a good download testsuite so we can detect any breakage of edge cases.

...

+ payload->max_size = pkg->size; + payload->servers = pkg->origin_data.db->servers; + payload->handle = handle; + payload->allow_resume = 1;

OK.

...

- if(download_single_file(handle, &payload, cachedir) == -1) { - errors++; - event.type = ALPM_EVENT_RETRIEVE_FAILED; - _alpm_log(handle, ALPM_LOG_WARNING, _("failed to retrieve some files\n")); - } - _alpm_dload_payload_reset(&payload); + payloads = alpm_list_add(payloads, payload); + + /* TOASK: do we need to initiate *.sig file download here? */

As discussed on IRC - we should. That way we can stop distributing them in repo dbs.

Ack. This *.sig file download functionality is implemented in the subsequent commit that currently stays in my WIP branch here https://github.com/anatol/pacman/tree/parallel-download

...

+ } + /* TOASK: the sequential download logic reported ALPM_EVENT_PKGDOWNLOAD_START/DONE/FAILED event. + * Do we want to report the event as well? If yes then it should be moved inside alpm_multi_*() + * function but at that point ALPM operates with dload_payload() and does not care if it downloads + * package or database file. + */

These events were not used by pacman. My recollection is that it was added to allow other frontends to monitor download progress.

Following my comments on an earlier patch about adding ALPM_EVENT_DB_RETRIEVE_* and changing ALPM_EVENT_RETRIEVE_* to ALPM_EVENT_PKG_RETRIEVE_*, I think this is covered enough for other frontends if they need it.

You can remove ALPM_EVENT_PKGDOWNLOAD_* from the codebase. Other frontends will need to overhaul for parallel download anyway.

SGTM. I am removing these unused events. I also added information about the API change to README.

...

+ event.type = ALPM_EVENT_RETRIEVE_DONE; + if(_alpm_multi_download(handle, payloads, cachedir) == -1) { + errors++;

errors = 1; Just to avoid it looking like we are counting something.

Yeah, this kind of error handling looks weird. I am changing it to a more typical "int ret = -1" style in one of the future patches where I add downloading *.sig file functionality.

...

+ event.type = ALPM_EVENT_RETRIEVE_FAILED; + _alpm_log(handle, ALPM_LOG_WARNING, _("failed to retrieve some files\n")); }

OK.

...

EVENT(handle, &event); }

finish: + if(payloads) { + alpm_list_free_inner(payloads, (alpm_list_fn_free)_alpm_dload_payload_reset); + FREELIST(payloads); + } + if(files) { alpm_list_free(files); }