Re: [PATCH] pakfire: Better errorhandling on downloads

[Adolf Belka <adolf.belka@ipfire.org>]

[-- Attachment #1: Type: text/plain, Size: 11576 bytes --]

Hi All,

On 04/03/2022 12:18, Michael Tremer wrote:
> Hello,
>
>> On 3 Mar 2022, at 21:20, Robin Roevens <robin.roevens(a)disroot.org> wrote:
>>
>> Hi
>>
>> Michael Tremer schreef op di 01-03-2022 om 13:31 [+0000]:
>>> Hello,
>>>
>>>> On 23 Feb 2022, at 20:21, Robin Roevens <robin.roevens(a)disroot.org>
>>>> wrote:
>>>>
>>>> - Add true/false return codes to fetchfile, getmetafile and
>>>> getmirrors
>>>>   indicating succes or failure.
>>>> - Check on those return codes and fail gracefully with clean
>>>>   error message(s) when downloads fail.
>>>> - Replace duplicate meta-file fetching code in dbgetlist with
>>>>   getmetafile function (fixing possibly missed cariage return
>>>>   conversion in meta-files).
>>>> - Remove pointless 5 retries to download server-list.db in
>>>>   selectmirror as fetchfile already retries 5 times.
>>> It would have been useful to have these four things in four different
>>> patches, because that makes reviewing things a lot easier.
>>>
>>> Altogether, the patch isn’t that large, but it is easier to spot any
>>> problems if changes are bundled into smaller logical sets.
>> I understand, but I considered this (or at least the first two items)
>> already as a single logical set. But you are right I could have split
>> the patch in 4. Should I resubmit it as a v2 patchset of 4 ?
> No, I don’t want to create any extra work for you now.
>
> I just wanted to note this for future reference.
>
> -Michael
>
>> Robin
>>
>>> Does anyone want to test this? I am just a little bit more concerned
>>> about breaking the package management system, because then we can’t
>>> fix things easily any more.
I have run a test with this patch. I set up a CU 165 vm and ran pakfire 
with and without internet access to try and download 4 different addons.
I then patched the pakfire functions.pl file and redid the test.

In both cases with internet access the four addons were successfully 
downloaded.

In both cases with no internet access I got the same error message that 
I needed Internet access to install addons.

 From the point of view of a pakfire user there is no difference between 
with and without the patch.

One thing I did notice with the current CU 165 is that after installing 
the selected addons the wui goes back to the original pakfire screen and 
does not show the addons having been installed in the right hand box. If 
I select the pakfire menu item again then the additional addons are 
shown. It looks like the wui page is not being updated after the addons 
have been installed. Should I raise this as a bug or is this an interim 
status as CU 165 is still a work in progress.

Regards,
Adolf.
>>> Best,
>>> -Michael
>>>
>>>> ---
>>>> src/pakfire/lib/functions.pl | 84 +++++++++++++++++++++------------
>>>> ---
>>>> 1 file changed, 49 insertions(+), 35 deletions(-)
>>>>
>>>> diff --git a/src/pakfire/lib/functions.pl
>>>> b/src/pakfire/lib/functions.pl
>>>> index d4e338f23..24c55fd4a 100644
>>>> --- a/src/pakfire/lib/functions.pl
>>>> +++ b/src/pakfire/lib/functions.pl
>>>> @@ -2,7 +2,7 @@
>>>> ###################################################################
>>>> ############
>>>> #
>>>>             #
>>>> # IPFire.org - A linux based
>>>> firewall                                         #
>>>> -# Copyright (C) 2007-2021   IPFire Team
>>>> <info(a)ipfire.org>                   #
>>>> +# Copyright (C) 2007-2022   IPFire Team
>>>> <info(a)ipfire.org>                   #
>>>> #
>>>>             #
>>>> # This program is free software: you can redistribute it and/or
>>>> modify        #
>>>> # it under the terms of the GNU General Public License as published
>>>> by        #
>>>> @@ -206,7 +206,7 @@ sub fetchfile {
>>>>
>>>>                  if ( $code eq "500" ) {
>>>>                          message("Giving up: There was no chance to
>>>> get the file \"$getfile\" from any available server.\nThere was an
>>>> error on the way. Please fix it.");
>>>> -                       return 1;
>>>> +                       return 0;
>>>>                  }
>>>>
>>>>                  if ($response->is_success) {
>>>> @@ -226,7 +226,7 @@ sub fetchfile {
>>>>                                  }
>>>>                                  logger("DOWNLOAD FINISHED: $file");
>>>>                                  $allok = 1;
>>>> -                               return 0;
>>>> +                               return 1;
>>>>                          } else {
>>>>                                  logger("DOWNLOAD ERROR: Could not
>>>> open $Conf::tmpdir/$bfile for writing.");
>>>>                          }
>>>> @@ -235,7 +235,7 @@ sub fetchfile {
>>>>                  }
>>>>          }
>>>>          message("DOWNLOAD ERROR: There was no chance to get the
>>>> file \"$getfile\" from any available server.\nMay be you should run
>>>> \"pakfire update\" to get some new servers.");
>>>> -       return 1;
>>>> +       return 0;
>>>> }
>>>>
>>>> sub getmirrors {
>>>> @@ -256,9 +256,14 @@ sub getmirrors {
>>>>          }
>>>>
>>>>          if ("$force" eq "force") {
>>>> -               fetchfile("$Conf::version/lists/server-list.db",
>>>> "$Conf::mainserver");
>>>> -               move("$Conf::cachedir/server-list.db",
>>>> "$Conf::dbdir/lists/server-list.db");
>>>> +               if (fetchfile("$Conf::version/lists/server-
>>>> list.db", "$Conf::mainserver")) {
>>>> +                       move("$Conf::cachedir/server-list.db",
>>>> "$Conf::dbdir/lists/server-list.db");
>>>> +               } elsif (! -e "$Conf::dbdir/lists/server-list.db" )
>>>> {
>>>> +                       # if we end up with no server-list at all,
>>>> return failure
>>>> +                       return 0;
>>>> +               }
>>>>          }
>>>> +       return 1;
>>>> }
>>>>
>>>> sub getcoredb {
>>>> @@ -279,8 +284,9 @@ sub getcoredb {
>>>>          }
>>>>
>>>>          if ("$force" eq "force") {
>>>> -               fetchfile("lists/core-list.db", "");
>>>> -               move("$Conf::cachedir/core-list.db",
>>>> "$Conf::dbdir/lists/core-list.db");
>>>> +               if (fetchfile("lists/core-list.db", "")) {
>>>> +                       move("$Conf::cachedir/core-list.db",
>>>> "$Conf::dbdir/lists/core-list.db");
>>>> +               }
>>>>          }
>>>> }
>>>>
>>>> @@ -318,15 +324,13 @@ sub selectmirror {
>>>>
>>>>          ### Check if there is a current server list and read it.
>>>>          #   If there is no list try to get one.
>>>> -       my $count = 0;
>>>> -       while (!(open(FILE, "<$Conf::dbdir/lists/server-list.db"))
>>>> && ($count lt 5)) {
>>>> -               $count++;
>>>> -               getmirrors("noforce");
>>>> -       }
>>>> -       if ($count == 5) {
>>>> -               message("MIRROR ERROR: Could not find or download a
>>>> server list");
>>>> -               exit 1;
>>>> +       unless (open(FILE, "<$Conf::dbdir/lists/server-list.db")) {
>>>> +               unless (getmirrors("noforce")) {
>>>> +                       message("MIRROR ERROR: Could not find or
>>>> download a server list");
>>>> +                       exit 1;
>>>> +               }
>>>>          }
>>>> +
>>>>          my @lines = <FILE>;
>>>>          close(FILE);
>>>>
>>>> @@ -390,8 +394,13 @@ sub dbgetlist {
>>>>          }
>>>>
>>>>          if ("$force" eq "force") {
>>>> -               fetchfile("lists/packages_list.db", "");
>>>> -               move("$Conf::cachedir/packages_list.db",
>>>> "$Conf::dbdir/lists/packages_list.db");
>>>> +               if (fetchfile("lists/packages_list.db", "")) {
>>>> +                       move("$Conf::cachedir/packages_list.db",
>>>> "$Conf::dbdir/lists/packages_list.db");
>>>> +               } elsif ( -e "$Conf::dbdir/lists/packages_list.db"
>>>> ) {
>>>> +                       # If we end up with no db file after
>>>> download error there
>>>> +                       # is nothing more we can do here.
>>>> +                       return 0;
>>>> +               }
>>>>          }
>>>>
>>>>          # Update the meta database if new packages was in the
>>>> package list
>>>> @@ -419,8 +428,7 @@ sub dbgetlist {
>>>>                          @templine = split(/\;/,$prog);
>>>>                          if (("$metadata{'Name'}" eq "$templine[0]")
>>>> && ("$metadata{'Release'}" ne "$templine[2]")) {
>>>>                                  move("$Conf::dbdir/meta/meta-
>>>> $metadata{'Name'}","$Conf::dbdir/meta/old_meta-$metadata{'Name'}");
>>>> -                               fetchfile("meta/meta-
>>>> $metadata{'Name'}", "");
>>>> -                               move("$Conf::cachedir/meta-
>>>> $metadata{'Name'}", "$Conf::dbdir/meta/meta-$metadata{'Name'}");
>>>> +                               getmetafile($metadata{'Name'});
>>>>                          }
>>>>                  }
>>>>          }
>>>> @@ -532,11 +540,14 @@ sub dblist {
>>>>
>>>> sub resolvedeps_one {
>>>>          my $pak = shift;
>>>> -
>>>> -       getmetafile("$pak");
>>>> -
>>>> +
>>>>          message("PAKFIRE RESV: $pak: Resolving dependencies...");
>>>>
>>>> +       unless (getmetafile("$pak")) {
>>>> +               message("PAKFIRE ERROR: Error retrieving dependency
>>>> information on $pak. Unable to resolve dependencies.");
>>>> +               exit 1;
>>>> +       };
>>>> +
>>>>          my %metadata = parsemetafile("$Conf::dbdir/meta/meta-
>>>> $pak");
>>>>          my @all;
>>>>          my @deps = split(/ /, $metadata{'Dependencies'});
>>>> @@ -629,14 +640,10 @@ sub cleanup {
>>>>
>>>> sub getmetafile {
>>>>          my $pak = shift;
>>>> -
>>>> -       unless ( -e "$Conf::dbdir/meta/meta-$pak" ) {
>>>> -               fetchfile("meta/meta-$pak", "");
>>>> -               move("$Conf::cachedir/meta-$pak",
>>>> "$Conf::dbdir/meta/meta-$pak");
>>>> -       }
>>>> -
>>>> -       if ( -z "$Conf::dbdir/meta/meta-$pak" ) {
>>>> -               fetchfile("meta/meta-$pak", "");
>>>> +
>>>> +       # Try to download meta-file if we don't have one yet, or it
>>>> is empty for some reason
>>>> +       if ((! -e "$Conf::dbdir/meta/meta-$pak" ) || ( -z
>>>> "$Conf::dbdir/meta/meta-$pak" )) {
>>>> +               return 0 unless (fetchfile("meta/meta-$pak", ""));
>>>>                  move("$Conf::cachedir/meta-$pak",
>>>> "$Conf::dbdir/meta/meta-$pak");
>>>>          }
>>>>
>>>> @@ -651,6 +658,7 @@ sub getmetafile {
>>>>                  print FILE $string;
>>>>          }
>>>>          close(FILE);
>>>> +
>>>>          return 1;
>>>> }
>>>>
>>>> @@ -713,8 +721,11 @@ sub getpak {
>>>>          my $pak = shift;
>>>>          my $force = shift;
>>>>
>>>> -       getmetafile("$pak");
>>>> -
>>>> +       unless (getmetafile("$pak")) {
>>>> +               message("PAKFIRE ERROR: Unable to retrieve $pak
>>>> metadata.");
>>>> +               exit 1;
>>>> +       }
>>>> +
>>>>          my %metadata = parsemetafile("$Conf::dbdir/meta/meta-
>>>> $pak");
>>>>          my $file = $metadata{'File'};
>>>>
>>>> @@ -728,8 +739,11 @@ sub getpak {
>>>>                          return $file;
>>>>                  }
>>>>          }
>>>> -
>>>> -       fetchfile("paks/$file", "");
>>>> +
>>>> +       unless (fetchfile("paks/$file", "")) {
>>>> +               message("PAKFIRE ERROR: Unable to download $pak.");
>>>> +               exit 1;
>>>> +       }
>>>>          return $file;
>>>> }
>>>>
>>>> -- 
>>>> 2.34.1
>>>>
>>>>
>>>> -- 
>>>> Dit bericht is gescanned op virussen en andere gevaarlijke
>>>> inhoud door MailScanner en lijkt schoon te zijn.
>>>>
>>>
>> -- 
>> Dit bericht is gescanned op virussen en andere gevaarlijke
>> inhoud door MailScanner en lijkt schoon te zijn.

-- 
Sent from my laptop