[pacman-dev] [PATCH] Add configuration options for libcurl's "low speed" timeout
Hi all, (Resending; looks like it didn't make it through the first time.) Behind an obnoxious "transparent" corporate virus-scanning firewall, I find pacman frequently failing to download packages, yielding an "Operation too slow" error. Setting the transfer command to a more forgiving program like wget is suboptimal, as it's often unclear what failed and why: most mirrors are missing the signatures for the package databases, for example, littering the terminal with irrelevant errors. This patch adds the "LowSpeedLimit" and "LowSpeedTime" configuration parameters to pacman.conf rather than hard-coding the defaults in libalpm. It adds two fields to the configuration structure and the ALPM handle structure to get the data all the way into curl_set_handle_opts(). Unfortunately, I had to include ALPM's handle.h in pacman's conf.h, else setting the handle's new fields fails (the type is incomplete at that point). I couldn't see a better way to do it. Comments requested. Thanks, Andrew From 85b5392be37fa6b8cc4392f6b308a8141255ab8f Mon Sep 17 00:00:00 2001 From: Andrew Hills <ahills@ednos.net> Date: Wed, 19 Mar 2014 14:03:30 -0400 Subject: [PATCH] Add LowSpeedLimit and LowSpeedTime configuration options to correspond to libcurl's CURLOPT_LOW_SPEED_LIMIT and CURLOPT_LOW_SPEED_TIME options. Signed-off-by: Andrew Hills <ahills@ednos.net> --- lib/libalpm/dload.c | 4 ++-- lib/libalpm/handle.h | 4 ++++ src/pacman/conf.c | 24 ++++++++++++++++++++++++ src/pacman/conf.h | 5 +++++ 4 files changed, 35 insertions(+), 2 deletions(-) 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.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; }; alpm_handle_t *_alpm_handle_new(void); diff --git a/src/pacman/conf.c b/src/pacman/conf.c index f75f3a7..3e5041f 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: %lu\n", config->lowspeedlimit); + } else if(strcmp(key, "LowSpeedTime") == 0) { + config->lowspeedtime = strtol(value, NULL, 0); + 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"), + file, linenum); + config->lowspeedtime = 10L; + } + pm_printf(ALPM_LOG_DEBUG, "config: lowspeedtime: %lu\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); + handle->lowspeedlimit = config->lowspeedlimit; + handle->lowspeedtime = 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..6ab8b52 100644 --- a/src/pacman/conf.h +++ b/src/pacman/conf.h @@ -21,6 +21,7 @@ #define _PM_CONF_H #include <alpm.h> +#include <handle.h> typedef struct __colstr_t { const char *colon; @@ -113,6 +114,10 @@ typedef struct __config_t { /* Color strings for output */ colstr_t colstr; + + /* Curl timeouts */ + long lowspeedlimit; + long lowspeedtime; } config_t; /* Operations */ -- 1.8.5.5
On 26/03/14 09:01, Andrew Hills wrote:
Hi all,
(Resending; looks like it didn't make it through the first time.)
Behind an obnoxious "transparent" corporate virus-scanning firewall, I find pacman frequently failing to download packages, yielding an "Operation too slow" error. Setting the transfer command to a more forgiving program like wget is suboptimal, as it's often unclear what failed and why: most mirrors are missing the signatures for the package databases, for example, littering the terminal with irrelevant errors.
This patch adds the "LowSpeedLimit" and "LowSpeedTime" configuration parameters to pacman.conf rather than hard-coding the defaults in libalpm. It adds two fields to the configuration structure and the ALPM handle structure to get the data all the way into curl_set_handle_opts().
Unfortunately, I had to include ALPM's handle.h in pacman's conf.h, else setting the handle's new fields fails (the type is incomplete at that point). I couldn't see a better way to do it. Comments requested.
Did this even compile? The include should fail given libalpm is not in the pacman source inlcude path. Anyway, as with all the other options you will need to add a alpm_option_set/get_...() function. This gives all frontends access.
Thanks, Andrew
From 85b5392be37fa6b8cc4392f6b308a8141255ab8f Mon Sep 17 00:00:00 2001 From: Andrew Hills <ahills@ednos.net> Date: Wed, 19 Mar 2014 14:03:30 -0400 Subject: [PATCH] Add LowSpeedLimit and LowSpeedTime configuration options to correspond to libcurl's CURLOPT_LOW_SPEED_LIMIT and CURLOPT_LOW_SPEED_TIME options.
Signed-off-by: Andrew Hills <ahills@ednos.net> --- lib/libalpm/dload.c | 4 ++-- lib/libalpm/handle.h | 4 ++++ src/pacman/conf.c | 24 ++++++++++++++++++++++++ src/pacman/conf.h | 5 +++++
No documentation updates to be seen. pacman.conf man page and example pacman.conf will need changed. Small comments below. @Dave: downloading is your thing. Any other comments?
4 files changed, 35 insertions(+), 2 deletions(-)
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.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; };
alpm_handle_t *_alpm_handle_new(void); diff --git a/src/pacman/conf.c b/src/pacman/conf.c index f75f3a7..3e5041f 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) {
Indenting is always tabs in pacman. This varies throughout your patch...
+ 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: %lu\n", config->lowspeedlimit);
How does curl handle negative values for the limits? Perhaps we should ensure this is positive too. (aside: do we check the range for the delta config value?)
+ } else if(strcmp(key, "LowSpeedTime") == 0) { + config->lowspeedtime = strtol(value, NULL, 0); + 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"), + file, linenum); + config->lowspeedtime = 10L; + } + pm_printf(ALPM_LOG_DEBUG, "config: lowspeedtime: %lu\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);
+ handle->lowspeedlimit = config->lowspeedlimit; + handle->lowspeedtime = 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..6ab8b52 100644 --- a/src/pacman/conf.h +++ b/src/pacman/conf.h @@ -21,6 +21,7 @@ #define _PM_CONF_H
#include <alpm.h> +#include <handle.h>
typedef struct __colstr_t { const char *colon; @@ -113,6 +114,10 @@ typedef struct __config_t {
/* Color strings for output */ colstr_t colstr; + + /* Curl timeouts */ + long lowspeedlimit; + long lowspeedtime; } config_t;
/* Operations */ -- 1.8.5.5
Thanks for the quick response. On 3/25/14, 9:19 PM, Allan McRae wrote:
Did this even compile? The include should fail given libalpm is not in the pacman source inlcude path.
It does indeed compile, I'm using it! :) The header I include (handle.h) is in the same directory as the header included immediately above it (alpm.h). I built it with makepkg; are there other environments in which you expect this to fail? It seems to me that libalpm is always distributed with pacman.
Anyway, as with all the other options you will need to add a alpm_option_set/get_...() function. This gives all frontends access.
No documentation updates to be seen. pacman.conf man page and example pacman.conf will need changed.
Indenting is always tabs in pacman. This varies throughout your patch...
I will send a patch shortly addressing these mistakes. Does libalpm have tests that I should also be updating?
How does curl handle negative values for the limits? Perhaps we should ensure this is positive too.
It's a simple less-than comparison; see curl's lib/speedcheck.c:39-57 for details. Setting a negative low speed time will always trigger the low speed failure, e.g.: Operation too slow. Less than 1 bytes/sec transferred the last -30 seconds Setting a negative (or zero) low speed limit will effectively disable the check, regardless of the low speed time. It may be more elegant to provide an option to disable the check, but I usually err on the side of flexibility. Thanks, Andrew
Please find below a revised patch, now including alpm_options_set/get functions, documentation updates, and repaired indentation. I tested this the same way as the previous patch: by building it with makepkg and installing it on my system and using it. This means I haven't tested the alpm_options_set/get functions. I wasn't sure what to return for an invalid handle in alpm_option_get, but -1 looked to be the standard elsewhere in the file for this type. Thanks, Andrew From 26ac679398d206338d625774ece69132fd369359 Mon Sep 17 00:00:00 2001 From: Andrew Hills <ahills@ednos.net> Date: Wed, 19 Mar 2014 14:03:30 -0400 Subject: [PATCH] Add LowSpeedLimit and LowSpeedTime configuration options to correspond to libcurl's CURLOPT_LOW_SPEED_LIMIT and CURLOPT_LOW_SPEED_TIME options. Signed-off-by: Andrew Hills <ahills@ednos.net> --- doc/pacman.conf.5.txt | 14 ++++++++++++++ etc/pacman.conf.in | 2 ++ lib/libalpm/alpm.h | 10 ++++++++++ lib/libalpm/dload.c | 4 ++-- lib/libalpm/handle.c | 26 ++++++++++++++++++++++++++ lib/libalpm/handle.h | 4 ++++ src/pacman/conf.c | 24 ++++++++++++++++++++++++ src/pacman/conf.h | 5 +++++ 8 files changed, 87 insertions(+), 2 deletions(-) diff --git a/doc/pacman.conf.5.txt b/doc/pacman.conf.5.txt index f19311c..3e70573 100644 --- a/doc/pacman.conf.5.txt +++ b/doc/pacman.conf.5.txt @@ -199,6 +199,20 @@ 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 or a negative number 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. Setting 'time' to a negative number will + cause all downloads to fail. 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..e8cea12 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. */ +long alpm_option_get_lowspeedlimit(alpm_handle_t *handle); +/** Sets the libcurl low speed limit. */ +int alpm_option_set_lowspeedlimit(alpm_handle_t *handle, long lowspeedlimit); + +/** Returns the libcurl low speed time. */ +long alpm_option_get_lowspeedtime(alpm_handle_t *handle); +/** Sets the libcurl low speed time. */ +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..d0cd01c 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,20 @@ 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); + handle->lowspeedlimit = lowspeedlimit; + return 0; +} + +int SYMEXPORT alpm_option_set_lowspeedtime(alpm_handle_t *handle, long lowspeedtime) +{ + CHECK_HANDLE(handle, return -1); + 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; }; alpm_handle_t *_alpm_handle_new(void); diff --git a/src/pacman/conf.c b/src/pacman/conf.c index f75f3a7..7916a1d 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); + 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"), + file, linenum); + config->lowspeedtime = 10L; + } + pm_printf(ALPM_LOG_DEBUG, "config: lowspeedtime: %lu\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); + handle->lowspeedlimit = config->lowspeedlimit; + handle->lowspeedtime = 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..6ab8b52 100644 --- a/src/pacman/conf.h +++ b/src/pacman/conf.h @@ -21,6 +21,7 @@ #define _PM_CONF_H #include <alpm.h> +#include <handle.h> typedef struct __colstr_t { const char *colon; @@ -113,6 +114,10 @@ typedef struct __config_t { /* Color strings for output */ colstr_t colstr; + + /* Curl timeouts */ + long lowspeedlimit; + long lowspeedtime; } config_t; /* Operations */ -- 1.9.0
On Wed, Mar 26, 2014 at 01:53:55PM -0400, Andrew Hills wrote:
Please find below a revised patch, now including alpm_options_set/get functions, documentation updates, and repaired indentation. I tested this the same way as the previous patch: by building it with makepkg and installing it on my system and using it. This means I haven't tested the alpm_options_set/get functions. I wasn't sure what to return for an invalid handle in alpm_option_get, but -1 looked to be the standard elsewhere in the file for this type.
What do you plan to set these values to in your environment? Why shouldn't we just adjust these to be more lax?
Thanks, Andrew
From 26ac679398d206338d625774ece69132fd369359 Mon Sep 17 00:00:00 2001 From: Andrew Hills <ahills@ednos.net> Date: Wed, 19 Mar 2014 14:03:30 -0400 Subject: [PATCH] Add LowSpeedLimit and LowSpeedTime configuration options to correspond to libcurl's CURLOPT_LOW_SPEED_LIMIT and CURLOPT_LOW_SPEED_TIME options.
Signed-off-by: Andrew Hills <ahills@ednos.net> --- doc/pacman.conf.5.txt | 14 ++++++++++++++ etc/pacman.conf.in | 2 ++ lib/libalpm/alpm.h | 10 ++++++++++ lib/libalpm/dload.c | 4 ++-- lib/libalpm/handle.c | 26 ++++++++++++++++++++++++++ lib/libalpm/handle.h | 4 ++++ src/pacman/conf.c | 24 ++++++++++++++++++++++++ src/pacman/conf.h | 5 +++++ 8 files changed, 87 insertions(+), 2 deletions(-)
diff --git a/doc/pacman.conf.5.txt b/doc/pacman.conf.5.txt index f19311c..3e70573 100644 --- a/doc/pacman.conf.5.txt +++ b/doc/pacman.conf.5.txt @@ -199,6 +199,20 @@ 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 or a negative number 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. Setting 'time' to a negative number will + cause all downloads to fail. 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..e8cea12 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. */ +long alpm_option_get_lowspeedlimit(alpm_handle_t *handle); +/** Sets the libcurl low speed limit. */ +int alpm_option_set_lowspeedlimit(alpm_handle_t *handle, long lowspeedlimit);
What is this measured in ? bytes? megabytes? mebibytes?
+ +/** Returns the libcurl low speed time. */ +long alpm_option_get_lowspeedtime(alpm_handle_t *handle); +/** Sets the libcurl low speed time. */ +int alpm_option_set_lowspeedtime(alpm_handle_t *handle, long lowspeedtime); +
seconds? milliseconds? hours?
/** @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..d0cd01c 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,20 @@ 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); + handle->lowspeedlimit = lowspeedlimit; + return 0; +} + +int SYMEXPORT alpm_option_set_lowspeedtime(alpm_handle_t *handle, long lowspeedtime) +{ + CHECK_HANDLE(handle, return -1); + 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; };
alpm_handle_t *_alpm_handle_new(void); diff --git a/src/pacman/conf.c b/src/pacman/conf.c index f75f3a7..7916a1d 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); + 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"), + file, linenum); + config->lowspeedtime = 10L; + } + pm_printf(ALPM_LOG_DEBUG, "config: lowspeedtime: %lu\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);
+ handle->lowspeedlimit = config->lowspeedlimit; + handle->lowspeedtime = config->lowspeedtime; +
Not sure how this compiles -- the internals of the handle aren't visible from the frontend code. You need to use the setter methods you added.
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..6ab8b52 100644 --- a/src/pacman/conf.h +++ b/src/pacman/conf.h @@ -21,6 +21,7 @@ #define _PM_CONF_H
#include <alpm.h> +#include <handle.h>
typedef struct __colstr_t { const char *colon; @@ -113,6 +114,10 @@ typedef struct __config_t {
/* Color strings for output */ colstr_t colstr; + + /* Curl timeouts */ + long lowspeedlimit; + long lowspeedtime; } config_t;
/* Operations */ -- 1.9.0
On 3/26/14, 2:07 PM, Dave Reisner wrote:
What do you plan to set these values to in your environment? Why shouldn't we just adjust these to be more lax?
I set the low speed limit to zero and disable the check completely, because of the ridiculous firewall. I don't really understand the reason libcurl even has these options, but obviously someone thought it was a good reason to keep this functionality in pacman, so I opted to add the configuration options to pacman.conf instead. Possibly someone has intermittent problems with certain mirrors and this allows them to fall back to the next mirror. I have the privilege of an excellent connection and have never observed such an issue. The patch for simply disabling it is much simpler; simply change lib/libalpm/dload.c:297's "1L" to "0L", and erase the line following.
+/** Returns the libcurl low speed limit. */ +long alpm_option_get_lowspeedlimit(alpm_handle_t *handle); +/** Sets the libcurl low speed limit. */ +int alpm_option_set_lowspeedlimit(alpm_handle_t *handle, long lowspeedlimit);
What is this measured in ? bytes? megabytes? mebibytes?
+ +/** Returns the libcurl low speed time. */ +long alpm_option_get_lowspeedtime(alpm_handle_t *handle); +/** Sets the libcurl low speed time. */ +int alpm_option_set_lowspeedtime(alpm_handle_t *handle, long lowspeedtime); +
seconds? milliseconds? hours?
Bytes per second and seconds. These were noted in the pacman.conf documentation, but I forgot to add them here. I'll update it and submit a revised patch for your consideration.
@@ -690,6 +711,9 @@ static int setup_libalpm(void) alpm_option_set_questioncb(handle, cb_question); alpm_option_set_progresscb(handle, cb_progress);
+ handle->lowspeedlimit = config->lowspeedlimit; + handle->lowspeedtime = config->lowspeedtime; +
Not sure how this compiles -- the internals of the handle aren't visible from the frontend code. You need to use the setter methods you added.
It was by adding the #include <handle.h>, but I will adjust it as described here. Thanks for the comments, Andrew
On Wed, Mar 26, 2014 at 03:27:34PM -0400, Andrew Hills wrote:
On 3/26/14, 2:07 PM, Dave Reisner wrote:
What do you plan to set these values to in your environment? Why shouldn't we just adjust these to be more lax?
I set the low speed limit to zero and disable the check completely, because of the ridiculous firewall. I don't really understand the reason libcurl even has these options, but obviously someone thought it was a good reason to keep this functionality in pacman, so I opted to add the configuration options to pacman.conf instead. Possibly someone has intermittent problems with certain mirrors and this allows them to fall back to the next mirror. I have the privilege of an excellent connection and have never observed such an issue. The patch for simply disabling it is much simpler; simply change lib/libalpm/dload.c:297's "1L" to "0L", and erase the line following.
The low speed limit exists as a way of cancelling a download when the network goes away. Do you have an alternate suggestion?
+/** Returns the libcurl low speed limit. */ +long alpm_option_get_lowspeedlimit(alpm_handle_t *handle); +/** Sets the libcurl low speed limit. */ +int alpm_option_set_lowspeedlimit(alpm_handle_t *handle, long lowspeedlimit);
What is this measured in ? bytes? megabytes? mebibytes?
+ +/** Returns the libcurl low speed time. */ +long alpm_option_get_lowspeedtime(alpm_handle_t *handle); +/** Sets the libcurl low speed time. */ +int alpm_option_set_lowspeedtime(alpm_handle_t *handle, long lowspeedtime); +
seconds? milliseconds? hours?
Bytes per second and seconds. These were noted in the pacman.conf documentation, but I forgot to add them here. I'll update it and submit a revised patch for your consideration.
@@ -690,6 +711,9 @@ static int setup_libalpm(void) alpm_option_set_questioncb(handle, cb_question); alpm_option_set_progresscb(handle, cb_progress);
+ handle->lowspeedlimit = config->lowspeedlimit; + handle->lowspeedtime = config->lowspeedtime; +
Not sure how this compiles -- the internals of the handle aren't visible from the frontend code. You need to use the setter methods you added.
It was by adding the #include <handle.h>, but I will adjust it as described here.
Thanks for the comments, Andrew
On 3/26/14, 3:31 PM, Dave Reisner wrote:
The low speed limit exists as a way of cancelling a download when the network goes away. Do you have an alternate suggestion?
Ah, that makes sense. Yes, my suggestion is to make it configurable, so I can adapt the network disappearance detection fit my scenario better without changing the defaults that have worked well for me outside this particular network. Hence, the patch, which I hope I will be able to craft to your satisfaction soon.
Please find below the third iteration of the patch, now using the new alpm_option_set methods, and with more descriptive documenting comments. From 9272595b5207e07799dfd49bac9d27b699253297 Mon Sep 17 00:00:00 2001 From: Andrew Hills <ahills@ednos.net> Date: Wed, 19 Mar 2014 14:03:30 -0400 Subject: [PATCH] Add LowSpeedLimit and LowSpeedTime configuration options to correspond to libcurl's CURLOPT_LOW_SPEED_LIMIT and CURLOPT_LOW_SPEED_TIME options. Signed-off-by: Andrew Hills <ahills@ednos.net> --- doc/pacman.conf.5.txt | 14 ++++++++++++++ etc/pacman.conf.in | 2 ++ lib/libalpm/alpm.h | 10 ++++++++++ lib/libalpm/dload.c | 4 ++-- lib/libalpm/handle.c | 26 ++++++++++++++++++++++++++ lib/libalpm/handle.h | 4 ++++ src/pacman/conf.c | 24 ++++++++++++++++++++++++ src/pacman/conf.h | 4 ++++ 8 files changed, 86 insertions(+), 2 deletions(-) diff --git a/doc/pacman.conf.5.txt b/doc/pacman.conf.5.txt index f19311c..3e70573 100644 --- a/doc/pacman.conf.5.txt +++ b/doc/pacman.conf.5.txt @@ -199,6 +199,20 @@ 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 or a negative number 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. Setting 'time' to a negative number will + cause all downloads to fail. 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..d0cd01c 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,20 @@ 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); + handle->lowspeedlimit = lowspeedlimit; + return 0; +} + +int SYMEXPORT alpm_option_set_lowspeedtime(alpm_handle_t *handle, long lowspeedtime) +{ + CHECK_HANDLE(handle, return -1); + 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; }; alpm_handle_t *_alpm_handle_new(void); diff --git a/src/pacman/conf.c b/src/pacman/conf.c index f75f3a7..327ab50 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); + 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"), + file, linenum); + config->lowspeedtime = 10L; + } + pm_printf(ALPM_LOG_DEBUG, "config: lowspeedtime: %lu\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
On 26.03.2014 21:14, Andrew Hills wrote:
Please find below the third iteration of the patch, now using the new alpm_option_set methods, and with more descriptive documenting comments.
From 9272595b5207e07799dfd49bac9d27b699253297 Mon Sep 17 00:00:00 2001 From: Andrew Hills <ahills@ednos.net> Date: Wed, 19 Mar 2014 14:03:30 -0400 Subject: [PATCH] Add LowSpeedLimit and LowSpeedTime configuration options to correspond to libcurl's CURLOPT_LOW_SPEED_LIMIT and CURLOPT_LOW_SPEED_TIME options.
Please try to keep subjects for git commits around the 50 char mark (or shorter). The threads subject would be fine. Also try to add some explanatory text like why this change is needed and if applicable why it isn't done differently (in this case why you don't just increase the timeout) in the comment so this information isn't lost when someone steps through the git history in a couple of years. Believe it or not that will happen eventually and I'm always sad if I hit stuff with no comments. And finally, if you use git-send-email you can put comments that aren't part of the patch, like the ones you have written above the patch thus far, after the 3 dashes down below. I'm writing this mainly because I'm really used to the git way of things and I was rather confused when I read this thread. Also one more thing: You might want to use -v2, -v3 and so on when sending patches via git-send-email. Those options will change the subject to [PATCH v2] and similar.
Signed-off-by: Andrew Hills <ahills@ednos.net> ---
Extra (non-patch) comments mentioned above can go here.
doc/pacman.conf.5.txt | 14 ++++++++++++++ etc/pacman.conf.in | 2 ++ lib/libalpm/alpm.h | 10 ++++++++++ lib/libalpm/dload.c | 4 ++-- lib/libalpm/handle.c | 26 ++++++++++++++++++++++++++ lib/libalpm/handle.h | 4 ++++ src/pacman/conf.c | 24 ++++++++++++++++++++++++ src/pacman/conf.h | 4 ++++ 8 files changed, 86 insertions(+), 2 deletions(-)
diff --git a/doc/pacman.conf.5.txt b/doc/pacman.conf.5.txt index f19311c..3e70573 100644 --- a/doc/pacman.conf.5.txt +++ b/doc/pacman.conf.5.txt @@ -199,6 +199,20 @@ 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 or a negative number will disable the speed check. Defaults to [...] + Sets the time in seconds that a download should be below the `LowSpeedLimit`
Bad line breaks here, won't happen if you use git-send-email.
+ transfer speed to abort the transfer for being too slow. Setting 'time' to + 0 will disable the speed check. Setting 'time' to a negative number will + cause all downloads to fail. Defaults to 10 seconds. Note that this option + will not affect external programs specified by `XferCommand`.
You allow negative values here, but alpm_option_get_lowspeed*() use -1 to indicate an error. Setting the limit to -1 could result in wired stuff happening where the manpage would suggest that -1 simply results in download failures (which might be useful to someone who wants to test what happens in a dl failure case). Either say negative values result in undefined behaviour (which it is given we rely on upstream and upstream doesn't specify it) or don't allow them at all which is probably the best.
diff --git a/lib/libalpm/handle.c b/lib/libalpm/handle.c index 0842d51..d0cd01c 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);
@@ -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); + 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"), + file, linenum); + config->lowspeedtime = 10L; + } + pm_printf(ALPM_LOG_DEBUG, "config: lowspeedtime: %lu\n", config->lowspeedtime); } else { pm_printf(ALPM_LOG_WARNING, _("config file %s, line %d: directive '%s' in section '%s' not recognized.\n"),
Broken lines make reading this chunk hard so not reviewed. Rest looks fine to me.
On 3/26/14, 5:07 PM, Florian Pritz wrote:
Please try to keep subjects for git commits around the 50 char mark (or shorter). The threads subject would be fine.
Also try to add some explanatory text like why this change is needed and if applicable why it isn't done differently (in this case why you don't just increase the timeout) in the comment so this information isn't lost when someone steps through the git history in a couple of years. Believe it or not that will happen eventually and I'm always sad if I hit stuff with no comments.
And finally, if you use git-send-email you can put comments that aren't part of the patch, like the ones you have written above the patch thus far, after the 3 dashes down below.
I'm writing this mainly because I'm really used to the git way of things and I was rather confused when I read this thread.
Also one more thing: You might want to use -v2, -v3 and so on when sending patches via git-send-email. Those options will change the subject to [PATCH v2] and similar.
I will switch to git-send-email, and follow these guidelines; thanks for the suggestion.
You allow negative values here, but alpm_option_get_lowspeed*() use -1 to indicate an error. Setting the limit to -1 could result in wired stuff happening where the manpage would suggest that -1 simply results in download failures (which might be useful to someone who wants to test what happens in a dl failure case).
Either say negative values result in undefined behaviour (which it is given we rely on upstream and upstream doesn't specify it) or don't allow them at all which is probably the best.
Not allowing them at all seems like the most sane solution; I will revise the patch. Should the pacman.conf manual page explicitly disallow negative numbers? Thanks, Andrew
participants (4)
-
Allan McRae
-
Andrew Hills
-
Dave Reisner
-
Florian Pritz