| Message ID | ada1ad50-78fb-e9ba-2cd9-703ffdac628b@manjaro.org |
|---|---|
| State | Under Review |
| Headers | show |
| Series | [pacman-dev] libalpm: fix alpm_fetch_pkgurl with a fetch callback | expand |
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/
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.
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
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 ?
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
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; }
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
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.
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
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