Re: [pacman-dev] [PATCH v4] Add configuration options for libcurl's "low speed" timeout

27 Mar 2014

On Wed, Mar 26, 2014 at 07:48:57PM -0400, Andrew Hills wrote:
...
Add LowSpeedLimit and LowSpeedTime configuration options to correspond to
libcurl's CURLOPT_LOW_SPEED_LIMIT and CURLOPT_LOW_SPEED_TIME options. This
allows, e.g., transfers behind corporate virus-scanning firewalls to survive
the delays. Increasing the timeout may not be desirable in all situations;
similarly, disabling the check prevents detection of disappearing networks.
Signed-off-by: Andrew Hills <ahills@ednos.net>
---
This version of the patch does not allow setting LowSpeedLimit, nor
LowSpeedTime, to a negative value.
doc/pacman.conf.5.txt | 13 +++++++++++++
 etc/pacman.conf.in    |  2 ++
 lib/libalpm/alpm.h    | 10 ++++++++++
 lib/libalpm/dload.c   |  4 ++--
 lib/libalpm/handle.c  | 34 ++++++++++++++++++++++++++++++++++
 lib/libalpm/handle.h  |  4 ++++
 src/pacman/conf.c     | 24 ++++++++++++++++++++++++
 src/pacman/conf.h     |  4 ++++
 8 files changed, 93 insertions(+), 2 deletions(-)

diff --git a/doc/pacman.conf.5.txt b/doc/pacman.conf.5.txt
index f19311c..bfc0191 100644
--- a/doc/pacman.conf.5.txt
+++ b/doc/pacman.conf.5.txt
@@ -199,6 +199,19 @@ Options
  Displays name, version and size of target packages formatted
  as a table for upgrade, sync and remove operations.
+*LowSpeedLimit* = speed::
+	Sets the speed in bytes per second that a download should be below during
+	`LowSpeedTime` seconds to abort the transfer for being too slow. Setting
+	'speed' to 0 will disable the speed check. Defaults to 1 byte per second.
+	Note that this option will not affect external programs specified by
+	`XferCommand`.
+
+*LowSpeedTime* = time::
+	Sets the time in seconds that a download should be below the `LowSpeedLimit`
+	transfer speed to abort the transfer for being too slow. Setting 'time' to
+	0 will disable the speed check. Defaults to 10 seconds. Note that this
+	option will not affect external programs specified by `XferCommand`.
+
 Repository Sections
 -------------------
 Each repository section defines a section name and at least one location where
diff --git a/etc/pacman.conf.in b/etc/pacman.conf.in
index b891de1..0bc5908 100644
--- a/etc/pacman.conf.in
+++ b/etc/pacman.conf.in
@@ -20,6 +20,8 @@ HoldPkg     = pacman glibc
 #CleanMethod = KeepInstalled
 #UseDelta    = 0.7
 Architecture = auto
+#LowSpeedLimit = 1
+#LowSpeedTime = 10
# Pacman won't upgrade packages listed in IgnorePkg and members of IgnoreGroup
 #IgnorePkg   =
diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h
index b0adb95..d23ae23 100644
--- a/lib/libalpm/alpm.h
+++ b/lib/libalpm/alpm.h
@@ -595,6 +595,16 @@ const char *alpm_option_get_dbpath(alpm_handle_t *handle);
 /** Get the name of the database lock file. Read-only. */
 const char *alpm_option_get_lockfile(alpm_handle_t *handle);
+/** Returns the libcurl low speed limit in bytes per second. */
+long alpm_option_get_lowspeedlimit(alpm_handle_t *handle);
+/** Sets the libcurl low speed limit in bytes per second. */
+int alpm_option_set_lowspeedlimit(alpm_handle_t *handle, long lowspeedlimit);
+
+/** Returns the libcurl low speed time in seconds. */
+long alpm_option_get_lowspeedtime(alpm_handle_t *handle);
+/** Sets the libcurl low speed time in seconds. */
+int alpm_option_set_lowspeedtime(alpm_handle_t *handle, long lowspeedtime);
+
 /** @name Accessors to the list of package cache directories.
  * @{
  */
diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c
index 53867f5..31edbf4 100644
--- a/lib/libalpm/dload.c
+++ b/lib/libalpm/dload.c
@@ -294,8 +294,8 @@ static void curl_set_handle_opts(struct dload_payload *payload,
  curl_easy_setopt(curl, CURLOPT_FOLLOWLOCATION, 1L);
  curl_easy_setopt(curl, CURLOPT_PROGRESSFUNCTION, dload_progress_cb);
  curl_easy_setopt(curl, CURLOPT_PROGRESSDATA, (void *)payload);
-	curl_easy_setopt(curl, CURLOPT_LOW_SPEED_LIMIT, 1L);
-	curl_easy_setopt(curl, CURLOPT_LOW_SPEED_TIME, 10L);
+	curl_easy_setopt(curl, CURLOPT_LOW_SPEED_LIMIT, handle->lowspeedlimit);
+	curl_easy_setopt(curl, CURLOPT_LOW_SPEED_TIME, handle->lowspeedtime);
  curl_easy_setopt(curl, CURLOPT_HEADERFUNCTION, dload_parseheader_cb);
  curl_easy_setopt(curl, CURLOPT_WRITEHEADER, (void *)payload);
  curl_easy_setopt(curl, CURLOPT_NETRC, CURL_NETRC_OPTIONAL);
diff --git a/lib/libalpm/handle.c b/lib/libalpm/handle.c
index 0842d51..7eb0f8f 100644
--- a/lib/libalpm/handle.c
+++ b/lib/libalpm/handle.c
@@ -198,6 +198,18 @@ const char SYMEXPORT *alpm_option_get_lockfile(alpm_handle_t *handle)
  return handle->lockfile;
 }
