[pacman-dev] [PATCH 3/5] sync: dont group sync records by repository
Dave Reisner
d at falconindy.com
Sun Oct 16 17:30:44 EDT 2011
On Sun, Oct 16, 2011 at 03:05:35PM -0500, Dan McGee wrote:
> On Sun, Oct 16, 2011 at 1:59 PM, Dave Reisner <d at 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 at 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?
Brainfart. I've conditionally added (in a separate patch) the repo to
the verbose package display (since it will be NULL on a -U operation).
> > Additionally, if a download fails, you're just
> > left with "failed to retreive some packages".
> Hopefully we spelled it as "retrieve". :)
"We" did!
> >
> > 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...
They also seem to go along nicely with our grandiose plans to move a lot
of the progress calculations to the backend.
d
> > 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
> >
> >
> >
>
More information about the pacman-dev
mailing list