Re: [pacman-dev] [PATCH] libalpm: fix alpm_fetch_pkgurl with a fetch callback
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> Yes, with pacman 5, fetching a URL with XferCommand doesn't work.With pacman 6, it doesn't work with the current code but it works with this patch,except for the special URLs given by Andrew Gregory.I can try to provide a different patch that pass the full payload to the front-endbut it can't help making pacman 6 correctly fetching a URL with XferCommand when thefilename can be guessed from the URL.
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>>>
On 5/2/21 3:55 PM, Guillaume Benoit wrote: 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> Yes, with pacman 5, fetching a URL with XferCommand doesn't work.With pacman 6, it doesn't work with the current code but it works with this patch,except for the special URLs given by Andrew Gregory.I can try to provide a different patch that pass the full payload to the front-endbut it can't help making pacman 6 correctly fetching a URL with XferCommand when thefilename can be guessed from the URL.
Help me out here a bit, and repeat that? -- Eli Schwartz Bug Wrangler and Trusted User
On 05/02/21 at 09:55pm, Guillaume Benoit wrote: ...
Yes, with pacman 5, fetching a URL with XferCommand doesn't work.With pacman 6, it doesn't work with the current code but it works with this patch,except for the special URLs given by Andrew Gregory.I can try to provide a different patch that pass the full payload to the front-endbut it can't help making pacman 6 correctly fetching a URL with XferCommand when thefilename can be guessed from the URL.
To be clear, when I said this patch is broken, I meant it actively breaks downloads that work now. This code runs regardless of which downloader is used and the internal downloader handles those urls just fine.
participants (3)
-
Andrew Gregory
-
Eli Schwartz
-
Guillaume Benoit