+long SYMEXPORT alpm_option_get_lowspeedlimit(alpm_handle_t *handle)
+{
+	CHECK_HANDLE(handle, return -1);
+	return handle->lowspeedlimit;
+}
+
+long SYMEXPORT alpm_option_get_lowspeedtime(alpm_handle_t *handle)
+{
+	CHECK_HANDLE(handle, return -1);
+	return handle->lowspeedtime;
+}
+
 const char SYMEXPORT *alpm_option_get_gpgdir(alpm_handle_t *handle)
 {
  CHECK_HANDLE(handle, return NULL);
@@ -294,6 +306,28 @@ int SYMEXPORT alpm_option_set_progresscb(alpm_handle_t *handle, alpm_cb_progress
  return 0;
 }
+int SYMEXPORT alpm_option_set_lowspeedlimit(alpm_handle_t *handle, long lowspeedlimit)
+{
+	CHECK_HANDLE(handle, return -1);
+	if(lowspeedlimit < 0)
+	{
Our style guide actually says that this opening brace should be on the
same line as the 'if'. We only declare that opening braces for functions
are on the following line.
...
+		RET_ERR(handle, ALPM_ERR_WRONG_ARGS, -1);
+	}
+	handle->lowspeedlimit = lowspeedlimit;
+	return 0;
+}
+
+int SYMEXPORT alpm_option_set_lowspeedtime(alpm_handle_t *handle, long lowspeedtime)
+{
+	CHECK_HANDLE(handle, return -1);
+	if(lowspeedtime < 0)
+	{
+		RET_ERR(handle, ALPM_ERR_WRONG_ARGS, -1);
Ditto.
...
+	}
+	handle->lowspeedtime = lowspeedtime;
+	return 0;
+}
+
 static char *canonicalize_path(const char *path)
 {
  char *new_path;
diff --git a/lib/libalpm/handle.h b/lib/libalpm/handle.h
index 27241ea..25a51ff 100644
--- a/lib/libalpm/handle.h
+++ b/lib/libalpm/handle.h
@@ -104,6 +104,10 @@ struct __alpm_handle_t {
  /* for delta parsing efficiency */
  int delta_regex_compiled;
  regex_t delta_regex;
+
+	/* Curl timeouts */
+	long lowspeedlimit;
+	long lowspeedtime;
So, until now, I've refused any patches which add configuration options
for the downloader. Consider this to be me giving in.

I sort of wonder how much longer it'll be until we start getting patches
for other downloader knobs. To that end, I'd really like it if we could
namespace this in the handle by creating a sub-struct within the handle for
downloader-specific things (this would also make it easier to replace in
the future). This would also allow us to easily contain the ifdef'ery
needed to shrink down the handle struct when there's no libcurl support
in alpm.
...
};
alpm_handle_t *_alpm_handle_new(void);
diff --git a/src/pacman/conf.c b/src/pacman/conf.c
index f75f3a7..0506329 100644
--- a/src/pacman/conf.c
+++ b/src/pacman/conf.c
@@ -120,6 +120,9 @@ config_t *config_new(void)
  newconfig->colstr.err     = "";
  newconfig->colstr.nocolor = "";
+	newconfig->lowspeedlimit = 1L;
+	newconfig->lowspeedtime = 10L;
+
  return newconfig;
 }
@@ -596,6 +599,24 @@ static int _parse_options(const char *key, char *value,
      		return 1;
      	}
      	FREELIST(values);
+		} else if(strcmp(key, "LowSpeedLimit") == 0) {
+			config->lowspeedlimit = strtol(value, NULL, 0);
+			if((config->lowspeedlimit == LONG_MAX || config->lowspeedlimit == LONG_MIN) && errno == ERANGE) {
+				pm_printf(ALPM_LOG_WARNING,
+						_("config file %s, line %d: LowSpeedLimit overflow, using default.\n"),
+						file, linenum);
+				config->lowspeedlimit = 1L;
+			}
+			pm_printf(ALPM_LOG_DEBUG, "config: lowspeedlimit: %ld\n", config->lowspeedlimit);
+		} else if(strcmp(key, "LowSpeedTime") == 0) {
+			config->lowspeedtime = strtol(value, NULL, 0);
This will do things like treat "10m" as 10 seconds and not provide any
user feedback. I think you want to separate this logic out into it's own
function and do the full error handling. You can reuse this for both
LowSpeedLimit and LowSpeedTime.
...
+			if((config->lowspeedtime == LONG_MAX || config->lowspeedtime == LONG_MIN) && errno == ERANGE) {
+				pm_printf(ALPM_LOG_WARNING,
+						_("config file %s, line %d: LowSpeedTime overflow, using default.\n"),
We'll have one less string to translate here if you use %s in place of
the key name and instead pass it as an argument.
...
+						file, linenum);
+				config->lowspeedtime = 10L;
+			}
+			pm_printf(ALPM_LOG_DEBUG, "config: lowspeedtime: %ld\n", config->lowspeedtime);
      } else {
      	pm_printf(ALPM_LOG_WARNING,
      			_("config file %s, line %d: directive '%s' in section '%s' not recognized.\n"),
@@ -690,6 +711,9 @@ static int setup_libalpm(void)
  alpm_option_set_questioncb(handle, cb_question);
  alpm_option_set_progresscb(handle, cb_progress);
+	alpm_option_set_lowspeedlimit(handle, config->lowspeedlimit);
+	alpm_option_set_lowspeedtime(handle, config->lowspeedtime);
+
  config->logfile = config->logfile ? config->logfile : strdup(LOGFILE);
  ret = alpm_option_set_logfile(handle, config->logfile);
  if(ret != 0) {
diff --git a/src/pacman/conf.h b/src/pacman/conf.h
index e8cac50..3f2cdb3 100644
--- a/src/pacman/conf.h
+++ b/src/pacman/conf.h
@@ -113,6 +113,10 @@ typedef struct __config_t {
/* Color strings for output */
  colstr_t colstr;
+
+	/* Curl timeouts */
+	long lowspeedlimit;
+	long lowspeedtime;
 } config_t;
/* Operations */
-- 
1.9.0

    

Re: [pacman-dev] [PATCH v4] Add configuration options for libcurl's "low speed" timeout

Dave Reisner