[pacman-dev] [PATCH v2] libalpm: download sig files with -U when missing

[Morgan Adamiec]

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 at 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 at Octavia ~git/pacman % rm /var/cache/pacman/pkg/*.sig
>>> morganamilo at Octavia ~git/pacman % sudo ./build/pacman -U https://mirrors.ims.nksc.lt/archlinux/extra/os/x86_64/xterm-363-1-x86_64.pkg.tar.zst
>>> 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.