[pacman-dev] [PATCH] libalpm: download sig files with -U when missing
--- Also, I think the way signature downloading is a bit weird. You can't just download a signature. You have to say you want to download the package then the downloader will download the sig after the package finishes downloading. I think it would make more sense for signatures to be their own payloads and then have a dlsigcb. This would go towards fixing FS#67813 If totaldlcb reports 0 packages to download, then we can show the progress bars for the sigs instead of the packages. --- lib/libalpm/dload.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index df5e8be7..66ebeae9 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -863,8 +863,27 @@ int SYMEXPORT alpm_fetch_pkgurl(alpm_handle_t *handle, const alpm_list_t *urls, char *url = i->data; /* attempt to find the file in our pkgcache */ + char *filepath = filecache_find_url(handle, url); - if(filepath) { + int need_download = !filepath; + /* even if the package file in the cache we need to check for + * accompanion *.sig file as well. + * If *.sig is not cached then force download the package + its signature file. + */ + if(!need_download && (handle->siglevel & ALPM_SIG_PACKAGE)) { + char *sig_filename = NULL; + int len = strlen(filepath) + 5; + + MALLOC(sig_filename, len, RET_ERR(handle, ALPM_ERR_MEMORY, -1)); + snprintf(sig_filename, len, "%s.sig", filepath); + + need_download = !_alpm_filecache_exists(handle, sig_filename); + + FREE(sig_filename); + } + + + if(!need_download) { /* the file is locally cached so add it to the output right away */ alpm_list_append(fetched, filepath); } else { -- 2.30.0
Hi The commit description sounds like a duplicate of https://bugs.archlinux.org/task/33992 Isn't it fixed by commit f3dfba73d22b7eca3810a8114f2aab63da488b4c ? On Sun, Jan 10, 2021 at 12:22 PM morganamilo <morganamilo@archlinux.org> wrote:
---
Also, I think the way signature downloading is a bit weird. You can't just download a signature. You have to say you want to download the package then the downloader will download the sig after the package finishes downloading.
That's because the download URL can be redirected. And the final URL of the package is not known until the download starts. There is also a requirement that *.sig file should come from the same server as the package itself, i.e. *.sig file URL is unknown until the package download URL is resolved.
I think it would make more sense for signatures to be their own payloads and then have a dlsigcb.
This would go towards fixing FS#67813
If totaldlcb reports 0 packages to download, then we can show the progress bars for the sigs instead of the packages. --- lib/libalpm/dload.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-)
diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index df5e8be7..66ebeae9 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -863,8 +863,27 @@ int SYMEXPORT alpm_fetch_pkgurl(alpm_handle_t *handle, const alpm_list_t *urls, char *url = i->data;
/* attempt to find the file in our pkgcache */ + char *filepath = filecache_find_url(handle, url); - if(filepath) { + int need_download = !filepath; + /* even if the package file in the cache we need to check for + * accompanion *.sig file as well. + * If *.sig is not cached then force download the package + its signature file. + */ + if(!need_download && (handle->siglevel & ALPM_SIG_PACKAGE)) { + char *sig_filename = NULL; + int len = strlen(filepath) + 5; + + MALLOC(sig_filename, len, RET_ERR(handle, ALPM_ERR_MEMORY, -1)); + snprintf(sig_filename, len, "%s.sig", filepath); + + need_download = !_alpm_filecache_exists(handle, sig_filename); + + FREE(sig_filename); + } + + + if(!need_download) { /* the file is locally cached so add it to the output right away */ alpm_list_append(fetched, filepath); } else { -- 2.30.0
Hi On Sun, Jan 10, 2021 at 12:45 PM Anatol Pomozov <anatol.pomozov@gmail.com> wrote:
Hi
The commit description sounds like a duplicate of https://bugs.archlinux.org/task/33992
Isn't it fixed by commit f3dfba73d22b7eca3810a8114f2aab63da488b4c ?
Current master works for me: $ sudo rm /var/cache/pacman/pkg/zxing-cpp-1.1.1-1-x86_64.pkg.tar.zst.sig $ sudo pacman -U https://archlinux.org/packages/extra/x86_64/zxing-cpp/download/ ... warning: zxing-cpp-1.1.1-1 is up to date -- reinstalling ... $ ls -l /var/cache/pacman/pkg/zxing-cpp-1.1.1-1-x86_64.pkg.tar.zst.sig -rw-r--r-- 1 root root 310 Sep 12 13:12 /var/cache/pacman/pkg/zxing-cpp-1.1.1-1-x86_64.pkg.tar.zst.sig Could you please share a bit more information about the issue you are trying to fix?
On Sun, Jan 10, 2021 at 12:22 PM morganamilo <morganamilo@archlinux.org> wrote:
---
Also, I think the way signature downloading is a bit weird. You can't just download a signature. You have to say you want to download the package then the downloader will download the sig after the package finishes downloading.
That's because the download URL can be redirected. And the final URL of the package is not known until the download starts.
There is also a requirement that *.sig file should come from the same server as the package itself, i.e. *.sig file URL is unknown until the package download URL is resolved.
I think it would make more sense for signatures to be their own payloads and then have a dlsigcb.
This would go towards fixing FS#67813
If totaldlcb reports 0 packages to download, then we can show the progress bars for the sigs instead of the packages. --- lib/libalpm/dload.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-)
diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index df5e8be7..66ebeae9 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -863,8 +863,27 @@ int SYMEXPORT alpm_fetch_pkgurl(alpm_handle_t *handle, const alpm_list_t *urls, char *url = i->data;
/* attempt to find the file in our pkgcache */ + char *filepath = filecache_find_url(handle, url); - if(filepath) { + int need_download = !filepath; + /* even if the package file in the cache we need to check for + * accompanion *.sig file as well. + * If *.sig is not cached then force download the package + its signature file. + */ + if(!need_download && (handle->siglevel & ALPM_SIG_PACKAGE)) { + char *sig_filename = NULL; + int len = strlen(filepath) + 5; + + MALLOC(sig_filename, len, RET_ERR(handle, ALPM_ERR_MEMORY, -1)); + snprintf(sig_filename, len, "%s.sig", filepath); + + need_download = !_alpm_filecache_exists(handle, sig_filename); + + FREE(sig_filename); + } + + + if(!need_download) { /* the file is locally cached so add it to the output right away */ alpm_list_append(fetched, filepath); } else { -- 2.30.0
On 10/01/2021 20:59, Anatol Pomozov wrote:
Hi
On Sun, Jan 10, 2021 at 12:45 PM Anatol Pomozov <anatol.pomozov@gmail.com> wrote:
Hi
The commit description sounds like a duplicate of https://bugs.archlinux.org/task/33992
Isn't it fixed by commit f3dfba73d22b7eca3810a8114f2aab63da488b4c ?
Current master works for me:
$ sudo rm /var/cache/pacman/pkg/zxing-cpp-1.1.1-1-x86_64.pkg.tar.zst.sig $ sudo pacman -U https://archlinux.org/packages/extra/x86_64/zxing-cpp/download/ ... warning: zxing-cpp-1.1.1-1 is up to date -- reinstalling ... $ ls -l /var/cache/pacman/pkg/zxing-cpp-1.1.1-1-x86_64.pkg.tar.zst.sig -rw-r--r-- 1 root root 310 Sep 12 13:12 /var/cache/pacman/pkg/zxing-cpp-1.1.1-1-x86_64.pkg.tar.zst.sig
Could you please share a bit more information about the issue you are trying to fix?
Strange, doesn't work for me: morganamilo@Octavia (HEAD detached at origin/master) ~git/pacman % sudo ./build/pacman -U https://mirrors.ims.nksc.lt/archlinux/extra/os/x86_64/xterm-363-1-x86_64.pkg... warning: config file /etc/pacman.conf, line 23: directive 'PackageSigLevel' in section 'options' not recognised. :: Retrieving packages... xterm-363-1-x86_64 430.8 KiB 577 KiB/s 00:01 [--------------------------------------------------------------] 100% loading packages... warning: xterm-363-1 is up to date -- reinstalling resolving dependencies... looking for conflicting packages... Packages (1) xterm-363-1 Total Installed Size: 1.05 MiB Net Upgrade Size: 0.00 MiB :: Proceed with installation? [Y/n] (1/1) checking keys in keyring [--------------------------------------------------------------] 100% (1/1) checking package integrity [--------------------------------------------------------------] 100% (1/1) loading package files [--------------------------------------------------------------] 100% (1/1) checking for file conflicts [--------------------------------------------------------------] 100% (1/1) checking available disk space [--------------------------------------------------------------] 100% :: Processing package changes... (1/1) reinstalling xterm [--------------------------------------------------------------] 100% :: Running post-transaction hooks... (1/2) Arming ConditionNeedsUpdate... (2/2) Updating the desktop file MIME type cache... morganamilo@Octavia (HEAD detached at origin/master) ~git/pacman % sudo rm /var/cache/pacman/pkg/*.sig morganamilo@Octavia (HEAD detached at origin/master) ~git/pacman % sudo ./build/pacman -U https://mirrors.ims.nksc.lt/archlinux/extra/os/x86_64/xterm-363-1-x86_64.pkg... warning: config file /etc/pacman.conf, line 23: directive 'PackageSigLevel' in section 'options' not recognised. loading packages... error: '/var/cache/pacman/pkg/xterm-363-1-x86_64.pkg.tar.zst': package missing required signature morganamilo@Octavia (HEAD detached at origin/master) ~git/pacman %
On 1/10/21 4:34 PM, Morgan Adamiec wrote:
On 10/01/2021 20:59, Anatol Pomozov wrote:
Current master works for me:
$ sudo rm /var/cache/pacman/pkg/zxing-cpp-1.1.1-1-x86_64.pkg.tar.zst.sig $ sudo pacman -U https://archlinux.org/packages/extra/x86_64/zxing-cpp/download/ ... warning: zxing-cpp-1.1.1-1 is up to date -- reinstalling ... $ ls -l /var/cache/pacman/pkg/zxing-cpp-1.1.1-1-x86_64.pkg.tar.zst.sig -rw-r--r-- 1 root root 310 Sep 12 13:12 /var/cache/pacman/pkg/zxing-cpp-1.1.1-1-x86_64.pkg.tar.zst.sig
Could you please share a bit more information about the issue you are trying to fix?
Strange, doesn't work for me:
morganamilo@Octavia (HEAD detached at origin/master) ~git/pacman % sudo ./build/pacman -U https://mirrors.ims.nksc.lt/archlinux/extra/os/x86_64/xterm-363-1-x86_64.pkg...
So it works for me if I use a link with a redirection like Anatol: $ ./builddir/pacman -U $url but if I use curl -I $url and resolve the location: header with my fingers, $ ./builddir/pacman -U $resolved_url goes right to "loading packages..." and fails. OTOH this is nothing new. The same error is manifested in pacman 5.2.2, with slightly different quirks (e.g. $resolved_url derps hard, but $url is redownloaded every single time to e.g. alpmtmp.ePOeJp then overwrites the existing cached file while the .sig is properly detected in the cache). -- Eli Schwartz Bug Wrangler and Trusted User
On 10/01/2021 20:45, Anatol Pomozov wrote:
Hi
The commit description sounds like a duplicate of https://bugs.archlinux.org/task/33992
Isn't it fixed by commit f3dfba73d22b7eca3810a8114f2aab63da488b4c ?
I looked at the commit. That's the codepath for -S. -U is different and i basically copy pasted the code from that commit into the -U codepath.
When downloading a package with -U, alpm only checks if the package itself is in cache when deciding whether anything needs to be downloaded. So if for some reason the package is in cache but the signature file is not, there's be no attempt to download the signature and instead just throw an error. morganamilo@Octavia ~git/pacman % rm /var/cache/pacman/pkg/*.sig morganamilo@Octavia ~git/pacman % sudo ./build/pacman -U https://mirrors.ims.nksc.lt/archlinux/extra/os/x86_64/xterm-363-1-x86_64.pkg... loading packages... error: '/var/cache/pacman/pkg/xterm-363-1-x86_64.pkg.tar.zst': package missing required signature So let's just make sure to check that the package and sig file is there before downloading. Like how the -S codepath already does. --- Also, I think the way signature downloading is a bit weird. You can't just download a signature. You have to say you want to download the package then the downloader will download the sig after the package finishes downloading. I think it would make more sense for signatures to be their own payloads and then have a dlsigcb. This would go towards fixing FS#67813 If totaldlcb reports 0 packages to download, then we can show the progress bars for the sigs instead of the packages. --- lib/libalpm/dload.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index df5e8be7..66ebeae9 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -863,8 +863,27 @@ int SYMEXPORT alpm_fetch_pkgurl(alpm_handle_t *handle, const alpm_list_t *urls, char *url = i->data; /* attempt to find the file in our pkgcache */ + char *filepath = filecache_find_url(handle, url); - if(filepath) { + int need_download = !filepath; + /* even if the package file in the cache we need to check for + * accompanion *.sig file as well. + * If *.sig is not cached then force download the package + its signature file. + */ + if(!need_download && (handle->siglevel & ALPM_SIG_PACKAGE)) { + char *sig_filename = NULL; + int len = strlen(filepath) + 5; + + MALLOC(sig_filename, len, RET_ERR(handle, ALPM_ERR_MEMORY, -1)); + snprintf(sig_filename, len, "%s.sig", filepath); + + need_download = !_alpm_filecache_exists(handle, sig_filename); + + FREE(sig_filename); + } + + + if(!need_download) { /* the file is locally cached so add it to the output right away */ alpm_list_append(fetched, filepath); } else { -- 2.30.0
Hi On Mon, Jan 11, 2021 at 4:27 AM morganamilo <morganamilo@archlinux.org> wrote:
When downloading a package with -U, alpm only checks if the package itself is in cache when deciding whether anything needs to be downloaded. So if for some reason the package is in cache but the signature file is not, there's be no attempt to download the signature and instead just throw an error.
morganamilo@Octavia ~git/pacman % rm /var/cache/pacman/pkg/*.sig morganamilo@Octavia ~git/pacman % sudo ./build/pacman -U https://mirrors.ims.nksc.lt/archlinux/extra/os/x86_64/xterm-363-1-x86_64.pkg... loading packages... error: '/var/cache/pacman/pkg/xterm-363-1-x86_64.pkg.tar.zst': package missing required signature
So let's just make sure to check that the package and sig file is there before downloading. Like how the -S codepath already does.
Thanks for fixing the issue. The commit looks good to me. A few questions below.
Also, I think the way signature downloading is a bit weird. You can't just download a signature. You have to say you want to download the package then the downloader will download the sig after the package finishes downloading.
Is this question resolved so it can be removed from the commit message?
I think it would make more sense for signatures to be their own payloads and then have a dlsigcb.
This would go towards fixing FS#67813
Could you please add a reference to FS#33992?
If totaldlcb reports 0 packages to download, then we can show the progress bars for the sigs instead of the packages. --- lib/libalpm/dload.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-)
diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index df5e8be7..66ebeae9 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -863,8 +863,27 @@ int SYMEXPORT alpm_fetch_pkgurl(alpm_handle_t *handle, const alpm_list_t *urls, char *url = i->data;
/* attempt to find the file in our pkgcache */ + char *filepath = filecache_find_url(handle, url); - if(filepath) { + int need_download = !filepath; + /* even if the package file in the cache we need to check for + * accompanion *.sig file as well. + * If *.sig is not cached then force download the package + its signature file. + */ + if(!need_download && (handle->siglevel & ALPM_SIG_PACKAGE)) { + char *sig_filename = NULL; + int len = strlen(filepath) + 5; + + MALLOC(sig_filename, len, RET_ERR(handle, ALPM_ERR_MEMORY, -1)); + snprintf(sig_filename, len, "%s.sig", filepath); + + need_download = !_alpm_filecache_exists(handle, sig_filename); + + FREE(sig_filename); + } + + + if(!need_download) { /* the file is locally cached so add it to the output right away */ alpm_list_append(fetched, filepath); } else {
Is there a chance of a memory leak for 'filepath' here? What if 'url' is in the cache, but its signature is not. It looks like in this case we do not free 'filepath' anywhere.
On 11/01/2021 23:38, Anatol Pomozov wrote:
Hi
On Mon, Jan 11, 2021 at 4:27 AM morganamilo <morganamilo@archlinux.org> wrote:
When downloading a package with -U, alpm only checks if the package itself is in cache when deciding whether anything needs to be downloaded. So if for some reason the package is in cache but the signature file is not, there's be no attempt to download the signature and instead just throw an error.
morganamilo@Octavia ~git/pacman % rm /var/cache/pacman/pkg/*.sig morganamilo@Octavia ~git/pacman % sudo ./build/pacman -U https://mirrors.ims.nksc.lt/archlinux/extra/os/x86_64/xterm-363-1-x86_64.pkg... loading packages... error: '/var/cache/pacman/pkg/xterm-363-1-x86_64.pkg.tar.zst': package missing required signature
So let's just make sure to check that the package and sig file is there before downloading. Like how the -S codepath already does.
Thanks for fixing the issue. The commit looks good to me. A few questions below.
Also, I think the way signature downloading is a bit weird. You can't just download a signature. You have to say you want to download the package then the downloader will download the sig after the package finishes downloading.
Is this question resolved so it can be removed from the commit message?
It's still something I want to tackle. It's also not part of the commit message, just a comment. Everything after --- won't actually make it into the commit.
I think it would make more sense for signatures to be their own payloads and then have a dlsigcb.
This would go towards fixing FS#67813
Could you please add a reference to FS#33992?
Ah didn't know there was a bug about it. If I may be lazy and just ask Allan to add it to the commit message.
If totaldlcb reports 0 packages to download, then we can show the progress bars for the sigs instead of the packages. --- lib/libalpm/dload.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-)
diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index df5e8be7..66ebeae9 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -863,8 +863,27 @@ int SYMEXPORT alpm_fetch_pkgurl(alpm_handle_t *handle, const alpm_list_t *urls, char *url = i->data;
/* attempt to find the file in our pkgcache */ + char *filepath = filecache_find_url(handle, url); - if(filepath) { + int need_download = !filepath; + /* even if the package file in the cache we need to check for + * accompanion *.sig file as well. + * If *.sig is not cached then force download the package + its signature file. + */ + if(!need_download && (handle->siglevel & ALPM_SIG_PACKAGE)) { + char *sig_filename = NULL; + int len = strlen(filepath) + 5; + + MALLOC(sig_filename, len, RET_ERR(handle, ALPM_ERR_MEMORY, -1)); + snprintf(sig_filename, len, "%s.sig", filepath); + + need_download = !_alpm_filecache_exists(handle, sig_filename); + + FREE(sig_filename); + } + + + if(!need_download) { /* the file is locally cached so add it to the output right away */ alpm_list_append(fetched, filepath); } else {
Is there a chance of a memory leak for 'filepath' here? What if 'url' is in the cache, but its signature is not. It looks like in this case we do not free 'filepath' anywhere.
The function returns a list of filepaths for where each file was downloaded to. The caller then frees it.
On 12/01/2021 11:53, Morgan Adamiec wrote:
On 11/01/2021 23:38, Anatol Pomozov wrote:
Hi
On Mon, Jan 11, 2021 at 4:27 AM morganamilo <morganamilo@archlinux.org> wrote:
When downloading a package with -U, alpm only checks if the package itself is in cache when deciding whether anything needs to be downloaded. So if for some reason the package is in cache but the signature file is not, there's be no attempt to download the signature and instead just throw an error.
morganamilo@Octavia ~git/pacman % rm /var/cache/pacman/pkg/*.sig morganamilo@Octavia ~git/pacman % sudo ./build/pacman -U https://mirrors.ims.nksc.lt/archlinux/extra/os/x86_64/xterm-363-1-x86_64.pkg... loading packages... error: '/var/cache/pacman/pkg/xterm-363-1-x86_64.pkg.tar.zst': package missing required signature
So let's just make sure to check that the package and sig file is there before downloading. Like how the -S codepath already does.
Thanks for fixing the issue. The commit looks good to me. A few questions below.
Also, I think the way signature downloading is a bit weird. You can't just download a signature. You have to say you want to download the package then the downloader will download the sig after the package finishes downloading.
Is this question resolved so it can be removed from the commit message?
It's still something I want to tackle. It's also not part of the commit message, just a comment. Everything after --- won't actually make it into the commit.
I think it would make more sense for signatures to be their own payloads and then have a dlsigcb.
This would go towards fixing FS#67813
Could you please add a reference to FS#33992?
Ah didn't know there was a bug about it.
If I may be lazy and just ask Allan to add it to the commit message.
If totaldlcb reports 0 packages to download, then we can show the progress bars for the sigs instead of the packages. --- lib/libalpm/dload.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-)
diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index df5e8be7..66ebeae9 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -863,8 +863,27 @@ int SYMEXPORT alpm_fetch_pkgurl(alpm_handle_t *handle, const alpm_list_t *urls, char *url = i->data;
/* attempt to find the file in our pkgcache */ + char *filepath = filecache_find_url(handle, url); - if(filepath) { + int need_download = !filepath; + /* even if the package file in the cache we need to check for + * accompanion *.sig file as well. + * If *.sig is not cached then force download the package + its signature file. + */ + if(!need_download && (handle->siglevel & ALPM_SIG_PACKAGE)) { + char *sig_filename = NULL; + int len = strlen(filepath) + 5; + + MALLOC(sig_filename, len, RET_ERR(handle, ALPM_ERR_MEMORY, -1)); + snprintf(sig_filename, len, "%s.sig", filepath); + + need_download = !_alpm_filecache_exists(handle, sig_filename); + + FREE(sig_filename); + } + + + if(!need_download) { /* the file is locally cached so add it to the output right away */ alpm_list_append(fetched, filepath); } else {
Is there a chance of a memory leak for 'filepath' here? What if 'url' is in the cache, but its signature is not. It looks like in this case we do not free 'filepath' anywhere.
The function returns a list of filepaths for where each file was downloaded to. The caller then frees it.
Actually, looking at it, filepath is not used in the else clause so it should probably be freed there.
participants (4)
-
Anatol Pomozov
-
Eli Schwartz
-
Morgan Adamiec
-
morganamilo