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

Message ID 20200304203853.99118-1-anatol.pomozov@gmail.com
State Superseded, archived
Headers show
Series
  • [pacman-dev,1/2] Add config option to specify amount of concurrent download streams
Related show

Commit Message

Anatol Pomozov March 4, 2020, 8:38 p.m. UTC
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(+)

Comments

Anatol Pomozov March 4, 2020, 9:29 p.m. UTC | #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
>
brainpower March 4, 2020, 10:32 p.m. UTC | #2
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
brainpower March 4, 2020, 10:40 p.m. UTC | #3
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.
> 
> [1]: https://en.cppreference.com/w/c/string/byte/strtol
> 
> 
>
brainpower March 4, 2020, 11:19 p.m. UTC | #4
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.
>>
>> [1]: https://en.cppreference.com/w/c/string/byte/strtol
>>
>>
>>
> 
>
Allan McRae March 5, 2020, 5:03 a.m. UTC | #5
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.
>>
>> [1]: https://en.cppreference.com/w/c/string/byte/strtol
>>

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
Allan McRae March 5, 2020, 1:14 p.m. UTC | #6
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;
>
Anatol Pomozov March 6, 2020, 4:56 a.m. UTC | #7
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.

Patch

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;