Re: [pacman-dev] [PATCH] Convert '-U pkg1 pkg2' codepath to parallel download
Hi
On Wed, Jun 10, 2020 at 7:57 PM Allan McRae wrote:
>
> On 11/5/20 6:50 pm, Anatol Pomozov wrote:
> > Installing remote packages using its URL is an interesting case for ALPM
> > API. Unlike package sync ('pacman -S pkg1 pkg2') '-U' does not deal with
> > server mirror list. Thus _alpm_multi_download() should be able to
> > handle file download for payloads that either have 'fileurl' field
> > or pair of fields ('servers' and 'filepath') set.
> >
> > Signature for alpm_fetch_pkgurl() has changed and it accepts an
> > output list that is populated with filepaths to fetched packages.
> >
> > Signed-off-by: Anatol Pomozov
>
> Thanks. Very minor comments belown.
>
> > ---
> > lib/libalpm/alpm.h | 11 ++-
> > lib/libalpm/dload.c | 204 +--
> > lib/libalpm/dload.h | 7 +-
> > src/pacman/upgrade.c | 102 ++
> > 4 files changed, 181 insertions(+), 143 deletions(-)
> >
> > diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h
> > index 3ea66ccc..c6a13273 100644
> > --- a/lib/libalpm/alpm.h
> > +++ b/lib/libalpm/alpm.h
> > @@ -755,12 +755,15 @@ typedef void (*alpm_cb_totaldl)(off_t total);
> > typedef int (*alpm_cb_fetch)(const char *url, const char *localpath,
> > int force);
> >
> > -/** Fetch a remote pkg.
> > +/** Fetch a list of remote packages.
> > * @param handle the context handle
> > - * @param url URL of the package to download
> > - * @return the downloaded filepath on success, NULL on error
> > + * @param urls list of package URLs to download
> > + * @param fetched list of filepaths to the fetched packages, each item
> > + *corresponds to one in `urls` list
>
> Can you make it clear that this is filled by the function, and not
> something that should be passed.
Done.
>
> > + * @return 0 on success or -1 on failure
> > */
> > -char *alpm_fetch_pkgurl(alpm_handle_t *handle, const char *url);
> > +int alpm_fetch_pkgurl(alpm_handle_t *handle, const alpm_list_t *urls,
> > + alpm_list_t **fetched);
> >
> > /** @addtogroup alpm_api_options Options
> > * Libalpm option getters and setters
> > diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c
> > index 13aa4086..43fe9847 100644
> > --- a/lib/libalpm/dload.c
> > +++ b/lib/libalpm/dload.c
> > @@ -613,7 +613,9 @@ static int curl_multi_retry_next_server(CURLM *curlm,
> > CURL *curl, struct dload_p
> > size_t len;
> > alpm_handle_t *handle = payload->handle;
> >
> > - payload->servers = payload->servers->next;
> > + if(payload->servers) {
> > + payload->servers = payload->servers->next;
> > + }
>
> OK.
>
> > if(!payload->servers) {
> > _alpm_log(payload->handle, ALPM_LOG_DEBUG,
> > "%s: no more servers to retry\n",
> > payload->remote_name);
> > @@ -622,7 +624,7 @@ static int curl_multi_retry_next_server(CURLM *curlm,
> > CURL *curl, struct dload_p
> > server = payload->servers->data;
> >
> > /* regenerate a new fileurl */
> > - free(payload->fileurl);
> > + FREE(payload->fileurl);
>
> Change unrelated to this patch, but OK.
>
> > len = strlen(server) + strlen(payload->filepath) + 2;
> > MALLOC(payload->fileurl, len, RET_ERR(handle, ALPM_ERR_MEMORY, -1));
> > snprintf(payload->fileurl, len, "%s/%s", server, payload->filepath);
> > @@ -849,20 +851,28 @@ static int curl_multi_add_payload(alpm_handle_t
> > *handle, CURLM *curlm,
> > struct dload_payload *payload, const char *localpath)
> > {
> > size_t len;
> > - const char *server;
> > CURL *curl = NULL;
> > char hostname[HOSTNAME_SIZE];
> > int ret = -1;
> >
> > - ASSERT(payload->servers, RET_ERR(handle, ALPM_ERR_SERVER_NONE, -1));
> > - server = payload->servers->data;
> > -
> > curl = curl_easy_init();
> > payload->curl = curl;
> >
> > - len = strlen(server) + strlen(payload->filepath) + 2;
> > - MALLOC(payload->fileurl, len, GOTO_ERR(handle, ALPM_ERR_MEMORY,
> > cleanup));
> > - snprintf(payload->fileurl, len, "%s/%s", server, payload->filepath);
> > + if(payload->fileurl) {
> > + ASSERT(!payload->servers, GOTO_ERR(handle,
> > ALPM_ERR_WRONG_ARGS, cleanup));
> > + ASSERT(!payload->filepath, GOTO_ERR(handle,
> > ALPM_ERR_WRONG_ARGS, cleanup));
>
> OK.
>
> > + } else {
> > + const char *server;
> > +
> > + ASSERT(payload->servers, GOTO_ERR(handle,
> > ALPM_ERR_SERVER_NONE, cleanup));
> > + ASSERT(payload->filepath, GOTO_ERR(handle,
> > ALPM_ERR_WRONG_ARGS, cleanup));
> > +
> > + server = payload->servers->data;
> > +
> > + len = strlen(server) + strlen(payload->filepath) + 2;
> > + MALLOC(payload->fileurl, len, GOTO_ERR(handle,
> > ALPM_ERR_MEMORY, cleanup));
> > + snprintf(payload->fileurl, len, "%s/%s", server,
> > payload->filepath);
> > + }
> >
>
> OK.
>
> >
Re: [pacman-dev] [PATCH] Convert '-U pkg1 pkg2' codepath to parallel download
On 11/5/20 6:50 pm, Anatol Pomozov wrote:
> Installing remote packages using its URL is an interesting case for ALPM
> API. Unlike package sync ('pacman -S pkg1 pkg2') '-U' does not deal with
> server mirror list. Thus _alpm_multi_download() should be able to
> handle file download for payloads that either have 'fileurl' field
> or pair of fields ('servers' and 'filepath') set.
>
> Signature for alpm_fetch_pkgurl() has changed and it accepts an
> output list that is populated with filepaths to fetched packages.
>
> Signed-off-by: Anatol Pomozov
Thanks. Very minor comments belown.
> ---
> lib/libalpm/alpm.h | 11 ++-
> lib/libalpm/dload.c | 204 +--
> lib/libalpm/dload.h | 7 +-
> src/pacman/upgrade.c | 102 ++
> 4 files changed, 181 insertions(+), 143 deletions(-)
>
> diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h
> index 3ea66ccc..c6a13273 100644
> --- a/lib/libalpm/alpm.h
> +++ b/lib/libalpm/alpm.h
> @@ -755,12 +755,15 @@ typedef void (*alpm_cb_totaldl)(off_t total);
> typedef int (*alpm_cb_fetch)(const char *url, const char *localpath,
> int force);
>
> -/** Fetch a remote pkg.
> +/** Fetch a list of remote packages.
> * @param handle the context handle
> - * @param url URL of the package to download
> - * @return the downloaded filepath on success, NULL on error
> + * @param urls list of package URLs to download
> + * @param fetched list of filepaths to the fetched packages, each item
> + *corresponds to one in `urls` list
Can you make it clear that this is filled by the function, and not
something that should be passed.
> + * @return 0 on success or -1 on failure
> */
> -char *alpm_fetch_pkgurl(alpm_handle_t *handle, const char *url);
> +int alpm_fetch_pkgurl(alpm_handle_t *handle, const alpm_list_t *urls,
> + alpm_list_t **fetched);
>
> /** @addtogroup alpm_api_options Options
> * Libalpm option getters and setters
> diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c
> index 13aa4086..43fe9847 100644
> --- a/lib/libalpm/dload.c
> +++ b/lib/libalpm/dload.c
> @@ -613,7 +613,9 @@ static int curl_multi_retry_next_server(CURLM *curlm,
> CURL *curl, struct dload_p
> size_t len;
> alpm_handle_t *handle = payload->handle;
>
> - payload->servers = payload->servers->next;
> + if(payload->servers) {
> + payload->servers = payload->servers->next;
> + }
OK.
> if(!payload->servers) {
> _alpm_log(payload->handle, ALPM_LOG_DEBUG,
> "%s: no more servers to retry\n",
> payload->remote_name);
> @@ -622,7 +624,7 @@ static int curl_multi_retry_next_server(CURLM *curlm,
> CURL *curl, struct dload_p
> server = payload->servers->data;
>
> /* regenerate a new fileurl */
> - free(payload->fileurl);
> + FREE(payload->fileurl);
Change unrelated to this patch, but OK.
> len = strlen(server) + strlen(payload->filepath) + 2;
> MALLOC(payload->fileurl, len, RET_ERR(handle, ALPM_ERR_MEMORY, -1));
> snprintf(payload->fileurl, len, "%s/%s", server, payload->filepath);
> @@ -849,20 +851,28 @@ static int curl_multi_add_payload(alpm_handle_t
> *handle, CURLM *curlm,
> struct dload_payload *payload, const char *localpath)
> {
> size_t len;
> - const char *server;
> CURL *curl = NULL;
> char hostname[HOSTNAME_SIZE];
> int ret = -1;
>
> - ASSERT(payload->servers, RET_ERR(handle, ALPM_ERR_SERVER_NONE, -1));
> - server = payload->servers->data;
> -
> curl = curl_easy_init();
> payload->curl = curl;
>
> - len = strlen(server) + strlen(payload->filepath) + 2;
> - MALLOC(payload->fileurl, len, GOTO_ERR(handle, ALPM_ERR_MEMORY,
> cleanup));
> - snprintf(payload->fileurl, len, "%s/%s", server, payload->filepath);
> + if(payload->fileurl) {
> + ASSERT(!payload->servers, GOTO_ERR(handle, ALPM_ERR_WRONG_ARGS,
> cleanup));
> + ASSERT(!payload->filepath, GOTO_ERR(handle,
> ALPM_ERR_WRONG_ARGS, cleanup));
OK.
> + } else {
> + const char *server;
> +
> + ASSERT(payload->servers, GOTO_ERR(handle, ALPM_ERR_SERVER_NONE,
> cleanup));
> + ASSERT(payload->filepath, GOTO_ERR(handle, ALPM_ERR_WRONG_ARGS,
> cleanup));
> +
> + server = payload->servers->data;
> +
> + len = strlen(server) + strlen(payload->filepath) + 2;
> + MALLOC(payload->fileurl, len, GOTO_ERR(handle, ALPM_ERR_MEMORY,
> cleanup));
> + snprintf(payload->fileurl, len, "%s/%s", server,
> payload->filepath);
> + }
>
OK.
> payload->tempfile_openmode = "wb";
> if(!payload->remote_name) {
> @@ -1046,22 +1056,29 @@ int _alpm_multi_download(alpm_handle_t *handle,
> alpm_list_t *s;
> int success = 0;
>
> - for(s = payload->servers; s; s = s->next) {
> -
[pacman-dev] [PATCH] Convert '-U pkg1 pkg2' codepath to parallel download
Installing remote packages using its URL is an interesting case for ALPM
API. Unlike package sync ('pacman -S pkg1 pkg2') '-U' does not deal with
server mirror list. Thus _alpm_multi_download() should be able to
handle file download for payloads that either have 'fileurl' field
or pair of fields ('servers' and 'filepath') set.
Signature for alpm_fetch_pkgurl() has changed and it accepts an
output list that is populated with filepaths to fetched packages.
Signed-off-by: Anatol Pomozov
---
lib/libalpm/alpm.h | 11 ++-
lib/libalpm/dload.c | 204 +--
lib/libalpm/dload.h | 7 +-
src/pacman/upgrade.c | 102 ++
4 files changed, 181 insertions(+), 143 deletions(-)
diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h
index 3ea66ccc..c6a13273 100644
--- a/lib/libalpm/alpm.h
+++ b/lib/libalpm/alpm.h
@@ -755,12 +755,15 @@ typedef void (*alpm_cb_totaldl)(off_t total);
typedef int (*alpm_cb_fetch)(const char *url, const char *localpath,
int force);
-/** Fetch a remote pkg.
+/** Fetch a list of remote packages.
* @param handle the context handle
- * @param url URL of the package to download
- * @return the downloaded filepath on success, NULL on error
+ * @param urls list of package URLs to download
+ * @param fetched list of filepaths to the fetched packages, each item
+ *corresponds to one in `urls` list
+ * @return 0 on success or -1 on failure
*/
-char *alpm_fetch_pkgurl(alpm_handle_t *handle, const char *url);
+int alpm_fetch_pkgurl(alpm_handle_t *handle, const alpm_list_t *urls,
+ alpm_list_t **fetched);
/** @addtogroup alpm_api_options Options
* Libalpm option getters and setters
diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c
index 13aa4086..43fe9847 100644
--- a/lib/libalpm/dload.c
+++ b/lib/libalpm/dload.c
@@ -613,7 +613,9 @@ static int curl_multi_retry_next_server(CURLM *curlm, CURL
*curl, struct dload_p
size_t len;
alpm_handle_t *handle = payload->handle;
- payload->servers = payload->servers->next;
+ if(payload->servers) {
+ payload->servers = payload->servers->next;
+ }
if(!payload->servers) {
_alpm_log(payload->handle, ALPM_LOG_DEBUG,
"%s: no more servers to retry\n",
payload->remote_name);
@@ -622,7 +624,7 @@ static int curl_multi_retry_next_server(CURLM *curlm, CURL
*curl, struct dload_p
server = payload->servers->data;
/* regenerate a new fileurl */
- free(payload->fileurl);
+ FREE(payload->fileurl);
len = strlen(server) + strlen(payload->filepath) + 2;
MALLOC(payload->fileurl, len, RET_ERR(handle, ALPM_ERR_MEMORY, -1));
snprintf(payload->fileurl, len, "%s/%s", server, payload->filepath);
@@ -849,20 +851,28 @@ static int curl_multi_add_payload(alpm_handle_t *handle,
CURLM *curlm,
struct dload_payload *payload, const char *localpath)
{
size_t len;
- const char *server;
CURL *curl = NULL;
char hostname[HOSTNAME_SIZE];
int ret = -1;
- ASSERT(payload->servers, RET_ERR(handle, ALPM_ERR_SERVER_NONE, -1));
- server = payload->servers->data;
-
curl = curl_easy_init();
payload->curl = curl;
- len = strlen(server) + strlen(payload->filepath) + 2;
- MALLOC(payload->fileurl, len, GOTO_ERR(handle, ALPM_ERR_MEMORY,
cleanup));
- snprintf(payload->fileurl, len, "%s/%s", server, payload->filepath);
+ if(payload->fileurl) {
+ ASSERT(!payload->servers, GOTO_ERR(handle, ALPM_ERR_WRONG_ARGS,
cleanup));
+ ASSERT(!payload->filepath, GOTO_ERR(handle,
ALPM_ERR_WRONG_ARGS, cleanup));
+ } else {
+ const char *server;
+
+ ASSERT(payload->servers, GOTO_ERR(handle, ALPM_ERR_SERVER_NONE,
cleanup));
+ ASSERT(payload->filepath, GOTO_ERR(handle, ALPM_ERR_WRONG_ARGS,
cleanup));
+
+ server = payload->servers->data;
+
+ len = strlen(server) + strlen(payload->filepath) + 2;
+ MALLOC(payload->fileurl, len, GOTO_ERR(handle, ALPM_ERR_MEMORY,
cleanup));
+ snprintf(payload->fileurl, len, "%s/%s", server,
payload->filepath);
+ }
payload->tempfile_openmode = "wb";
if(!payload->remote_name) {
@@ -1046,22 +1056,29 @@ int _alpm_multi_download(alpm_handle_t *handle,
alpm_list_t *s;
int success = 0;
- for(s = payload->servers; s; s = s->next) {
- const char *server = s->data;
- char *fileurl;
- int ret;
-
- size_t len = strlen(server) +
strlen(payload->filepath) + 2;
- MALLOC(fileurl, len, RET_ERR(handle,
ALPM_ERR_MEMORY, -1));
- snprintf(fileurl, len,
