From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michael Tremer To: development@lists.ipfire.org Subject: Re: [PATCH] pakfire: Better errorhandling on downloads Date: Tue, 01 Mar 2022 13:31:53 +0000 Message-ID: In-Reply-To: <20220223202130.6104-2-robin.roevens@disroot.org> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============4902399589836921161==" List-Id: --===============4902399589836921161== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hello, > On 23 Feb 2022, at 20:21, Robin Roevens wrote: >=20 > - 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=E2=80=99t that large, but it is easier to spot any = problems if changes are bundled into smaller logical sets. Does anyone want to test this? I am just a little bit more concerned about br= eaking the package management system, because then we can=E2=80=99t fix thing= s easily any more. Best, -Michael > --- > src/pakfire/lib/functions.pl | 84 +++++++++++++++++++++--------------- > 1 file changed, 49 insertions(+), 35 deletions(-) >=20 > 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 = # > +# Copyright (C) 2007-2022 IPFire Team = # > # = # > # 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 { >=20 > if ( $code eq "500" ) { > message("Giving up: There was no chance to get the file \"$getfile\" fro= m any available server.\nThere was an error on the way. Please fix it."); > - return 1; > + return 0; > } >=20 > if ($response->is_success) { > @@ -226,7 +226,7 @@ sub fetchfile { > } > logger("DOWNLOAD FINISHED: $file"); > $allok =3D 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 s= ome new servers."); > - return 1; > + return 0; > } >=20 > sub getmirrors { > @@ -256,9 +256,14 @@ sub getmirrors { > } >=20 > if ("$force" eq "force") { > - fetchfile("$Conf::version/lists/server-list.db", "$Conf::mainserver"); > - move("$Conf::cachedir/server-list.db", "$Conf::dbdir/lists/server-list.d= b"); > + 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; > } >=20 > sub getcoredb { > @@ -279,8 +284,9 @@ sub getcoredb { > } >=20 > 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"); > + } > } > } >=20 > @@ -318,15 +324,13 @@ sub selectmirror { >=20 > ### Check if there is a current server list and read it. > # If there is no list try to get one. > - my $count =3D 0; > - while (!(open(FILE, "<$Conf::dbdir/lists/server-list.db")) && ($count lt = 5)) { > - $count++; > - getmirrors("noforce"); > - } > - if ($count =3D=3D 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 =3D ; > close(FILE); >=20 > @@ -390,8 +394,13 @@ sub dbgetlist { > } >=20 > if ("$force" eq "force") { > - fetchfile("lists/packages_list.db", ""); > - move("$Conf::cachedir/packages_list.db", "$Conf::dbdir/lists/packages_li= st.db"); > + if (fetchfile("lists/packages_list.db", "")) { > + move("$Conf::cachedir/packages_list.db", "$Conf::dbdir/lists/packages_l= ist.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; > + } > } >=20 > # Update the meta database if new packages was in the package list > @@ -419,8 +428,7 @@ sub dbgetlist { > @templine =3D split(/\;/,$prog); > if (("$metadata{'Name'}" eq "$templine[0]") && ("$metadata{'Release'}" n= e "$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 { >=20 > sub resolvedeps_one { > my $pak =3D shift; > - > - getmetafile("$pak"); > - > +=09 > message("PAKFIRE RESV: $pak: Resolving dependencies..."); >=20 > + unless (getmetafile("$pak")) { > + message("PAKFIRE ERROR: Error retrieving dependency information on $pak.= Unable to resolve dependencies."); > + exit 1; > + }; > +=09 > my %metadata =3D parsemetafile("$Conf::dbdir/meta/meta-$pak"); > my @all; > my @deps =3D split(/ /, $metadata{'Dependencies'}); > @@ -629,14 +640,10 @@ sub cleanup { >=20 > sub getmetafile { > my $pak =3D 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", ""); > +=09 > + # 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"); > } >=20 > @@ -651,6 +658,7 @@ sub getmetafile { > print FILE $string; > } > close(FILE); > + > return 1; > } >=20 > @@ -713,8 +721,11 @@ sub getpak { > my $pak =3D shift; > my $force =3D shift; >=20 > - getmetafile("$pak"); > - > + unless (getmetafile("$pak")) { > + message("PAKFIRE ERROR: Unable to retrieve $pak metadata."); > + exit 1; > + } > +=09 > my %metadata =3D parsemetafile("$Conf::dbdir/meta/meta-$pak"); > my $file =3D $metadata{'File'}; >=20 > @@ -728,8 +739,11 @@ sub getpak { > return $file; > } > } > - > - fetchfile("paks/$file", ""); > +=09 > + unless (fetchfile("paks/$file", "")) { > + message("PAKFIRE ERROR: Unable to download $pak."); > + exit 1; > + } > return $file; > } >=20 > --=20 > 2.34.1 >=20 >=20 > --=20 > Dit bericht is gescanned op virussen en andere gevaarlijke > inhoud door MailScanner en lijkt schoon te zijn. >=20 --===============4902399589836921161==--