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

[Florian Pritz]

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 at 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 at 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.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <http://mailman.archlinux.org/pipermail/pacman-dev/attachments/20140326/618251a9/attachment-0001.asc>