[pacman-dev] [PATCH] Normalize alpm download callback function's args to frontend
In response to my recent patches attempting to fix pacman's repetitive output with --noprogressbar, agregory kindly pointed out an obvious error I missed and suggested that I tackle the problem by making alpm's download callback function return normalized output[1]. [1]: https://wiki.archlinux.org/index.php/User:Apg#download_callback I deviated only slightly from his suggested spec. - When the total download size is unknown, return before calling the frontend's callback function. This is libalpm's original behavior. - Do not attempt to report download errors. I don't see any indication in the curl docs that it's possible for the progress function to be called after a download error. I'm happy to add either of those conditions, of course. Oh, and agregory was quite right: a side effect of this change is that it fixes the repeating-output bug that my previous, more complicated patches attempted to. As always, I welcome critiques. iff
From: Ivy Foster <ivy.foster@gmail.com> When curl calls alpm's dlcb, alpm calls the frontend's cb with the following (dlsize, totalsize) arguments: 0, -1: initialize 0, 0: no change since last call x {x>0, x<y}, y {y>0}: data downloaded, total size known x {x>0}, x: download finished If total size is not known, do not call frontend cb (no change to original behavior); alpm's callback shouldn't be called if there is a download error. See agregory's original spec here: https://wiki.archlinux.org/index.php/User:Apg#download_callback --- lib/libalpm/dload.c | 22 ++++++++++++++++------ src/pacman/callback.c | 19 +++++++++++-------- 2 files changed, 27 insertions(+), 14 deletions(-) diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index 31ae82c..f4e6a27 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -126,14 +126,24 @@ static int dload_progress_cb(void *file, double dltotal, double dlnow, } /* initialize the progress bar here to avoid displaying it when - * a repo is up to date and nothing gets downloaded */ - if(payload->prevprogress == 0) { - payload->handle->dlcb(payload->remote_name, 0, (off_t)dltotal); - } - + * a repo is up to date and nothing gets downloaded. + * payload->handle->dlcb will receive the remote_name + * and the following arguments: + * 0, -1: download initialized + * 0, 0: non-download event + * x {x>0}, x: download complete + * x {x>0, x<y}, y {y > 0}: download progress, expected total is known */ + if(current_size == total_size) { + payload->handle->dlcb(payload->remote_name, (off_t)dlnow, (off_t)dltotal); + } else if(!payload->prevprogress) { + payload->handle->dlcb(payload->remote_name, 0, -1); + } else if(payload->prevprogress == current_size) { + payload->handle->dlcb(payload->remote_name, 0, 0); + } else { /* do NOT include initial_size since it wasn't part of the package's * download_size (nor included in the total download size callback) */ - payload->handle->dlcb(payload->remote_name, (off_t)dlnow, (off_t)dltotal); + payload->handle->dlcb(payload->remote_name, (off_t)dlnow, (off_t)dltotal); + } payload->prevprogress = current_size; diff --git a/src/pacman/callback.c b/src/pacman/callback.c index ab3e14f..7f72b84 100644 --- a/src/pacman/callback.c +++ b/src/pacman/callback.c @@ -678,8 +678,13 @@ void cb_dl_progress(const char *filename, off_t file_xfered, off_t file_total) const unsigned short cols = getcols(); - if(config->noprogressbar || cols == 0 || file_total == -1) { - if(file_xfered == 0) { + /* Nothing has changed since last callback; stop here */ + if(file_xfered == 0 && file_total == 0) { + return; + } + + if(config->noprogressbar || cols == 0) { + if(file_xfered == 0 && file_total == -1) { printf(_("downloading %s...\n"), filename); fflush(stdout); } @@ -710,14 +715,9 @@ void cb_dl_progress(const char *filename, off_t file_xfered, off_t file_total) total = file_total; } - /* bogus values : stop here */ - if(xfered > total || xfered < 0) { - return; - } - /* this is basically a switch on xfered: 0, total, and * anything else */ - if(file_xfered == 0) { + if(file_xfered == 0 && file_total == -1) { /* set default starting values, ensure we only call this once * if TotalDownload is enabled */ if(!totaldownload || (totaldownload && list_xfered == 0)) { @@ -726,6 +726,9 @@ void cb_dl_progress(const char *filename, off_t file_xfered, off_t file_total) rate_last = 0.0; get_update_timediff(1); } + } else if(xfered > total || xfered < 0) { + /* bogus values : stop here */ + return; } else if(file_xfered == file_total) { /* compute final values */ int64_t timediff = get_time_ms() - initial_time; -- 2.9.0
On 09/07/16 13:11, ivy.foster@gmail.com wrote:
From: Ivy Foster <ivy.foster@gmail.com>
When curl calls alpm's dlcb, alpm calls the frontend's cb with the following (dlsize, totalsize) arguments:
0, -1: initialize 0, 0: no change since last call x {x>0, x<y}, y {y>0}: data downloaded, total size known x {x>0}, x: download finished
If total size is not known, do not call frontend cb (no change to original behavior); alpm's callback shouldn't be called if there is a download error.
See agregory's original spec here: https://wiki.archlinux.org/index.php/User:Apg#download_callback
Implementing the full idea from Andrew is probably still worthwhile. But this gets the main parts done and is an improvement so I am accepting. Thanks, Allan
participants (2)
-
Allan McRae
-
ivy.foster＠gmail.com