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

[Anatol Pomozov]

Hi

On Thu, Mar 5, 2020 at 5:14 AM Allan McRae <allan at 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 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.

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.