16 Oct
2011
16 Oct
'11
8:05 p.m.
On Sun, Oct 16, 2011 at 1:59 PM, Dave Reisner <d@falconindy.com> wrote:
s/dont/don't/
> Break out the logic of finding payloads into a separate static function
> to avoid nesting mayhem. After gathering all the records, download them
> all at once.
>
> Signed-off-by: Dave Reisner <dreisner@archlinux.org>
> ---
> This is mostly just code movement, but there's some string changes in here I'm
> not entirely satisfied with. Addition of this patch means that the user never
> sees where packages come from -- something I consider to be a loss of useful
> information (particulary if you're on [testing]). It didn't look straight
> forward to add this data to the download prompt -- alpm_pkg_get_db() returns
> garbage when called from pacman.
This is surprising to hear- I know we have this problem once we load
the packages off disk in sync.c/load_packages(), but it really isn't
working here?
> Additionally, if a download fails, you're just
> left with "failed to retreive some packages".
Hopefully we spelled it as "retrieve". :)
>
> Ideas?
1. Right now we have the hacky frontend removal of extensions, etc.
from the package and db downloads, and re-appending .sig and all that
mess. It would seem wise to me for the backend to provide a
display_name string to the frontend, and we could pass things like
this:
* community
* community.sig
* testing/pacman
* testing/pacman.sig
Thoughts? Are package versions important here, given you just saw them
anyway when you chose what to install?
2. As far as the error message when a download fails, this might be
the time to start splitting up the absolutely braindead do-everything
alpm_sync_commit() and move the entire download stuff to a new API
call (not to mention not needing to reducing the time we hold a DB
lock in the long-term vision). This would then allow the return value
from this to be an int, and the frontend could pass in a pointer that
would serve a similar purpose to *data on sync commit but be limited
to download errors only, with some sane type in the returned list. The
error display could then be improved dramatically.
Of course, both of these are rather large in scope suggestions...
> lib/libalpm/sync.c | 160 +++++++++++++++++++++++++-----------------------
> src/pacman/callback.c | 2 +-
> 2 files changed, 84 insertions(+), 78 deletions(-)
>
> diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c
> index f7147db..cac4c8f 100644
> --- a/lib/libalpm/sync.c
> +++ b/lib/libalpm/sync.c
> @@ -790,11 +790,64 @@ static int validate_deltas(alpm_handle_t *handle, alpm_list_t *deltas)
> return 0;
> }
>
> +static int find_dl_candidates(alpm_db_t *repo, alpm_list_t **files, alpm_list_t **deltas)
> +{
> + alpm_list_t *i;
> + alpm_handle_t *handle = repo->handle;
> +
> + for(i = handle->trans->add; i; i = i->next) {
> + alpm_pkg_t *spkg = i->data;
> +
> + if(spkg->origin != PKG_FROM_FILE && repo == spkg->origin_data.db) {
> + alpm_list_t *delta_path = spkg->delta_path;
> +
> + if(!repo->servers) {
> + handle->pm_errno = ALPM_ERR_SERVER_NONE;
> + _alpm_log(handle, ALPM_LOG_ERROR, "%s: %s\n",
> + alpm_strerror(handle->pm_errno), repo->treename);
> + return 1;
> + }
> +
> + if(delta_path) {
> + /* using deltas */
> + alpm_list_t *dlts;
> + for(dlts = delta_path; dlts; dlts = dlts->next) {
> + alpm_delta_t *delta = dlts->data;
> + if(delta->download_size != 0) {
> + struct dload_payload *dpayload;
> +
> + CALLOC(dpayload, 1, sizeof(*dpayload), RET_ERR(handle, ALPM_ERR_MEMORY, -1));
> + STRDUP(dpayload->remote_name, delta->delta, RET_ERR(handle, ALPM_ERR_MEMORY, -1));
> + dpayload->max_size = delta->download_size;
> + dpayload->servers = repo->servers;
> +
> + *files = alpm_list_add(*files, dpayload);
> + }
> + /* keep a list of all the delta files for md5sums */
> + *deltas = alpm_list_add(*deltas, delta);
> + }
> +
> + } else if(spkg->download_size != 0) {
> + struct dload_payload *payload;
> +
> + ASSERT(spkg->filename != NULL, RET_ERR(handle, ALPM_ERR_PKG_INVALID_NAME, -1));
> + CALLOC(payload, 1, sizeof(*payload), RET_ERR(handle, ALPM_ERR_MEMORY, -1));
> + STRDUP(payload->remote_name, spkg->filename, RET_ERR(handle, ALPM_ERR_MEMORY, -1));
> + payload->max_size = spkg->size;
> + payload->servers = repo->servers;
> +
> + *files = alpm_list_add(*files, payload);
> + }
> + }
> + }
> +
> + return 0;
> +}
> +
> static int download_files(alpm_handle_t *handle, alpm_list_t **deltas)
> {
> const char *cachedir;
> - alpm_list_t *i, *j;
> - alpm_list_t *files = NULL;
> + alpm_list_t *i, *files = NULL;
> int errors = 0;
>
> cachedir = _alpm_filecache_setup(handle);
> @@ -813,93 +866,46 @@ static int download_files(alpm_handle_t *handle, alpm_list_t **deltas)
> handle->totaldlcb(total_size);
> }
>
> - /* group sync records by repository and download */
> + /* gather sync records needed to be downloaded */
Might as well just kill this comment completely as the function name
in the loop seems adequate.
> for(i = handle->dbs_sync; i; i = i->next) {
> - alpm_db_t *current = i->data;
> -
> - for(j = handle->trans->add; j; j = j->next) {
> - alpm_pkg_t *spkg = j->data;
> -
> - if(spkg->origin != PKG_FROM_FILE && current == spkg->origin_data.db) {
> - alpm_list_t *delta_path = spkg->delta_path;
> - if(delta_path) {
> - /* using deltas */
> - alpm_list_t *dlts;
> - for(dlts = delta_path; dlts; dlts = dlts->next) {
> - alpm_delta_t *delta = dlts->data;
> - if(delta->download_size != 0) {
> - struct dload_payload *dpayload;
> -
> - CALLOC(dpayload, 1, sizeof(*dpayload), RET_ERR(handle, ALPM_ERR_MEMORY, -1));
> - STRDUP(dpayload->remote_name, delta->delta, RET_ERR(handle, ALPM_ERR_MEMORY, -1));
> - dpayload->max_size = delta->download_size;
> - dpayload->servers = current->servers;
> + errors += find_dl_candidates(i->data, &files, deltas);
> + }
>
> - files = alpm_list_add(files, dpayload);
> - }
> - /* keep a list of all the delta files for md5sums */
> - *deltas = alpm_list_add(*deltas, delta);
> - }
> + if(files) {
> + EVENT(handle, ALPM_EVENT_RETRIEVE_START, NULL, NULL);
> + for(i = files; i; i = i->next) {
> + struct dload_payload *payload = i->data;
> + const alpm_list_t *server;
> + int ret = -1;
>
> - } else if(spkg->download_size != 0) {
> - struct dload_payload *payload;
> + for(server = payload->servers; server; server = server->next) {
> + const char *server_url = server->data;
> + size_t len;
>
> - ASSERT(spkg->filename != NULL, RET_ERR(handle, ALPM_ERR_PKG_INVALID_NAME, -1));
> - CALLOC(payload, 1, sizeof(*payload), RET_ERR(handle, ALPM_ERR_MEMORY, -1));
> - STRDUP(payload->remote_name, spkg->filename, RET_ERR(handle, ALPM_ERR_MEMORY, -1));
> - payload->max_size = spkg->size;
> - payload->servers = current->servers;
> + /* 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);
> + payload->handle = handle;
> + payload->allow_resume = 1;
>
> - files = alpm_list_add(files, payload);
> + ret = _alpm_download(payload, cachedir, NULL);
> + if(ret != -1) {
> + break;
> }
> -
> }
> - }
> -
> - if(files) {
> - if(!current->servers) {
> - handle->pm_errno = ALPM_ERR_SERVER_NONE;
> - _alpm_log(handle, ALPM_LOG_ERROR, "%s: %s\n",
> - alpm_strerror(handle->pm_errno), current->treename);
> + if(ret == -1) {
> errors++;
> - continue;
> + _alpm_log(handle, ALPM_LOG_WARNING, _("failed to retrieve some files\n"));
> }
> -
> - EVENT(handle, ALPM_EVENT_RETRIEVE_START, current->treename, NULL);
> - for(j = files; j; j = j->next) {
> - struct dload_payload *payload = j->data;
> - alpm_list_t *server;
> - int ret = -1;
> - for(server = current->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);
> - payload->handle = handle;
> - payload->allow_resume = 1;
> -
> - ret = _alpm_download(payload, cachedir, NULL);
> - if(ret != -1) {
> - break;
> - }
> - }
> - if(ret == -1) {
> - errors++;
> - _alpm_log(handle, ALPM_LOG_WARNING, _("failed to retrieve some files from %s\n"),
> - current->treename);
> - }
> - }
> -
> - alpm_list_free_inner(files, (alpm_list_fn_free)_alpm_dload_payload_reset);
> - FREELIST(files);
> }
> +
> + alpm_list_free_inner(files, (alpm_list_fn_free)_alpm_dload_payload_reset);
> + FREELIST(files);
> }
>
> - for(j = handle->trans->add; j; j = j->next) {
> - alpm_pkg_t *pkg = j->data;
> + for(i = handle->trans->add; i; i = i->next) {
> + alpm_pkg_t *pkg = i->data;
> pkg->infolevel &= ~INFRQ_DSIZE;
> pkg->download_size = 0;
> }
> diff --git a/src/pacman/callback.c b/src/pacman/callback.c
> index d856455..c9846b0 100644
> --- a/src/pacman/callback.c
> +++ b/src/pacman/callback.c
> @@ -231,7 +231,7 @@ void cb_event(alpm_event_t event, void *data1, void *data2)
> fputs((const char *)data1, stdout);
> break;
> case ALPM_EVENT_RETRIEVE_START:
> - printf(_(":: Retrieving packages from %s...\n"), (char *)data1);
> + fputs(_(":: Retrieving packages ...\n"), stdout);
> break;
> case ALPM_EVENT_DISKSPACE_START:
> if(config->noprogressbar) {
> --
> 1.7.7
>
>
>