[pacman-dev] [PATCH 1/2] Add config option to specify amount of concurrent download streams
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@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*:: + 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); } 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
'output' is a list of messages that pacman received but delayed printing to avoid messing with UI. Such functionality is useful for the upcoming multi-line progress bar UI. Let's move it to a separate function. Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com> --- src/pacman/callback.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/src/pacman/callback.c b/src/pacman/callback.c index 548e2df2..432fc734 100644 --- a/src/pacman/callback.c +++ b/src/pacman/callback.c @@ -160,6 +160,16 @@ static void fill_progress(const int bar_percent, const int disp_percent, fflush(stdout); } +static void flush_output_list(void) { + alpm_list_t *i = NULL; + fflush(stdout); + for(i = output; i; i = i->next) { + fputs((const char *)i->data, stderr); + } + fflush(stderr); + FREELIST(output); +} + static int number_length(size_t n) { int digits = 1; @@ -610,14 +620,9 @@ void cb_progress(alpm_progress_t event, const char *pkgname, int percent, fill_progress(percent, percent, cols - infolen); if(percent == 100) { - alpm_list_t *i = NULL; + putchar('\n'); + flush_output_list(); on_progress = 0; - fflush(stdout); - for(i = output; i; i = i->next) { - fputs((const char *)i->data, stderr); - } - fflush(stderr); - FREELIST(output); } else { on_progress = 1; } -- 2.25.1
On 5/3/20 6:38 am, Anatol Pomozov wrote:
'output' is a list of messages that pacman received but delayed printing to avoid messing with UI.
Such functionality is useful for the upcoming multi-line progress bar UI. Let's move it to a separate function.
Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com> --- src/pacman/callback.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/src/pacman/callback.c b/src/pacman/callback.c index 548e2df2..432fc734 100644 --- a/src/pacman/callback.c +++ b/src/pacman/callback.c @@ -160,6 +160,16 @@ static void fill_progress(const int bar_percent, const int disp_percent, fflush(stdout); }
+static void flush_output_list(void) { + alpm_list_t *i = NULL; + fflush(stdout); + for(i = output; i; i = i->next) { + fputs((const char *)i->data, stderr); + } + fflush(stderr); + FREELIST(output); +} + static int number_length(size_t n) { int digits = 1; @@ -610,14 +620,9 @@ void cb_progress(alpm_progress_t event, const char *pkgname, int percent, fill_progress(percent, percent, cols - infolen);
if(percent == 100) { - alpm_list_t *i = NULL; + putchar('\n');
New line return added here. Why is that needed?
+ flush_output_list(); on_progress = 0; - fflush(stdout); - for(i = output; i; i = i->next) { - fputs((const char *)i->data, stderr); - } - fflush(stderr); - FREELIST(output); } else { on_progress = 1; }
Hi On Wed, Mar 4, 2020 at 9:16 PM Allan McRae <allan@archlinux.org> wrote:
On 5/3/20 6:38 am, Anatol Pomozov wrote:
'output' is a list of messages that pacman received but delayed printing to avoid messing with UI.
Such functionality is useful for the upcoming multi-line progress bar UI. Let's move it to a separate function.
Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com> --- src/pacman/callback.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/src/pacman/callback.c b/src/pacman/callback.c index 548e2df2..432fc734 100644 --- a/src/pacman/callback.c +++ b/src/pacman/callback.c @@ -160,6 +160,16 @@ static void fill_progress(const int bar_percent, const int disp_percent, fflush(stdout); }
+static void flush_output_list(void) { + alpm_list_t *i = NULL; + fflush(stdout); + for(i = output; i; i = i->next) { + fputs((const char *)i->data, stderr); + } + fflush(stderr); + FREELIST(output); +} + static int number_length(size_t n) { int digits = 1; @@ -610,14 +620,9 @@ void cb_progress(alpm_progress_t event, const char *pkgname, int percent, fill_progress(percent, percent, cols - infolen);
if(percent == 100) { - alpm_list_t *i = NULL; + putchar('\n');
New line return added here. Why is that needed?
Oops, it leaked from the other (future) change. Will resend an update soon.
+ flush_output_list(); on_progress = 0; - fflush(stdout); - for(i = output; i; i = i->next) { - fputs((const char *)i->data, stderr); - } - fflush(stderr); - FREELIST(output); } else { on_progress = 1; }
On Wed, Mar 4, 2020 at 12:39 PM Anatol Pomozov <anatol.pomozov@gmail.com> 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@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*:: + 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
Hi! Am 04.03.20 um 22:29 schrieb Anatol Pomozov:
On Wed, Mar 4, 2020 at 12:39 PM Anatol Pomozov <anatol.pomozov@gmail.com> wrote:
+ } 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?
I'd recommend strtol() [1] over atoi() any time. It makes it a lot easier to get error handling right, well, in most cases actually possible at all! I do not know of any way to differentiate between a valid "0" input and the error case with atoi(). There is no way to detect if the input was out of range, atoi() just gives some undefined value. (The only valid use case for atoi() I might find acceptable would be if you can be *absolutely* sure the input is a valid int. e.g. validate before passing to atoi) With strtol() do the following: 1. Call strol() 2. Check if *end is NULL, if it is not, parsing was aborted at the position *end points to 3. Check errno for ERANGE, it gets set if the integer given does not fit into a long 4. Now use the number. Check range again, if you want to downcast the long to int. [1]: https://en.cppreference.com/w/c/string/byte/strtol -- regards, brainpower
Am 04.03.20 um 23:32 schrieb brainpower:
Hi!
Am 04.03.20 um 22:29 schrieb Anatol Pomozov:
On Wed, Mar 4, 2020 at 12:39 PM Anatol Pomozov <anatol.pomozov@gmail.com> wrote:
+ } 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?
I'd recommend strtol() [1] over atoi() any time. It makes it a lot easier to get error handling right, well, in most cases actually possible at all!
I do not know of any way to differentiate between a valid "0" input and the error case with atoi(). There is no way to detect if the input was out of range, atoi() just gives some undefined value. (The only valid use case for atoi() I might find acceptable would be if you can be *absolutely* sure the input is a valid int. e.g. validate before passing to atoi)
With strtol() do the following:
1. Call strol() 2. Check if *end is NULL, if it is not, parsing was aborted at the position *end points to
Sorry. I messed up and misread the documentation here. The first part of the above is incorrect. You'll have to check str != end, where str is the first pointer passed to strtol and end the second.
3. Check errno for ERANGE, it gets set if the integer given does not fit into a long 4. Now use the number. Check range again, if you want to downcast the long to int.
-- regards, brainpower
Am 04.03.20 um 23:40 schrieb brainpower:
Am 04.03.20 um 23:32 schrieb brainpower:
Hi!
Am 04.03.20 um 22:29 schrieb Anatol Pomozov:
On Wed, Mar 4, 2020 at 12:39 PM Anatol Pomozov <anatol.pomozov@gmail.com> wrote:
+ } 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?
I'd recommend strtol() [1] over atoi() any time. It makes it a lot easier to get error handling right, well, in most cases actually possible at all!
I do not know of any way to differentiate between a valid "0" input and the error case with atoi(). There is no way to detect if the input was out of range, atoi() just gives some undefined value. (The only valid use case for atoi() I might find acceptable would be if you can be *absolutely* sure the input is a valid int. e.g. validate before passing to atoi)
With strtol() do the following:
1. Call strol() 2. Check if *end is NULL, if it is not, parsing was aborted at the position *end points to
Sorry. I messed up and misread the documentation here. The first part of the above is incorrect.
You'll have to check str != end, where str is the first pointer passed to strtol and end the second.
ah, sorry. Not my day today. I forgot to mention the if (str == end) { /* error */ } check is incomplete, it'll only check if parsing did not fail at the first char. So it would not detect garbage at the end of the string. "abc500" would be detected, "500abc" would get you 500 and discard the "abc". With the case I had in mind where I needed this last, that behavior was the result I wanted, that's probably not be the case here. If you want to detect garbage at the end you'll have to check if end points to the end of the string, so something like if ((end - str) < strlen(str)) { /* error */ } .
3. Check errno for ERANGE, it gets set if the integer given does not fit into a long 4. Now use the number. Check range again, if you want to downcast the long to int.
-- regards, brainpower
On 5/3/20 8:40 am, brainpower wrote:
Am 04.03.20 um 23:32 schrieb brainpower:
Hi!
Am 04.03.20 um 22:29 schrieb Anatol Pomozov:
On Wed, Mar 4, 2020 at 12:39 PM Anatol Pomozov <anatol.pomozov@gmail.com> wrote:
+ } 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?
I'd recommend strtol() [1] over atoi() any time. It makes it a lot easier to get error handling right, well, in most cases actually possible at all!
I do not know of any way to differentiate between a valid "0" input and the error case with atoi(). There is no way to detect if the input was out of range, atoi() just gives some undefined value. (The only valid use case for atoi() I might find acceptable would be if you can be *absolutely* sure the input is a valid int. e.g. validate before passing to atoi)
With strtol() do the following:
1. Call strol() 2. Check if *end is NULL, if it is not, parsing was aborted at the position *end points to
Sorry. I messed up and misread the documentation here. The first part of the above is incorrect.
You'll have to check str != end, where str is the first pointer passed to strtol and end the second.
3. Check errno for ERANGE, it gets set if the integer given does not fit into a long 4. Now use the number. Check range again, if you want to downcast the long to int.
man strtol has an example usage program, and provides instuctions on how to check for "0" from input vs error. For pacman, that would likely be considered an error anyway. A
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@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;
Hi On Thu, Mar 5, 2020 at 5:14 AM Allan McRae <allan@archlinux.org> 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 <anatol.pomozov@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.
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.
participants (3)
-
Allan McRae
-
Anatol Pomozov
-
brainpower