[pacman-dev] [PATCH 1/2] Add config option to specify amount of concurrent download streams

[Allan McRae]

On 5/3/20 6:38 am, Anatol Pomozov wrote:
> It includes pacman.conf new 'ConcurrentDownloadStreams' option that
> specifies how many concurrent downloads curl starts in parallel.
> 
> The value is set to '5' by default. Setting it to '0' removes upper
> limit on number of concurrent downloads.
> 
> Add alpm_option_set_concurrent_download_streams() ALPM function that
> allows to set this config option programmatically.
> 
> Signed-off-by: Anatol Pomozov <anatol.pomozov at gmail.com>
> ---
>  doc/pacman.conf.5.asciidoc |  5 +++++
>  etc/pacman.conf.in         |  1 +
>  lib/libalpm/alpm.h         |  1 +
>  lib/libalpm/handle.c       | 12 ++++++++++++
>  lib/libalpm/handle.h       |  1 +
>  src/pacman/conf.c          |  6 ++++++
>  src/pacman/conf.h          |  2 ++
>  7 files changed, 28 insertions(+)
> 
> diff --git a/doc/pacman.conf.5.asciidoc b/doc/pacman.conf.5.asciidoc
> index b297e332..fb00ad18 100644
> --- a/doc/pacman.conf.5.asciidoc
> +++ b/doc/pacman.conf.5.asciidoc
> @@ -205,6 +205,11 @@ Options
>  	Disable defaults for low speed limit and timeout on downloads. Use this
>  	if you have issues downloading files with proxy and/or security gateway.
>  
> +*ConcurrentDownloadStreams*::

*ConcurrentDownloadStreams =* 5

I'd prefer just "ParallelDownloads" or "ConcurrentDownloads"  Streams is
technical terminology.

> +	Specifies number of concurrent download streams. This value is set to 5
> +	by default. Setting this value to 0 removes upper limit of concurrent
> +	streams i.e. all files start downloading in parallel.
> +

I need to think of whether that is a sane implementation for this
setting being 0.

Clafiy that when unset, downloads proceed one at a time.

>  
>  Repository Sections
>  -------------------
> diff --git a/etc/pacman.conf.in b/etc/pacman.conf.in
> index 7446944f..0a857ef2 100644
> --- a/etc/pacman.conf.in
> +++ b/etc/pacman.conf.in
> @@ -34,6 +34,7 @@ Architecture = auto
>  #TotalDownload
>  CheckSpace
>  #VerbosePkgLists
> +#ConcurrentDownloadStreams = 5>
>  # PGP signature checking
>  #SigLevel = Optional

OK

> diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h
> index c2a069ad..1e488847 100644
> --- a/lib/libalpm/alpm.h
> +++ b/lib/libalpm/alpm.h
> @@ -902,6 +902,7 @@ int alpm_option_get_remote_file_siglevel(alpm_handle_t *handle);
>  int alpm_option_set_remote_file_siglevel(alpm_handle_t *handle, int level);
>  
>  int alpm_option_set_disable_dl_timeout(alpm_handle_t *handle, unsigned short disable_dl_timeout);
> +int alpm_option_set_concurrent_download_streams(alpm_handle_t *handle, unsigned int streams_num);

Can you document this function.

>  
>  /** @} */
>  
> diff --git a/lib/libalpm/handle.c b/lib/libalpm/handle.c
> index fc7c1faf..a74086d9 100644
> --- a/lib/libalpm/handle.c
> +++ b/lib/libalpm/handle.c
> @@ -856,3 +856,15 @@ int SYMEXPORT alpm_option_set_disable_dl_timeout(alpm_handle_t *handle,
>  #endif
>  	return 0;
>  }
> +
> +int SYMEXPORT alpm_option_set_concurrent_download_streams(alpm_handle_t *handle,
> +		unsigned int streams_num)
> +{
> +	CHECK_HANDLE(handle, return -1);
> +#ifdef HAVE_LIBCURL
> +	handle->concurrent_download_streams = streams_num;
> +#else
> +	(void)streams_num; /* silence unused variable warnings */
> +#endif
> +	return 0;
> +}

OK.

> \ No newline at end of file
> diff --git a/lib/libalpm/handle.h b/lib/libalpm/handle.h
> index c343f6e0..d6fe435c 100644
> --- a/lib/libalpm/handle.h
> +++ b/lib/libalpm/handle.h
> @@ -61,6 +61,7 @@ struct __alpm_handle_t {
>  	/* libcurl handle */
>  	CURL *curl;             /* reusable curl_easy handle */
>  	unsigned short disable_dl_timeout;
> +	unsigned int concurrent_download_streams; /* Number of parallel downloads, 0 - no limit */

Get rid of the  "0 - no limit" comment.  The range is defined by the
type, and it does not include "no limit"

>  #endif
>  
>  #ifdef HAVE_LIBGPGME
> diff --git a/src/pacman/conf.c b/src/pacman/conf.c
> index f9de386f..1b224eb6 100644
> --- a/src/pacman/conf.c
> +++ b/src/pacman/conf.c
> @@ -110,6 +110,8 @@ config_t *config_new(void)
>  		newconfig->localfilesiglevel = ALPM_SIG_USE_DEFAULT;
>  		newconfig->remotefilesiglevel = ALPM_SIG_USE_DEFAULT;
>  	}
> +	/* By default use 5 concurrent download streams */
> +	newconfig->concurrent_download_streams = 5;

So...  unsetting the variable in pacman.conf does not turn off the
feature.  That would be unexpected.

Default should 1.  If you allow the varaible to be specified without a
number, then that could default to 5.

>  
>  	newconfig->colstr.colon   = ":: ";
>  	newconfig->colstr.title   = "";
> @@ -677,6 +679,9 @@ static int _parse_options(const char *key, char *value,
>  				return 1;
>  			}
>  			FREELIST(values);
> +		} else if(strcmp(key, "ConcurrentDownloadStreams") == 0) {
> +			/* TODO: what is the best way to handle int conversion errors? */
> +			config->concurrent_download_streams = atoi(value);

Follow the example in "man strtol", though you need to check for a
positive number.  And potential overflow?  Probably best to dump that to
a utility function.

Again... are you having the ability to specify "ConcurrentDownload..."
in pacman.conf without a value?

>  		} else {
>  			pm_printf(ALPM_LOG_WARNING,
>  					_("config file %s, line %d: directive '%s' in section '%s' not recognized.\n"),
> @@ -845,6 +850,7 @@ static int setup_libalpm(void)
>  	alpm_option_set_noextracts(handle, config->noextract);
>  
>  	alpm_option_set_disable_dl_timeout(handle, config->disable_dl_timeout);
> +	alpm_option_set_concurrent_download_streams(handle, config->concurrent_download_streams);
>  

OK.

>  	for(i = config->assumeinstalled; i; i = i->next) {
>  		char *entry = i->data;
> diff --git a/src/pacman/conf.h b/src/pacman/conf.h
> index d954e637..33773efd 100644
> --- a/src/pacman/conf.h
> +++ b/src/pacman/conf.h
> @@ -115,6 +115,8 @@ typedef struct __config_t {
>  	/* When downloading, display the amount downloaded, rate, ETA, and percent
>  	 * downloaded of the total download list */
>  	unsigned short totaldownload;
> +	/* Number of concurrent download streams, 0 - no limit */
> +	unsigned int concurrent_download_streams;

When the variable name is exactly the comment, you probably don't need
the comment!  Again, "no limit" is not right.

>  	/* select -Sc behavior */
>  	unsigned short cleanmethod;
>  	alpm_list_t *holdpkg;
>