Re: [PATCH] pakfire: Better errorhandling on downloads

[Michael Tremer <michael.tremer@ipfire.org>]

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

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.
> 
>> 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.