[pacman-dev] [PATCH 0/5] download diskspace checking
I don't think this patchset is ready for merging, but I figured I'd post it here for some possible feedback. This is mostly for the purposes of adding diskspace checking prior to downloads, but it also changes some ancient behavior that Judd added of grouping sync records by repo. Instead, we just gather all the records together at once and download them. I've left some comments in the header for each patch... d Dave Reisner (5): diskspace: add _alpm_check_downloadspace() dload: add RO copy of servers list to each payload sync: dont group sync records by repository sync: check for necessary disk space for download sync: move file download loop out of download_files lib/libalpm/diskspace.c | 54 ++++++++++++++ lib/libalpm/diskspace.h | 2 + lib/libalpm/dload.h | 1 + lib/libalpm/sync.c | 184 ++++++++++++++++++++++++++++------------------ src/pacman/callback.c | 2 +- 5 files changed, 170 insertions(+), 73 deletions(-) -- 1.7.7
This function determines if the given cachedir has at least the given amount of free space on it. This will be later used in the sync code to preemptively halt downloads. Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- lib/libalpm/diskspace.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++ lib/libalpm/diskspace.h | 2 + 2 files changed, 56 insertions(+), 0 deletions(-) diff --git a/lib/libalpm/diskspace.c b/lib/libalpm/diskspace.c index 9dedbe1..d376444 100644 --- a/lib/libalpm/diskspace.c +++ b/lib/libalpm/diskspace.c @@ -253,6 +253,60 @@ static int check_mountpoint(alpm_handle_t *handle, alpm_mountpoint_t *mp) return 0; } +int _alpm_check_downloadspace(alpm_handle_t *handle, const char *cachedir, + size_t num_files, off_t *file_sizes) +{ + alpm_list_t *i, *mount_points; + alpm_mountpoint_t *cachedir_mp; + fsblkcnt_t blocks_needed = 0; + size_t j; + int error = 0; + + mount_points = mount_point_list(handle); + if(mount_points == NULL) { + _alpm_log(handle, ALPM_LOG_ERROR, _("could not determine filesystem mount points\n")); + return -1; + } + + cachedir_mp = match_mount_point(mount_points, cachedir); + if(cachedir == NULL) { + _alpm_log(handle, ALPM_LOG_ERROR, _("could not determine cachedir mount point %s\n"), + cachedir); + error = 1; + goto finish; + } + + /* there's no need to check for a R/O mounted filesystem here, as + * _alpm_filecache_setup will never give us a non-writeable directory */ + + /* round up the size of each file to the nearest block and accumulate */ + for(j = 0; j < num_files; j++) { + blocks_needed += (file_sizes[j] + cachedir_mp->fsp.f_bsize + 1) / + cachedir_mp->fsp.f_bsize; + } + + if(cachedir_mp->fsp.f_bfree < blocks_needed) { + _alpm_log(handle, ALPM_LOG_ERROR, + _("Partition %s too full: %jd blocks needed, %jd blocks free\n"), + cachedir_mp->mount_dir, (intmax_t)blocks_needed, + (uintmax_t)cachedir_mp->fsp.f_bfree); + error = 1; + } + +finish: + for(i = mount_points; i; i = i->next) { + alpm_mountpoint_t *data = i->data; + FREE(data->mount_dir); + } + FREELIST(mount_points); + + if(error) { + RET_ERR(handle, ALPM_ERR_DISK_SPACE, -1); + } + + return 0; +} + int _alpm_check_diskspace(alpm_handle_t *handle) { alpm_list_t *mount_points, *i; diff --git a/lib/libalpm/diskspace.h b/lib/libalpm/diskspace.h index 5944bb1..a613e23 100644 --- a/lib/libalpm/diskspace.h +++ b/lib/libalpm/diskspace.h @@ -50,6 +50,8 @@ typedef struct __alpm_mountpoint_t { } alpm_mountpoint_t; int _alpm_check_diskspace(alpm_handle_t *handle); +int _alpm_check_downloadspace(alpm_handle_t *handle, const char *cachedir, + size_t num_files, off_t *file_sizes); #endif /* _ALPM_DISKSPACE_H */ -- 1.7.7
This function determines if the given cachedir has at least the given amount of free space on it. This will be later used in the sync code to preemptively halt downloads.
Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- lib/libalpm/diskspace.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++ lib/libalpm/diskspace.h | 2 + 2 files changed, 56 insertions(+), 0 deletions(-)
diff --git a/lib/libalpm/diskspace.c b/lib/libalpm/diskspace.c index 9dedbe1..d376444 100644 --- a/lib/libalpm/diskspace.c +++ b/lib/libalpm/diskspace.c @@ -253,6 +253,60 @@ static int check_mountpoint(alpm_handle_t *handle, alpm_mountpoint_t *mp) return 0; }
+int _alpm_check_downloadspace(alpm_handle_t *handle, const char *cachedir, + size_t num_files, off_t *file_sizes) +{ + alpm_list_t *i, *mount_points; + alpm_mountpoint_t *cachedir_mp; + fsblkcnt_t blocks_needed = 0; + size_t j; + int error = 0; + + mount_points = mount_point_list(handle); + if(mount_points == NULL) { + _alpm_log(handle, ALPM_LOG_ERROR, _("could not determine filesystem mount points\n")); + return -1; + } + + cachedir_mp = match_mount_point(mount_points, cachedir); + if(cachedir == NULL) { + _alpm_log(handle, ALPM_LOG_ERROR, _("could not determine cachedir mount point %s\n"), + cachedir); + error = 1; + goto finish; + } + + /* there's no need to check for a R/O mounted filesystem here, as + * _alpm_filecache_setup will never give us a non-writeable directory */ s/writeable/writable/ + + /* round up the size of each file to the nearest block and accumulate */ + for(j = 0; j < num_files; j++) { + blocks_needed += (file_sizes[j] + cachedir_mp->fsp.f_bsize + 1) / + cachedir_mp->fsp.f_bsize; + } + + if(cachedir_mp->fsp.f_bfree < blocks_needed) { Can we use the extracted check_mountpoint() for this please so we get
On Sun, Oct 16, 2011 at 1:59 PM, Dave Reisner <d@falconindy.com> wrote: the same 5%/20 MiB cushion for this check that we do on a normal install?
+ _alpm_log(handle, ALPM_LOG_ERROR, + _("Partition %s too full: %jd blocks needed, %jd blocks free\n"), + cachedir_mp->mount_dir, (intmax_t)blocks_needed, + (uintmax_t)cachedir_mp->fsp.f_bfree); + error = 1; + } + +finish: + for(i = mount_points; i; i = i->next) { + alpm_mountpoint_t *data = i->data; + FREE(data->mount_dir); + } + FREELIST(mount_points); Might be worthwhile to add a mountpoint_list_free() static function since we do this in multiple places now. + + if(error) { + RET_ERR(handle, ALPM_ERR_DISK_SPACE, -1); + } + + return 0; +} + int _alpm_check_diskspace(alpm_handle_t *handle) { alpm_list_t *mount_points, *i; diff --git a/lib/libalpm/diskspace.h b/lib/libalpm/diskspace.h index 5944bb1..a613e23 100644 --- a/lib/libalpm/diskspace.h +++ b/lib/libalpm/diskspace.h @@ -50,6 +50,8 @@ typedef struct __alpm_mountpoint_t { } alpm_mountpoint_t;
int _alpm_check_diskspace(alpm_handle_t *handle); +int _alpm_check_downloadspace(alpm_handle_t *handle, const char *cachedir, + size_t num_files, off_t *file_sizes);
#endif /* _ALPM_DISKSPACE_H */
-- 1.7.7
Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- lib/libalpm/dload.h | 1 + lib/libalpm/sync.c | 2 ++ 2 files changed, 3 insertions(+), 0 deletions(-) diff --git a/lib/libalpm/dload.h b/lib/libalpm/dload.h index 27d865d..158e0b7 100644 --- a/lib/libalpm/dload.h +++ b/lib/libalpm/dload.h @@ -38,6 +38,7 @@ struct dload_payload { int allow_resume; int errors_ok; int unlink_on_fail; + const alpm_list_t *servers; #ifdef HAVE_LIBCURL CURLcode curlerr; /* last error produced by curl */ #endif diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c index 968625b..f7147db 100644 --- a/lib/libalpm/sync.c +++ b/lib/libalpm/sync.c @@ -833,6 +833,7 @@ static int download_files(alpm_handle_t *handle, alpm_list_t **deltas) 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; files = alpm_list_add(files, dpayload); } @@ -847,6 +848,7 @@ static int download_files(alpm_handle_t *handle, alpm_list_t **deltas) 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; files = alpm_list_add(files, payload); } -- 1.7.7
Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- Nitpicky, but the commit message is a tad misleading- this isn't a copy but just a pointer, no? So something like "add pointer to server
On Sun, Oct 16, 2011 at 1:59 PM, Dave Reisner <d@falconindy.com> wrote: list for each download payload" might make more sense. -Dan
lib/libalpm/dload.h | 1 + lib/libalpm/sync.c | 2 ++ 2 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/lib/libalpm/dload.h b/lib/libalpm/dload.h index 27d865d..158e0b7 100644 --- a/lib/libalpm/dload.h +++ b/lib/libalpm/dload.h @@ -38,6 +38,7 @@ struct dload_payload { int allow_resume; int errors_ok; int unlink_on_fail; + const alpm_list_t *servers; #ifdef HAVE_LIBCURL CURLcode curlerr; /* last error produced by curl */ #endif diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c index 968625b..f7147db 100644 --- a/lib/libalpm/sync.c +++ b/lib/libalpm/sync.c @@ -833,6 +833,7 @@ static int download_files(alpm_handle_t *handle, alpm_list_t **deltas) 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;
files = alpm_list_add(files, dpayload); } @@ -847,6 +848,7 @@ static int download_files(alpm_handle_t *handle, alpm_list_t **deltas) 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;
files = alpm_list_add(files, payload); } -- 1.7.7
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. Additionally, if a download fails, you're just left with "failed to retreive some packages". Ideas? 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 */ 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
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 > > >
On Sun, Oct 16, 2011 at 03:05:35PM -0500, Dan McGee wrote:
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
On Sun, Oct 16, 2011 at 1:59 PM, Dave Reisner <d@falconindy.com> wrote: s/dont/don't/ 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
Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- lib/libalpm/sync.c | 28 ++++++++++++++++++++++++++++ 1 files changed, 28 insertions(+), 0 deletions(-) diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c index cac4c8f..3c304a3 100644 --- a/lib/libalpm/sync.c +++ b/lib/libalpm/sync.c @@ -872,6 +872,31 @@ static int download_files(alpm_handle_t *handle, alpm_list_t **deltas) } if(files) { + /* check for necessary disk space for download */ + if(handle->checkspace) { + off_t *file_sizes; + size_t num_files; + int idx, ret; + + _alpm_log(handle, ALPM_LOG_DEBUG, "checking available disk space for download\n"); + + num_files = alpm_list_count(files); + CALLOC(file_sizes, num_files, sizeof(off_t), goto finish); + + for(i = files, idx = 0; i; i = i->next, idx++) { + const struct dload_payload *payload = i->data; + file_sizes[idx] = payload->max_size; + } + + ret = _alpm_check_downloadspace(handle, cachedir, num_files, file_sizes); + free(file_sizes); + + if(ret != 0) { + errors++; + goto finish; + } + } + EVENT(handle, ALPM_EVENT_RETRIEVE_START, NULL, NULL); for(i = files; i; i = i->next) { struct dload_payload *payload = i->data; @@ -899,7 +924,10 @@ static int download_files(alpm_handle_t *handle, alpm_list_t **deltas) _alpm_log(handle, ALPM_LOG_WARNING, _("failed to retrieve some files\n")); } } + } +finish: + if(files) { alpm_list_free_inner(files, (alpm_list_fn_free)_alpm_dload_payload_reset); FREELIST(files); } -- 1.7.7
On Sun, Oct 16, 2011 at 1:59 PM, Dave Reisner <d@falconindy.com> wrote:
Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- lib/libalpm/sync.c | 28 ++++++++++++++++++++++++++++ 1 files changed, 28 insertions(+), 0 deletions(-)
diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c index cac4c8f..3c304a3 100644 --- a/lib/libalpm/sync.c +++ b/lib/libalpm/sync.c @@ -872,6 +872,31 @@ static int download_files(alpm_handle_t *handle, alpm_list_t **deltas) }
if(files) { + /* check for necessary disk space for download */ + if(handle->checkspace) { + off_t *file_sizes; + size_t num_files; + int idx, ret; + + _alpm_log(handle, ALPM_LOG_DEBUG, "checking available disk space for download\n"); + + num_files = alpm_list_count(files); + CALLOC(file_sizes, num_files, sizeof(off_t), goto finish); + + for(i = files, idx = 0; i; i = i->next, idx++) { idx should be a size_t too. + const struct dload_payload *payload = i->data; + file_sizes[idx] = payload->max_size; + } + + ret = _alpm_check_downloadspace(handle, cachedir, num_files, file_sizes); + free(file_sizes); + + if(ret != 0) { + errors++; + goto finish; + } + } + EVENT(handle, ALPM_EVENT_RETRIEVE_START, NULL, NULL); for(i = files; i; i = i->next) { struct dload_payload *payload = i->data; @@ -899,7 +924,10 @@ static int download_files(alpm_handle_t *handle, alpm_list_t **deltas) _alpm_log(handle, ALPM_LOG_WARNING, _("failed to retrieve some files\n")); } } + }
+finish: + if(files) { alpm_list_free_inner(files, (alpm_list_fn_free)_alpm_dload_payload_reset); FREELIST(files); } -- 1.7.7
Create a new static function called 'download_single_file' which iterates over the servers for each payload. Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- More code movement -- mostly just to improve readability and keep the function size down to something reasonable. lib/libalpm/sync.c | 46 +++++++++++++++++++++++++--------------------- 1 files changed, 25 insertions(+), 21 deletions(-) diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c index 3c304a3..d62950c 100644 --- a/lib/libalpm/sync.c +++ b/lib/libalpm/sync.c @@ -844,6 +844,30 @@ static int find_dl_candidates(alpm_db_t *repo, alpm_list_t **files, alpm_list_t return 0; } +static int download_single_file(alpm_handle_t *handle, struct dload_payload *payload, + const char *cachedir) +{ + const alpm_list_t *server; + + 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); + payload->handle = handle; + payload->allow_resume = 1; + + if(_alpm_download(payload, cachedir, NULL) != -1) { + return 0; + } + } + + return -1; +} + static int download_files(alpm_handle_t *handle, alpm_list_t **deltas) { const char *cachedir; @@ -899,27 +923,7 @@ static int download_files(alpm_handle_t *handle, alpm_list_t **deltas) 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; - - 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); - payload->handle = handle; - payload->allow_resume = 1; - - ret = _alpm_download(payload, cachedir, NULL); - if(ret != -1) { - break; - } - } - if(ret == -1) { + if(download_single_file(handle, i->data, cachedir) == -1) { errors++; _alpm_log(handle, ALPM_LOG_WARNING, _("failed to retrieve some files\n")); } -- 1.7.7
On Sun, Oct 16, 2011 at 1:59 PM, Dave Reisner <d@falconindy.com> wrote:
Create a new static function called 'download_single_file' which iterates over the servers for each payload.
Signed-off-by: Dave Reisner <dreisner@archlinux.org> Signed-off-by: Dan McGee <dan@archlinux.org> --- More code movement -- mostly just to improve readability and keep the function size down to something reasonable.
lib/libalpm/sync.c | 46 +++++++++++++++++++++++++--------------------- 1 files changed, 25 insertions(+), 21 deletions(-)
diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c index 3c304a3..d62950c 100644 --- a/lib/libalpm/sync.c +++ b/lib/libalpm/sync.c @@ -844,6 +844,30 @@ static int find_dl_candidates(alpm_db_t *repo, alpm_list_t **files, alpm_list_t return 0; }
+static int download_single_file(alpm_handle_t *handle, struct dload_payload *payload, + const char *cachedir) +{ + const alpm_list_t *server; + + 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); + payload->handle = handle; + payload->allow_resume = 1; + + if(_alpm_download(payload, cachedir, NULL) != -1) { + return 0; + } + } + + return -1; +} + static int download_files(alpm_handle_t *handle, alpm_list_t **deltas) { const char *cachedir; @@ -899,27 +923,7 @@ static int download_files(alpm_handle_t *handle, alpm_list_t **deltas)
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; - - 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); - payload->handle = handle; - payload->allow_resume = 1; - - ret = _alpm_download(payload, cachedir, NULL); - if(ret != -1) { - break; - } - } - if(ret == -1) { + if(download_single_file(handle, i->data, cachedir) == -1) { errors++; _alpm_log(handle, ALPM_LOG_WARNING, _("failed to retrieve some files\n")); } -- 1.7.7
participants (2)
-
Dan McGee
-
Dave Reisner