| Message ID | 20210121133707.111032-1-sava.jakovljev@teufel.de |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v5] channel_curl: Improve tracking of download progress | expand |
Hi Sava, On 21.01.21 14:37, Sava Jakovljev wrote: > * Compile xferinfo_legacy function only for version of libcurl > not supporting newer XFERINFOFUNCTION option. > * Use either XFERINFOFUNCTION or PROGRESSFUNCTION libcurl option > depending on libcurl version - use XFERINFOFUNCTION if possible. > * Introduce a structure containing information about file being > downloaded. > * Move setting progress tracking options into separate functions. > * Improve callback for tracking download progress - correctly track > percentage using valid variable and fix passing address of a > temporary variable as callback argument. > > Signed-off-by: Sava Jakovljev <sava.jakovljev@teufel.de> > --- > When a server doesn't insert Content-Length header, there is no sense in > enabling libcurl option XFERINFOFUNCTION/PROGRESSFUNCTION. > This change also solves a bug that currently exists, where an address of > a local variable 'percent' is passed to libcurl, which in turn gives it > as an argument to a callback specified with > XFERINFOFUNCTION/PROGRESSFUNCTION. > Also, this change will use XFERINFOFUNCTION if libcurl supports it, and > PROGRESSFUNCTION otherwise, and not both of them will be set, as the > situation is right now. Legacy xferinfo callback function is now > compiled only when libcurl does not support XFERINFOFUNCTION. > Secondly, download progress tracking is now enabled only for get_file > HTTP request and if server has correctly reported total download file > size. > > v2 of this patch deals with compiler warning and removes unused fields > of introduced download_callback_data_t structure. Also, code is cleaned > up. > > v3 of this patch deals with grammar errors in the git commit message, > with no functional changes to the patch itself. > > v4 of this patch addresses review comments: xferinfo callback has been > simplified, but download_callback_data_t structure has been kept, in > order to enable possible future extensions. > > v5 of this patch addresses issued found by Coverty tool. > Secondly, TRACE logs of download progress, in xferinfo function, are replaced > with DEBUG logs. > > Best regards, > Sava Jakovljev > > corelib/channel_curl.c | 124 +++++++++++++++++++++++++++++++---------- > 1 file changed, 95 insertions(+), 29 deletions(-) > > diff --git a/corelib/channel_curl.c b/corelib/channel_curl.c > index a3e785f..f734be3 100644 > --- a/corelib/channel_curl.c > +++ b/corelib/channel_curl.c > @@ -55,6 +55,11 @@ typedef struct { > output_data_t *outdata; > } write_callback_t; > > +typedef struct { > + curl_off_t total_download_size; > + uint8_t percent; > +} download_callback_data_t; > + > > /* Prototypes for "internal" functions */ > /* Note that they're not `static` so that they're callable from unit tests. */ > @@ -414,25 +419,31 @@ static int channel_callback_xferinfo(void *p, curl_off_t dltotal, curl_off_t dln > curl_off_t __attribute__((__unused__)) ultotal, > curl_off_t __attribute__((__unused__)) ulnow) > { > - if ((dltotal <= 0) || (dlnow > dltotal)) { > + if ((dltotal <= 0) || (dlnow > dltotal)) > return 0; > - } > - double percent = 100.0 * (dlnow/1024.0) / (dltotal/1024.0); > - double *last_percent = (double*)p; > - if ((int)*last_percent == (int)percent) { > + > + uint8_t percent = 100.0 * ((double)dlnow / dltotal); > + download_callback_data_t *data = (download_callback_data_t*)p; > + > + if (data->percent >= percent) > return 0; > - } > - *last_percent = percent; > + else > + data->percent = percent; > + > + DEBUG("Downloaded %d%% (%lu of %lu kB).", percent, dlnow / 1024, dltotal / 1024); > swupdate_download_update(percent, dltotal); > + > return 0; > } > > +#if LIBCURL_VERSION_NUM < 0x072000 > static int channel_callback_xferinfo_legacy(void *p, double dltotal, double dlnow, > double ultotal, double ulnow) > { > return channel_callback_xferinfo(p, (curl_off_t)dltotal, (curl_off_t)dlnow, > (curl_off_t)ultotal, (curl_off_t)ulnow); > } > +#endif > > static size_t channel_callback_headers(char *buffer, size_t size, size_t nitems, void *userdata) > { > @@ -566,28 +577,6 @@ channel_op_res_t channel_set_options(channel_t *this, channel_data_t *channel_da > goto cleanup; > } > > - double percent = -INFINITY; > - if ((curl_easy_setopt(channel_curl->handle, CURLOPT_PROGRESSFUNCTION, > - channel_callback_xferinfo_legacy) != CURLE_OK) || > - (curl_easy_setopt(channel_curl->handle, CURLOPT_PROGRESSDATA, > - &percent) != CURLE_OK)) { > - result = CHANNEL_EINIT; > - goto cleanup; > - } > -#if LIBCURL_VERSION_NUM >= 0x072000 > - if ((curl_easy_setopt(channel_curl->handle, CURLOPT_XFERINFOFUNCTION, > - channel_callback_xferinfo) != CURLE_OK) || > - (curl_easy_setopt(channel_curl->handle, CURLOPT_XFERINFODATA, > - &percent) != CURLE_OK)) { > - result = CHANNEL_EINIT; > - goto cleanup; > - } > -#endif > - if (curl_easy_setopt(channel_curl->handle, CURLOPT_NOPROGRESS, 0L) != CURLE_OK) { > - result = CHANNEL_EINIT; > - goto cleanup; > - } > - > if (channel_data->headers) { > /* > * Setup supply request and receive reply HTTP headers. > @@ -731,6 +720,73 @@ cleanup: > return result; > } > > +static curl_off_t channel_get_total_download_size(channel_curl_t *this, > + const char *url) > +{ > + assert(this != NULL); > + assert(url != NULL); > + > + curl_off_t size = -1; > + if (curl_easy_setopt(this->handle, CURLOPT_URL, url) != CURLE_OK || > + curl_easy_setopt(this->handle, CURLOPT_NOBODY, 1L) != CURLE_OK || > + curl_easy_perform(this->handle) != CURLE_OK) > + goto cleanup; > + > + if (curl_easy_getinfo(this->handle, CURLINFO_CONTENT_LENGTH_DOWNLOAD_T, > + &size) != CURLE_OK) > + goto cleanup; > + > +cleanup: > + if (curl_easy_setopt(this->handle, CURLOPT_NOBODY, 0L) != CURLE_OK) { > + ERROR("Failed to properly clean-up handle."); > + size = -1; > + } > + > + return size; > +} > + > +static channel_op_res_t channel_enable_download_progress_tracking( > + channel_curl_t *this, > + const char *url, > + download_callback_data_t *download_data) > +{ > + assert(url != NULL); > + assert(download_data != NULL); > + > + download_data->percent = 0; > + > + channel_op_res_t result = CHANNEL_OK; > + if ((download_data->total_download_size = channel_get_total_download_size( > + this, url)) <= 0) { > + result = CHANNEL_EINIT; > + goto cleanup; > + } > + > +#if LIBCURL_VERSION_NUM >= 0x072000 > + if ((curl_easy_setopt(this->handle, CURLOPT_XFERINFOFUNCTION, > + channel_callback_xferinfo) != CURLE_OK) || > + (curl_easy_setopt(this->handle, CURLOPT_XFERINFODATA, > + download_data) != CURLE_OK)) { > + result = CHANNEL_EINIT; > + goto cleanup; > + } > +#else > + if ((curl_easy_setopt(this->handle, CURLOPT_PROGRESSFUNCTION, > + channel_callback_xferinfo_legacy) != CURLE_OK) || > + (curl_easy_setopt(this->handle, CURLOPT_PROGRESSDATA, > + download_data) != CURLE_OK)) { > + result = CHANNEL_EINIT; > + goto cleanup; > + } > +#endif > + if (curl_easy_setopt(this->handle, CURLOPT_NOPROGRESS, 0L) != CURLE_OK) { > + result = CHANNEL_EINIT; > + goto cleanup; > + } > +cleanup: > + return result; > +} > + > static size_t put_read_callback(void *ptr, size_t size, size_t nmemb, void *data) > { > channel_data_t *channel_data = (channel_data_t *)data; > @@ -1030,6 +1086,16 @@ channel_op_res_t channel_get_file(channel_t *this, void *data) > goto cleanup_header; > } > > + download_callback_data_t download_data; > + if (channel_enable_download_progress_tracking(channel_curl, > + channel_data->url, > + &download_data) == CHANNEL_EINIT) { > + WARN("Failed to get total download size for URL %s.", > + channel_data->url); > + } else > + INFO("Total download size is %lu kB.", > + download_data.total_download_size / 1024); > + > if (curl_easy_setopt(channel_curl->handle, CURLOPT_CUSTOMREQUEST, "GET") != > CURLE_OK) { > ERROR("Set GET channel method option failed."); > Applied to -master, thanks ! Best regards, Stefano Babic
diff --git a/corelib/channel_curl.c b/corelib/channel_curl.c index a3e785f..f734be3 100644 --- a/corelib/channel_curl.c +++ b/corelib/channel_curl.c @@ -55,6 +55,11 @@ typedef struct { output_data_t *outdata; } write_callback_t; +typedef struct { + curl_off_t total_download_size; + uint8_t percent; +} download_callback_data_t; + /* Prototypes for "internal" functions */ /* Note that they're not `static` so that they're callable from unit tests. */ @@ -414,25 +419,31 @@ static int channel_callback_xferinfo(void *p, curl_off_t dltotal, curl_off_t dln curl_off_t __attribute__((__unused__)) ultotal, curl_off_t __attribute__((__unused__)) ulnow) { - if ((dltotal <= 0) || (dlnow > dltotal)) { + if ((dltotal <= 0) || (dlnow > dltotal)) return 0; - } - double percent = 100.0 * (dlnow/1024.0) / (dltotal/1024.0); - double *last_percent = (double*)p; - if ((int)*last_percent == (int)percent) { + + uint8_t percent = 100.0 * ((double)dlnow / dltotal); + download_callback_data_t *data = (download_callback_data_t*)p; + + if (data->percent >= percent) return 0; - } - *last_percent = percent; + else + data->percent = percent; + + DEBUG("Downloaded %d%% (%lu of %lu kB).", percent, dlnow / 1024, dltotal / 1024); swupdate_download_update(percent, dltotal); + return 0; } +#if LIBCURL_VERSION_NUM < 0x072000 static int channel_callback_xferinfo_legacy(void *p, double dltotal, double dlnow, double ultotal, double ulnow) { return channel_callback_xferinfo(p, (curl_off_t)dltotal, (curl_off_t)dlnow, (curl_off_t)ultotal, (curl_off_t)ulnow); } +#endif static size_t channel_callback_headers(char *buffer, size_t size, size_t nitems, void *userdata) { @@ -566,28 +577,6 @@ channel_op_res_t channel_set_options(channel_t *this, channel_data_t *channel_da goto cleanup; } - double percent = -INFINITY; - if ((curl_easy_setopt(channel_curl->handle, CURLOPT_PROGRESSFUNCTION, - channel_callback_xferinfo_legacy) != CURLE_OK) || - (curl_easy_setopt(channel_curl->handle, CURLOPT_PROGRESSDATA, - &percent) != CURLE_OK)) { - result = CHANNEL_EINIT; - goto cleanup; - } -#if LIBCURL_VERSION_NUM >= 0x072000 - if ((curl_easy_setopt(channel_curl->handle, CURLOPT_XFERINFOFUNCTION, - channel_callback_xferinfo) != CURLE_OK) || - (curl_easy_setopt(channel_curl->handle, CURLOPT_XFERINFODATA, - &percent) != CURLE_OK)) { - result = CHANNEL_EINIT; - goto cleanup; - } -#endif - if (curl_easy_setopt(channel_curl->handle, CURLOPT_NOPROGRESS, 0L) != CURLE_OK) { - result = CHANNEL_EINIT; - goto cleanup; - } - if (channel_data->headers) { /* * Setup supply request and receive reply HTTP headers. @@ -731,6 +720,73 @@ cleanup: return result; } +static curl_off_t channel_get_total_download_size(channel_curl_t *this, + const char *url) +{ + assert(this != NULL); + assert(url != NULL); + + curl_off_t size = -1; + if (curl_easy_setopt(this->handle, CURLOPT_URL, url) != CURLE_OK || + curl_easy_setopt(this->handle, CURLOPT_NOBODY, 1L) != CURLE_OK || + curl_easy_perform(this->handle) != CURLE_OK) + goto cleanup; + + if (curl_easy_getinfo(this->handle, CURLINFO_CONTENT_LENGTH_DOWNLOAD_T, + &size) != CURLE_OK) + goto cleanup; + +cleanup: + if (curl_easy_setopt(this->handle, CURLOPT_NOBODY, 0L) != CURLE_OK) { + ERROR("Failed to properly clean-up handle."); + size = -1; + } + + return size; +} + +static channel_op_res_t channel_enable_download_progress_tracking( + channel_curl_t *this, + const char *url, + download_callback_data_t *download_data) +{ + assert(url != NULL); + assert(download_data != NULL); + + download_data->percent = 0; + + channel_op_res_t result = CHANNEL_OK; + if ((download_data->total_download_size = channel_get_total_download_size( + this, url)) <= 0) { + result = CHANNEL_EINIT; + goto cleanup; + } + +#if LIBCURL_VERSION_NUM >= 0x072000 + if ((curl_easy_setopt(this->handle, CURLOPT_XFERINFOFUNCTION, + channel_callback_xferinfo) != CURLE_OK) || + (curl_easy_setopt(this->handle, CURLOPT_XFERINFODATA, + download_data) != CURLE_OK)) { + result = CHANNEL_EINIT; + goto cleanup; + } +#else + if ((curl_easy_setopt(this->handle, CURLOPT_PROGRESSFUNCTION, + channel_callback_xferinfo_legacy) != CURLE_OK) || + (curl_easy_setopt(this->handle, CURLOPT_PROGRESSDATA, + download_data) != CURLE_OK)) { + result = CHANNEL_EINIT; + goto cleanup; + } +#endif + if (curl_easy_setopt(this->handle, CURLOPT_NOPROGRESS, 0L) != CURLE_OK) { + result = CHANNEL_EINIT; + goto cleanup; + } +cleanup: + return result; +} + static size_t put_read_callback(void *ptr, size_t size, size_t nmemb, void *data) { channel_data_t *channel_data = (channel_data_t *)data; @@ -1030,6 +1086,16 @@ channel_op_res_t channel_get_file(channel_t *this, void *data) goto cleanup_header; } + download_callback_data_t download_data; + if (channel_enable_download_progress_tracking(channel_curl, + channel_data->url, + &download_data) == CHANNEL_EINIT) { + WARN("Failed to get total download size for URL %s.", + channel_data->url); + } else + INFO("Total download size is %lu kB.", + download_data.total_download_size / 1024); + if (curl_easy_setopt(channel_curl->handle, CURLOPT_CUSTOMREQUEST, "GET") != CURLE_OK) { ERROR("Set GET channel method option failed.");
* Compile xferinfo_legacy function only for version of libcurl not supporting newer XFERINFOFUNCTION option. * Use either XFERINFOFUNCTION or PROGRESSFUNCTION libcurl option depending on libcurl version - use XFERINFOFUNCTION if possible. * Introduce a structure containing information about file being downloaded. * Move setting progress tracking options into separate functions. * Improve callback for tracking download progress - correctly track percentage using valid variable and fix passing address of a temporary variable as callback argument. Signed-off-by: Sava Jakovljev <sava.jakovljev@teufel.de> --- When a server doesn't insert Content-Length header, there is no sense in enabling libcurl option XFERINFOFUNCTION/PROGRESSFUNCTION. This change also solves a bug that currently exists, where an address of a local variable 'percent' is passed to libcurl, which in turn gives it as an argument to a callback specified with XFERINFOFUNCTION/PROGRESSFUNCTION. Also, this change will use XFERINFOFUNCTION if libcurl supports it, and PROGRESSFUNCTION otherwise, and not both of them will be set, as the situation is right now. Legacy xferinfo callback function is now compiled only when libcurl does not support XFERINFOFUNCTION. Secondly, download progress tracking is now enabled only for get_file HTTP request and if server has correctly reported total download file size. v2 of this patch deals with compiler warning and removes unused fields of introduced download_callback_data_t structure. Also, code is cleaned up. v3 of this patch deals with grammar errors in the git commit message, with no functional changes to the patch itself. v4 of this patch addresses review comments: xferinfo callback has been simplified, but download_callback_data_t structure has been kept, in order to enable possible future extensions. v5 of this patch addresses issued found by Coverty tool. Secondly, TRACE logs of download progress, in xferinfo function, are replaced with DEBUG logs. Best regards, Sava Jakovljev corelib/channel_curl.c | 124 +++++++++++++++++++++++++++++++---------- 1 file changed, 95 insertions(+), 29 deletions(-)