On Wed, Mar 4, 2020 at 12:39 PM 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 --- 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*:: + 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. +

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 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);

/** @} */

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; +} \ 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 */ #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;

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);

Here is a question I have. What is the best way to handle int conversion errors for this option?

} 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);

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; /* select -Sc behavior */ unsigned short cleanmethod; alpm_list_t *holdpkg; -- 2.25.1

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 --- 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;

Hi On Thu, Mar 5, 2020 at 5:14 AM Allan McRae wrote:

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

Sure, I reverted the option name back to ParallelDownloads.

...

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

Ok, if the option is not set then the struct field is initialized with 1 (download stream).

...

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.

Done.

...

/** @} */

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.

Ok I set the field default to 1. And moved "5" as a default to pacman.conf. If the option is not set in pacman.conf then value "1" is used.

...

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.

Done.

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;

Patch V2 has been sent to the maillist. PTAL.