[pacman-dev] libalpm: fix alpm_fetch_pkgurl with a fetch callback

Message ID ada1ad50-78fb-e9ba-2cd9-703ffdac628b@manjaro.org
State New
Headers show
Series [pacman-dev] libalpm: fix alpm_fetch_pkgurl with a fetch callback | expand

Commit Message

Guillaume Benoit April 30, 2021, 10:09 a.m. UTC
After download, alpm_fetch_pkgurl uses payload->destfile_name
and payload->tempfile_name in order to find the downloaded file path.
Those fields are not set if a custom fetch callback is defined.
Use filecache_find_url instead, like in the beginning of the function.

---
  lib/libalpm/dload.c | 13 ++++---------
  1 file changed, 4 insertions(+), 9 deletions(-)

Comments

Andrew Gregory May 1, 2021, 3:53 a.m. UTC | #1
On 04/30/21 at 12:09pm, Guillaume Benoit wrote:
> After download, alpm_fetch_pkgurl uses payload->destfile_name
> and payload->tempfile_name in order to find the downloaded file path.
> Those fields are not set if a custom fetch callback is defined.
> Use filecache_find_url instead, like in the beginning of the function.
> 
> ---
>  lib/libalpm/dload.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c
> index 6f33451a..d45c7707 100644
> --- a/lib/libalpm/dload.c
> +++ b/lib/libalpm/dload.c
> @@ -1007,16 +1007,11 @@ int SYMEXPORT alpm_fetch_pkgurl(alpm_handle_t *handle, const alpm_list_t *urls,
>  			EVENT(handle, &event);
>  		}
>  -		for(i = payloads; i; i = i->next) {
> -			struct dload_payload *payload = i->data;
> -			char *filepath;
> +		for(i = urls; i; i = i->next) {
> +			char *url = i->data;
>  -			if(payload->destfile_name) {
> -				const char *filename = mbasename(payload->destfile_name);
> -				filepath = _alpm_filecache_find(handle, filename);
> -			} else {
> -				STRDUP(filepath, payload->tempfile_name, GOTO_ERR(handle, ALPM_ERR_MEMORY, err));
> -			}
> +			/* attempt again to find the file in our pkgcache */
> +			char *filepath = filecache_find_url(handle, url);
>  			if(filepath) {
>  				alpm_list_append(fetched, filepath);
>  			} else {

Without testing since this needs to be rebased now; this looks broken.
filecache_find_url with fail for any url where destfile_name doesn't
match the file name in the url.  Notably, that includes the download
links on Arch's package pages:
https://archlinux.org/packages/core/x86_64/pacman/download/
Guillaume Benoit May 1, 2021, 2:33 p.m. UTC | #2
Le 01/05/2021 à 05:53, Andrew Gregory a écrit :
> On 04/30/21 at 12:09pm, Guillaume Benoit wrote:
>> After download, alpm_fetch_pkgurl uses payload->destfile_name
>> and payload->tempfile_name in order to find the downloaded file path.
>> Those fields are not set if a custom fetch callback is defined.
>> Use filecache_find_url instead, like in the beginning of the function.
>>
>> ---
>>   lib/libalpm/dload.c | 13 ++++---------
>>   1 file changed, 4 insertions(+), 9 deletions(-)
>>
>> diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c
>> index 6f33451a..d45c7707 100644
>> --- a/lib/libalpm/dload.c
>> +++ b/lib/libalpm/dload.c
>> @@ -1007,16 +1007,11 @@ int SYMEXPORT alpm_fetch_pkgurl(alpm_handle_t *handle, const alpm_list_t *urls,
>>   			EVENT(handle, &event);
>>   		}
>>   -		for(i = payloads; i; i = i->next) {
>> -			struct dload_payload *payload = i->data;
>> -			char *filepath;
>> +		for(i = urls; i; i = i->next) {
>> +			char *url = i->data;
>>   -			if(payload->destfile_name) {
>> -				const char *filename = mbasename(payload->destfile_name);
>> -				filepath = _alpm_filecache_find(handle, filename);
>> -			} else {
>> -				STRDUP(filepath, payload->tempfile_name, GOTO_ERR(handle, ALPM_ERR_MEMORY, err));
>> -			}
>> +			/* attempt again to find the file in our pkgcache */
>> +			char *filepath = filecache_find_url(handle, url);
>>   			if(filepath) {
>>   				alpm_list_append(fetched, filepath);
>>   			} else {
> 
> Without testing since this needs to be rebased now; this looks broken.
> filecache_find_url with fail for any url where destfile_name doesn't
> match the file name in the url.  Notably, that includes the download
> links on Arch's package pages:
> https://archlinux.org/packages/core/x86_64/pacman/download/
> 
Yes, with or without this patch, the current implementation doesn't work for this kind of url when using a fetch callback.
The solution I see is to change the fetch callback signature by making it return the path of the downloaded file.
Otherwise this patch works for standard mirror urls.
Allan McRae May 2, 2021, 9:47 a.m. UTC | #3
On 2/5/21 12:33 am, Guillaume Benoit wrote:
> Le 01/05/2021 à 05:53, Andrew Gregory a écrit :
>> On 04/30/21 at 12:09pm, Guillaume Benoit wrote:
>>> After download, alpm_fetch_pkgurl uses payload->destfile_name
>>> and payload->tempfile_name in order to find the downloaded file path.
>>> Those fields are not set if a custom fetch callback is defined.
>>> Use filecache_find_url instead, like in the beginning of the function.
>>>
>>> ---
>>>   lib/libalpm/dload.c | 13 ++++---------
>>>   1 file changed, 4 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c
>>> index 6f33451a..d45c7707 100644
>>> --- a/lib/libalpm/dload.c
>>> +++ b/lib/libalpm/dload.c
>>> @@ -1007,16 +1007,11 @@ int SYMEXPORT alpm_fetch_pkgurl(alpm_handle_t
>>> *handle, const alpm_list_t *urls,
>>>               EVENT(handle, &event);
>>>           }
>>>   -        for(i = payloads; i; i = i->next) {
>>> -            struct dload_payload *payload = i->data;
>>> -            char *filepath;
>>> +        for(i = urls; i; i = i->next) {
>>> +            char *url = i->data;
>>>   -            if(payload->destfile_name) {
>>> -                const char *filename =
>>> mbasename(payload->destfile_name);
>>> -                filepath = _alpm_filecache_find(handle, filename);
>>> -            } else {
>>> -                STRDUP(filepath, payload->tempfile_name,
>>> GOTO_ERR(handle, ALPM_ERR_MEMORY, err));
>>> -            }
>>> +            /* attempt again to find the file in our pkgcache */
>>> +            char *filepath = filecache_find_url(handle, url);
>>>               if(filepath) {
>>>                   alpm_list_append(fetched, filepath);
>>>               } else {
>>
>> Without testing since this needs to be rebased now; this looks broken.
>> filecache_find_url with fail for any url where destfile_name doesn't
>> match the file name in the url.  Notably, that includes the download
>> links on Arch's package pages:
>> https://archlinux.org/packages/core/x86_64/pacman/download/
>>
> Yes, with or without this patch, the current implementation doesn't work
> for this kind of url when using a fetch callback.
> The solution I see is to change the fetch callback signature by making
> it return the path of the downloaded file.
> Otherwise this patch works for standard mirror urls.
> .

While this patch does improve the current situation, it is a step
backwards in terms the full fix, which will need to pass the full
payload to the front-end so it can fill in the needed values.

On that basis, I will not be accepting this patch.

Given fetching a URL with XferCommand has not worked for a long time (or
ever?), this issue is not a blocker for 6.0.

Allan
Guillaume Benoit May 3, 2021, 11:28 a.m. UTC | #4
Le 02/05/2021 à 11:47, Allan McRae a écrit :
> On 2/5/21 12:33 am, Guillaume Benoit wrote:
>> Le 01/05/2021 à 05:53, Andrew Gregory a écrit :
>>> On 04/30/21 at 12:09pm, Guillaume Benoit wrote:
>>>> After download, alpm_fetch_pkgurl uses payload->destfile_name
>>>> and payload->tempfile_name in order to find the downloaded file path.
>>>> Those fields are not set if a custom fetch callback is defined.
>>>> Use filecache_find_url instead, like in the beginning of the function.
>>>>
>>>> ---
>>>>    lib/libalpm/dload.c | 13 ++++---------
>>>>    1 file changed, 4 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c
>>>> index 6f33451a..d45c7707 100644
>>>> --- a/lib/libalpm/dload.c
>>>> +++ b/lib/libalpm/dload.c
>>>> @@ -1007,16 +1007,11 @@ int SYMEXPORT alpm_fetch_pkgurl(alpm_handle_t
>>>> *handle, const alpm_list_t *urls,
>>>>                EVENT(handle, &event);
>>>>            }
>>>>    -        for(i = payloads; i; i = i->next) {
>>>> -            struct dload_payload *payload = i->data;
>>>> -            char *filepath;
>>>> +        for(i = urls; i; i = i->next) {
>>>> +            char *url = i->data;
>>>>    -            if(payload->destfile_name) {
>>>> -                const char *filename =
>>>> mbasename(payload->destfile_name);
>>>> -                filepath = _alpm_filecache_find(handle, filename);
>>>> -            } else {
>>>> -                STRDUP(filepath, payload->tempfile_name,
>>>> GOTO_ERR(handle, ALPM_ERR_MEMORY, err));
>>>> -            }
>>>> +            /* attempt again to find the file in our pkgcache */
>>>> +            char *filepath = filecache_find_url(handle, url);
>>>>                if(filepath) {
>>>>                    alpm_list_append(fetched, filepath);
>>>>                } else {
>>>
>>> Without testing since this needs to be rebased now; this looks broken.
>>> filecache_find_url with fail for any url where destfile_name doesn't
>>> match the file name in the url.  Notably, that includes the download
>>> links on Arch's package pages:
>>> https://archlinux.org/packages/core/x86_64/pacman/download/
>>>
>> Yes, with or without this patch, the current implementation doesn't work
>> for this kind of url when using a fetch callback.
>> The solution I see is to change the fetch callback signature by making
>> it return the path of the downloaded file.
>> Otherwise this patch works for standard mirror urls.
>> .
> 
> While this patch does improve the current situation, it is a step
> backwards in terms the full fix, which will need to pass the full
> payload to the front-end so it can fill in the needed values.
> 
> On that basis, I will not be accepting this patch.
> 
> Given fetching a URL with XferCommand has not worked for a long time (or
> ever?), this issue is not a blocker for 6.0.
> 
> Allan
> 
Sorry for the bad formatting of my previous message. I had the brilliant idea to
answer from my phone...
I understand the priority is that the internal curl functions work so,
with this point of view, this patch can be seen as a regression.
The point is, currently alpm_fetch_pkgurl is broken with a fetch callback defined,
whatever url is used.
If I have time to work on another version of a patch that pass the full payload to
the front-end, is there a chance that it could be included in pacman 6.0 ?
Allan McRae May 3, 2021, 11:30 p.m. UTC | #5
On 3/5/21 9:28 pm, Guillaume Benoit wrote:
> 
> If I have time to work on another version of a patch that pass the full
> payload to
> the front-end, is there a chance that it could be included in pacman 6.0 ?

There is a chance...

The things going against me accepting it for 6.0 are I have called a
freeze apart from already submitted patches and regressions.  This is
not a regression.  I guess there will also be an API change.

However, I am slow at reviewing the last patches I need to apply, so a
patch that appears soon and I judge as having minimal risk for the
internal download may be accepted.

Allan
Guillaume Benoit May 5, 2021, 1:54 p.m. UTC | #6
Le 04/05/2021 à 01:30, Allan McRae a écrit :
> On 3/5/21 9:28 pm, Guillaume Benoit wrote:
>>
>> If I have time to work on another version of a patch that pass the full
>> payload to
>> the front-end, is there a chance that it could be included in pacman 6.0 ?
> 
> There is a chance...
> 
> The things going against me accepting it for 6.0 are I have called a
> freeze apart from already submitted patches and regressions.  This is
> not a regression.  I guess there will also be an API change.
> 
> However, I am slow at reviewing the last patches I need to apply, so a
> patch that appears soon and I judge as having minimal risk for the
> internal download may be accepted.
> 
> Allan
> 

v2: I choosed to create another alpm_download_payload struct to only expose
required fields to the API, alpm_cb_fetch callback now has this struct as argument.
Those are the only API changes.
I also rewrote the download_with_xfercommand function in pacman code.
What is fixed:
- download from an url with a fetch callback for any front-end
- download from an standard url with pacman with a xfercommand
What is not fixed:
- download from an url, with pacman with an xfercommand, when this url doesn't
contain the filename like https://archlinux.org/packages/core/x86_64/pacman/download/

---
  lib/libalpm/alpm.h  |  30 ++++++-
  lib/libalpm/dload.c |  36 ++++----
  src/pacman/conf.c   | 202 ++++++++++++++++++++++++++++++++------------
  3 files changed, 193 insertions(+), 75 deletions(-)

diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h
index 101d686b..48c94052 100644
--- a/lib/libalpm/alpm.h
+++ b/lib/libalpm/alpm.h
@@ -1203,6 +1203,28 @@ typedef struct _alpm_download_event_completed_t {
  	int result;
  } alpm_download_event_completed_t;

+/** Infos on the file to download when using a fetch callback */
+typedef struct _alpm_dload_payload_t {
+	/** the path of the downloaded file, MUST BE SET by the front-end after a successful download */
+	char *destfile_name;
+	/** alpm provides either
+	 *  1) fileurl - full URL to the file
+	 *  2) pair of (servers, filepath), in this case front-end need to iterate over the
+	 *     server list and tries to download "$server/$filepath"
+	 */
+	char *fileurl;
+	char *filepath;
+	alpm_list_t *servers;
+	/** if the download must be forced even if an up to date file already exists */
+	int force;
+	/** if the download can be resumed from a already partial download */
+	int allow_resume;
+	/** specifies if an accompanion *.sig file need to be downloaded */
+	int download_signature;
+	/** *.sig file is optional */
+	int signature_optional;
+} alpm_dload_payload_t;
+
  /** Type of download progress callbacks.
   * @param ctx user-provided context
   * @param filename the name of the file being downloaded
@@ -1215,14 +1237,14 @@ typedef void (*alpm_cb_download)(void *ctx, const char *filename,

  /** A callback for downloading files
   * @param ctx user-provided context
- * @param url the URL of the file to be downloaded
+ * @param payload infos on the file to download
   * @param localpath the directory to which the file should be downloaded
- * @param force whether to force an update, even if the file is the same
   * @return 0 on success, 1 if the file exists and is identical, -1 on
   * error.
+ * On success, the destfile_name field of the payload MUST BE SET by the front-end
   */
-typedef int (*alpm_cb_fetch)(void *ctx, const char *url, const char *localpath,
-		int force);
+typedef int (*alpm_cb_fetch)(void *ctx, alpm_dload_payload_t *payload,
+		const char *localpath);

  /* End of libalpm_cb */
  /** @} */
diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c
index 8fa46bb2..66954182 100644
--- a/lib/libalpm/dload.c
+++ b/lib/libalpm/dload.c
@@ -882,30 +882,30 @@ int _alpm_download(alpm_handle_t *handle,
  		int updated = 0;
  		for(p = payloads; p; p = p->next) {
  			struct dload_payload *payload = p->data;
-			alpm_list_t *s;
-			int ret = -1;
+			int ret;
+			struct _alpm_dload_payload_t *alpm_payload = NULL;

-			if(payload->fileurl) {
-				ret = handle->fetchcb(handle->fetchcb_ctx, payload->fileurl, localpath, payload->force);
-			} else {
-				for(s = payload->servers; s && ret == -1; s = s->next) {
-					const char *server = s->data;
-					char *fileurl;
-
-					size_t len = strlen(server) + strlen(payload->filepath) + 2;
-					MALLOC(fileurl, len, RET_ERR(handle, ALPM_ERR_MEMORY, -1));
-					snprintf(fileurl, len, "%s/%s", server, payload->filepath);
+			CALLOC(alpm_payload, 1, sizeof(*alpm_payload), RET_ERR(handle, ALPM_ERR_MEMORY, -1));
+			alpm_payload->fileurl = payload->fileurl;
+			alpm_payload->filepath = payload->filepath;
+			alpm_payload->servers = payload->servers;
+			alpm_payload->allow_resume = payload->allow_resume;
+			alpm_payload->download_signature = payload->download_signature;
+			alpm_payload->signature_optional = payload->signature_optional;

-					ret = handle->fetchcb(handle->fetchcb_ctx, fileurl, localpath, payload->force);
-					free(fileurl);
-				}
-			}
+			ret = handle->fetchcb(handle->fetchcb_ctx, alpm_payload, localpath);

  			if(ret == -1 && !payload->errors_ok) {
  				RET_ERR(handle, ALPM_ERR_EXTERNAL_DOWNLOAD, -1);
-			} else if(ret == 0) {
-				updated = 1;
+			} else {
+				/* needed by alpm_fetch_pkgurl  */
+				payload->destfile_name = alpm_payload->destfile_name;
+				alpm_payload->destfile_name = NULL;
+				if(ret == 0) {
+					updated = 1;
+				}
  			}
+			FREE(alpm_payload);
  		}
  		return updated ? 0 : 1;
  	}
diff --git a/src/pacman/conf.c b/src/pacman/conf.c
index 12fee64c..445ca6fa 100644
--- a/src/pacman/conf.c
+++ b/src/pacman/conf.c
@@ -277,37 +277,12 @@ static int systemvp(const char *file, char *const argv[])
  	return ret;
  }

-/** External fetch callback */
-static int download_with_xfercommand(void *ctx, const char *url,
-		const char *localpath, int force)
+static int download_url_with_xfercommand(const char *url, const char *destfile)
  {
-	int ret = 0, retval;
-	int usepart = 0;
-	int cwdfd = -1;
-	struct stat st;
-	char *destfile, *tempfile, *filename;
  	const char **argv;
  	size_t i;
-
-	(void)ctx;
-
-	if(!config->xfercommand_argv) {
-		return -1;
-	}
-
-	filename = get_filename(url);
-	if(!filename) {
-		return -1;
-	}
-	destfile = get_destfile(localpath, filename);
-	tempfile = get_tempfile(localpath, filename);
-
-	if(force && stat(tempfile, &st) == 0) {
-		unlink(tempfile);
-	}
-	if(force && stat(destfile, &st) == 0) {
-		unlink(destfile);
-	}
+	int retval;
+	int ret = -1;

  	if((argv = calloc(config->xfercommand_argc + 1, sizeof(char*))) == NULL) {
  		size_t bytes = (config->xfercommand_argc + 1) * sizeof(char*);
@@ -316,35 +291,19 @@ static int download_with_xfercommand(void *ctx, const char *url,
  				   "malloc failure: could not allocate %zu bytes\n",
  					 bytes),
  				bytes);
-		goto cleanup;
+		goto end;
  	}

  	for(i = 0; i <= config->xfercommand_argc; i++) {
  		const char *val = config->xfercommand_argv[i];
  		if(val && strcmp(val, "%o") == 0) {
-			usepart = 1;
-			val = tempfile;
+			val = destfile;
  		} else if(val && strcmp(val, "%u") == 0) {
  			val = url;
  		}
  		argv[i] = val;
  	}

-	/* save the cwd so we can restore it later */
-	do {
-		cwdfd = open(".", O_RDONLY);
-	} while(cwdfd == -1 && errno == EINTR);
-	if(cwdfd < 0) {
-		pm_printf(ALPM_LOG_ERROR, _("could not get current working directory\n"));
-	}
-
-	/* cwd to the download directory */
-	if(chdir(localpath)) {
-		pm_printf(ALPM_LOG_WARNING, _("could not chdir to download directory %s\n"), localpath);
-		ret = -1;
-		goto cleanup;
-	}
-
  	if(config->logmask & ALPM_LOG_DEBUG) {
  		char *cmd = arg_to_string(config->xfercommand_argc, (char**)argv);
  		if(cmd) {
@@ -356,21 +315,159 @@ static int download_with_xfercommand(void *ctx, const char *url,

  	if(retval == -1) {
  		pm_printf(ALPM_LOG_WARNING, _("running XferCommand: fork failed!\n"));
-		ret = -1;
  	} else if(retval != 0) {
  		/* download failed */
  		pm_printf(ALPM_LOG_DEBUG, "XferCommand command returned non-zero status "
  				"code (%d)\n", retval);
-		ret = -1;
  	} else {
  		/* download was successful */
  		ret = 0;
-		if(usepart) {
-			if(rename(tempfile, destfile)) {
-				pm_printf(ALPM_LOG_ERROR, _("could not rename %s to %s (%s)\n"),
-						tempfile, destfile, strerror(errno));
+	}
+
+end:
+	free(argv);
+
+	return ret;
+}
+
+/** External fetch callback */
+static int download_with_xfercommand(void *ctx, alpm_dload_payload_t *payload,
+		const char *localpath)
+{
+	int ret = -1;
+	int cwdfd = -1;
+	struct stat st;
+	char *destfile, *tempfile, *filename;
+
+	(void)ctx;
+
+	if(!config->xfercommand_argv) {
+		return -1;
+	}
+
+	if(payload->fileurl) {
+		filename = get_filename(payload->fileurl);
+	} else {
+		filename = strdup(payload->filepath);
+	}
+	if(!filename) {
+		return -1;
+	}
+	destfile = get_destfile(localpath, filename);
+	tempfile = get_tempfile(localpath, filename);
+
+	if(payload->force && stat(tempfile, &st) == 0) {
+		unlink(tempfile);
+	}
+	if(payload->force && stat(destfile, &st) == 0) {
+		unlink(destfile);
+	}
+
+	/* save the cwd so we can restore it later */
+	do {
+		cwdfd = open(".", O_RDONLY);
+	} while(cwdfd == -1 && errno == EINTR);
+	if(cwdfd < 0) {
+		pm_printf(ALPM_LOG_ERROR, _("could not get current working directory\n"));
+	}
+
+	/* cwd to the download directory */
+	if(chdir(localpath)) {
+		pm_printf(ALPM_LOG_WARNING, _("could not chdir to download directory %s\n"), localpath);
+		goto cleanup;
+	}
+
+	if (payload->fileurl) {
+		ret = download_url_with_xfercommand(payload->fileurl, tempfile);
+		/* Let's check if client requested downloading accompanion *.sig file */
+		if(payload->download_signature) {
+			int sig_ret;
+			size_t sig_url_len;
+			size_t sig_dest_len;
+			char *sig_fileurl;
+			char *sig_destfile;
+
+			sig_url_len = strlen(payload->fileurl) + 5;
+			sig_fileurl = calloc(sig_url_len, sizeof(char));
+			snprintf(sig_fileurl, sig_url_len, "%s.sig", payload->fileurl);
+
+			/* Don't use a .part file for the signature */
+			sig_dest_len = strlen(destfile) + 5;
+			sig_destfile = calloc(sig_dest_len, sizeof(char));
+			snprintf(sig_destfile, sig_dest_len, "%s.sig", destfile);
+
+			if(payload->force && stat(sig_destfile, &st) == 0) {
+				unlink(sig_destfile);
+			}
+
+			sig_ret = download_url_with_xfercommand(sig_fileurl, sig_destfile);
+			if (sig_ret == -1 && !payload->signature_optional) {
  				ret = -1;
  			}
+			free(sig_fileurl);
+			free(sig_destfile);
+		}
+	} else if(payload->filepath) {
+		while(payload->servers) {
+			const char *server = payload->servers->data;
+			char *url;
+			size_t len;
+
+			len = strlen(server) + strlen(payload->filepath) + 2;
+			url = calloc(len, sizeof(char));
+			snprintf(url, len, "%s/%s", server, payload->filepath);
+
+			ret = download_url_with_xfercommand(url, tempfile);
+			free(url);
+
+			if (ret == -1) {
+				payload->servers = payload->servers->next;
+			} else {
+				/* Let's check if client requested downloading accompanion *.sig file */
+				if(payload->download_signature) {
+					int sig_ret;
+					size_t sig_url_len;
+					size_t sig_dest_len;
+					char *sig_fileurl;
+					char *sig_destfile;
+
+					/* Use url with current server */
+					sig_url_len = strlen(url) + 5;
+					sig_fileurl = calloc(sig_url_len, sizeof(char));
+					snprintf(sig_fileurl, sig_url_len, "%s.sig", url);
+
+					/* Don't use a .part file for the signature */
+					sig_dest_len = strlen(destfile) + 5;
+					sig_destfile = calloc(sig_dest_len, sizeof(char));
+					snprintf(sig_destfile, sig_dest_len, "%s.sig", destfile);
+
+					if(payload->force && stat(sig_destfile, &st) == 0) {
+						unlink(sig_destfile);
+					}
+
+					sig_ret = download_url_with_xfercommand(sig_fileurl, sig_destfile);
+					if (sig_ret == -1 && !payload->signature_optional) {
+						ret = -1;
+					}
+					free(sig_fileurl);
+					free(sig_destfile);
+				}
+				/* Break whatever the state of download signature is,
+				 * the package download succeed and we don't to download the signature from another mirror.
+				 * If the signature is required, it will failed later */
+				break;
+			}
+		}
+	}
+
+	if (ret == 0) {
+		if(rename(tempfile, destfile)) {
+			pm_printf(ALPM_LOG_ERROR, _("could not rename %s to %s (%s)\n"),
+					tempfile, destfile, strerror(errno));
+			ret = -1;
+		} else {
+			payload->destfile_name = destfile;
+			destfile = NULL;
  		}
  	}

@@ -390,7 +487,6 @@ cleanup:
  	}
  	free(destfile);
  	free(tempfile);
-	free(argv);

  	return ret;
  }
Allan McRae May 9, 2021, 1:18 p.m. UTC | #7
On 5/5/21 11:54 pm, Guillaume Benoit wrote:
> Le 04/05/2021 à 01:30, Allan McRae a écrit :
>> On 3/5/21 9:28 pm, Guillaume Benoit wrote:
>>>
>>> If I have time to work on another version of a patch that pass the full
>>> payload to
>>> the front-end, is there a chance that it could be included in pacman
>>> 6.0 ?
>>
>> There is a chance...
>>
>> The things going against me accepting it for 6.0 are I have called a
>> freeze apart from already submitted patches and regressions.  This is
>> not a regression.  I guess there will also be an API change.
>>
>> However, I am slow at reviewing the last patches I need to apply, so a
>> patch that appears soon and I judge as having minimal risk for the
>> internal download may be accepted.
>>
>> Allan
>>
> 
> v2: I choosed to create another alpm_download_payload struct to only expose
> required fields to the API, alpm_cb_fetch callback now has this struct
> as argument.
> Those are the only API changes.
> I also rewrote the download_with_xfercommand function in pacman code.
> What is fixed:
> - download from an url with a fetch callback for any front-end
> - download from an standard url with pacman with a xfercommand
> What is not fixed:
> - download from an url, with pacman with an xfercommand, when this url
> doesn't
> contain the filename like
> https://archlinux.org/packages/core/x86_64/pacman/download/
> 

Does this also replace:
libalpm: download signatures with a fetch callback

That was on my list next, but I took a quick skim at this patch and it
appears to supersede that patch.

Allan
Guillaume Benoit May 9, 2021, 5 p.m. UTC | #8
Le 09/05/2021 à 15:18, Allan McRae a écrit :
> On 5/5/21 11:54 pm, Guillaume Benoit wrote:
>> Le 04/05/2021 à 01:30, Allan McRae a écrit :
>>> On 3/5/21 9:28 pm, Guillaume Benoit wrote:
>>>>
>>>> If I have time to work on another version of a patch that pass the full
>>>> payload to
>>>> the front-end, is there a chance that it could be included in pacman
>>>> 6.0 ?
>>>
>>> There is a chance...
>>>
>>> The things going against me accepting it for 6.0 are I have called a
>>> freeze apart from already submitted patches and regressions.  This is
>>> not a regression.  I guess there will also be an API change.
>>>
>>> However, I am slow at reviewing the last patches I need to apply, so a
>>> patch that appears soon and I judge as having minimal risk for the
>>> internal download may be accepted.
>>>
>>> Allan
>>>
>>
>> v2: I choosed to create another alpm_download_payload struct to only expose
>> required fields to the API, alpm_cb_fetch callback now has this struct
>> as argument.
>> Those are the only API changes.
>> I also rewrote the download_with_xfercommand function in pacman code.
>> What is fixed:
>> - download from an url with a fetch callback for any front-end
>> - download from an standard url with pacman with a xfercommand
>> What is not fixed:
>> - download from an url, with pacman with an xfercommand, when this url
>> doesn't
>> contain the filename like
>> https://archlinux.org/packages/core/x86_64/pacman/download/
>>
> 
> Does this also replace:
> libalpm: download signatures with a fetch callback
> 
> That was on my list next, but I took a quick skim at this patch and it
> appears to supersede that patch.
> 
> Allan
> 
Yes, with this patch, signatures download is under the responsibility of the fetch callback,
checking payload->download_signature.
That's now implemented in pacman's download_with_xfercommand function.
Allan McRae May 18, 2021, 1:47 a.m. UTC | #9
On 5/5/21 11:54 pm, Guillaume Benoit wrote:
> v2: I choosed to create another alpm_download_payload struct to only expose
> required fields to the API, alpm_cb_fetch callback now has this struct
> as argument.
> Those are the only API changes.
> I also rewrote the download_with_xfercommand function in pacman code.
> What is fixed:
> - download from an url with a fetch callback for any front-end
> - download from an standard url with pacman with a xfercommand
> What is not fixed:
> - download from an url, with pacman with an xfercommand, when this url
> doesn't
> contain the filename like
> https://archlinux.org/packages/core/x86_64/pacman/download/
> 
> ---
>  lib/libalpm/alpm.h  |  30 ++++++-
>  lib/libalpm/dload.c |  36 ++++----
>  src/pacman/conf.c   | 202 ++++++++++++++++++++++++++++++++------------
>  3 files changed, 193 insertions(+), 75 deletions(-)

I have had a look at this patch.  Likely a step in the right direction,
but there is a decent amount of work to be done too.

I have decided this is not going to make the release.  The patch is not
ready so more changes will need made, and this will drag out a release
that is otherwise good to go. This is also a massive change after the
freeze was called, and the issue was present in at least pacman-5.2.
Given we had no bug reports in 18 months, so I don't consider it
critical, or even high priority.

I'll put this as one of the first things on my post-6.0 list.

Allan

Patch

diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c
index 6f33451a..d45c7707 100644
--- a/lib/libalpm/dload.c
+++ b/lib/libalpm/dload.c
@@ -1007,16 +1007,11 @@  int SYMEXPORT alpm_fetch_pkgurl(alpm_handle_t *handle, const alpm_list_t *urls,
  			EVENT(handle, &event);
  		}
  -		for(i = payloads; i; i = i->next) {
-			struct dload_payload *payload = i->data;
-			char *filepath;
+		for(i = urls; i; i = i->next) {
+			char *url = i->data;
  -			if(payload->destfile_name) {
-				const char *filename = mbasename(payload->destfile_name);
-				filepath = _alpm_filecache_find(handle, filename);
-			} else {
-				STRDUP(filepath, payload->tempfile_name, GOTO_ERR(handle, ALPM_ERR_MEMORY, err));
-			}
+			/* attempt again to find the file in our pkgcache */
+			char *filepath = filecache_find_url(handle, url);
  			if(filepath) {
  				alpm_list_append(fetched, filepath);
  			} else {
-- 
2.31.1