diff mbox series

[v2] channel_curl: Improve handling of tracking download progress

Message ID 20201214115927.41039-1-sava.jakovljev@teufel.de
State Changes Requested
Headers show
Series [v2] channel_curl: Improve handling of tracking download progress | expand

Commit Message

Sava Jakovljev Dec. 14, 2020, 11:59 a.m. UTC
* Compile xferinfo_legay function only for version of libcurl
  not supporting newer xferinfo 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 ptach deals with compiler warnings and removes unused fields
of introduced download_callback_data_t structure. Also, code is cleaned
up.

Best regards,
Sava Jakovljev

 corelib/channel_curl.c | 122 +++++++++++++++++++++++++++++++----------
 1 file changed, 93 insertions(+), 29 deletions(-)

--
2.26.2
diff mbox series

Patch

diff --git a/corelib/channel_curl.c b/corelib/channel_curl.c
index a3e785f..843da2a 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,33 @@  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)) {
+	download_callback_data_t *data = (download_callback_data_t*)p;
+
+	if (data->total_download_size != dltotal)
 		return 0;
-	}
-	double percent = 100.0 * (dlnow/1024.0) / (dltotal/1024.0);
-	double *last_percent = (double*)p;
-	if ((int)*last_percent == (int)percent) {
+	if ((dltotal <= 0) || (dlnow > dltotal))
 		return 0;
-	}
-	*last_percent = percent;
+
+	uint8_t percent = 100.0 * ((double)dlnow / dltotal) + 0.5;
+
+	if (data->percent == percent)
+		return 0;
+	else
+		data->percent = percent;
+
+	TRACE("Download %d%% (%lu of %lu kB).", percent, dlnow, dltotal);
 	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 +579,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 +722,69 @@  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:
+	curl_easy_setopt(this->handle, CURLOPT_NOBODY, 0L);
+	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 +1084,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.");