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: Fri, 04 Mar 2022 11:18:45 +0000 Message-ID: In-Reply-To: <7b55cb89dd4c4380d4ea5d6424e2d907246b5c69.camel@sicho.home> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============2740498195249939949==" List-Id: --===============2740498195249939949== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit Hello, > On 3 Mar 2022, at 21:20, Robin Roevens wrote: > > Hi > > Michael Tremer schreef op di 01-03-2022 om 13:31 [+0000]: >> Hello, >> >>> On 23 Feb 2022, at 20:21, Robin Roevens >>> 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 >>> # >>> +# 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 { >>> >>> 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 = ; >>> 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. --===============2740498195249939949==--