From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michael Tremer To: development@lists.ipfire.org Subject: Re: [PATCH 1/9] pakfire: Refactor dblist seperating UI and logic Date: Mon, 21 Mar 2022 16:18:43 +0000 Message-ID: In-Reply-To: <20220309225655.4472-2-robin.roevens@disroot.org> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============5325901878139201024==" List-Id: --===============5325901878139201024== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hello Robin, Thank you for looking into Pakfire and making this thing more robust. I have been meaning to give you a good review on this, but it is a lot of cod= e with a lot of changes, which cannot introduce any regressions, because if s= omething breaks we cannot fix it in an update... > On 9 Mar 2022, at 22:56, Robin Roevens wrote: >=20 > - Removed UI code from dblist function and refactor it making it return > a hash representing the pak db for easier handling of this data. Yes. This is a good idea. > - Moved core update check in dblist to new seperate dbcoreinfo function > making it return a hash with current and possibly available core > version info. > - Update existing calls to dblist > - Bring UI parts previously in dblist to pakfire program itself, > pakfire.cgi and index.cgi with a few small enhancements: > - Add currently installed version numbers to installed paks list in > pakfire.cgi > - Add 'Installed: yes/no' to pakfire list output so people not using > colors have this information too. > - Add update available details to pakfire list output if package has > updates available. Again, this would have been easier to review if it was split into smaller cha= nges. >=20 > Signed-off-by: Robin Roevens > --- > html/cgi-bin/index.cgi | 6 +- > html/cgi-bin/pakfire.cgi | 24 +++++- > src/pakfire/lib/functions.pl | 147 ++++++++++++++++------------------- > src/pakfire/pakfire | 99 +++++++++++++++++------ > 4 files changed, 168 insertions(+), 108 deletions(-) >=20 > diff --git a/html/cgi-bin/index.cgi b/html/cgi-bin/index.cgi > index 18c26942e..6fecae1ff 100644 > --- a/html/cgi-bin/index.cgi > +++ b/html/cgi-bin/index.cgi > @@ -604,7 +604,11 @@ if ($warnmessage) { > &Header::closebox(); > } >=20 > -&Pakfire::dblist("upgrade", "notice"); > +my %coredb =3D &Pakfire::coredbinfo(); > +if (defined $coredb{'AvailableRelease'}) { > + print "


$Lang::tr{'core notice 1= '} $coredb{'Release'} $Lang::tr{'core notice 2'} $coredb{'AvailableRelease'} = $Lang::tr{'core notice 3'}"; > +} > + > if ( -e "/var/run/need_reboot" ) { > print "
"; > print "

$Lang::tr{'needreboot'}!"; > diff --git a/html/cgi-bin/pakfire.cgi b/html/cgi-bin/pakfire.cgi > index 65c67fb90..30a23128b 100644 > --- a/html/cgi-bin/pakfire.cgi > +++ b/html/cgi-bin/pakfire.cgi > @@ -370,7 +370,17 @@ print < is a very stupid idea. I have no idea why I ever pi= cked that - presumably because there was the intention that users can pick wh= at they want to install and what not. We should move away from this I believe. > + > + > print < > > @@ -386,7 +396,11 @@ END > > > @@ -398,7 +412,11 @@ END > > > diff --git a/src/pakfire/lib/functions.pl b/src/pakfire/lib/functions.pl > index d4e338f23..f08f43622 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 = # > @@ -44,7 +44,7 @@ my @VALID_KEY_FINGERPRINTS =3D ( > ); >=20 > # A small color-hash :D > -my %color; > +our %color; > $color{'normal'} =3D "\033[0m"; > $color{'black'} =3D "\033[0;30m"; > $color{'darkgrey'} =3D "\033[1;30m"; > @@ -426,108 +426,93 @@ sub dbgetlist { > } > } >=20 > +sub coredbinfo { > + ### This subroutine returns core db version information in a hash. > + # Usage is without arguments > + > + eval(`grep "core_" $Conf::dbdir/lists/core-list.db`); I know the eval() statement has been used in the past, but since we touched t= his code now, can we remove this and just parse the line that we are looking = for? I consider this a security problem as it allows code injection. > + > + my %coredb =3D ( > + CoreVersion =3D> $Conf::version, > + Release =3D> $Conf::core_mine, > + ); > + > + $coredb{'AvailableRelease'} =3D $core_release if ("$Conf::core_mine" < "$= core_release"); > + > + return %coredb; > +} > + > sub dblist { > - ### This subroutine lists the packages. > - # You may also pass a filter: &Pakfire::dblist(filter) > - # Usage is always with two arguments. > - # filter may be: all, notinstalled, installed > + ### This subroutine returns the packages from the packages_list db in a h= ash. > + # It uses the currently cached version of packages_list. To ensure late= st=20 > + # data, run Pakfire::dbgetlist first. > + # You may also pass a filter: &Pakfire::dblist(filter)=20 > + # Usage is always with one argument. > + # filter may be: all, notinstalled, installed, upgrade > my $filter =3D shift; > - my $forweb =3D shift; > - my @updatepaks; > + my %paklist =3D (); > my $file; > my $line; > - my $prog; > my %metadata; > my @templine; > - > - ### Make sure that the list is not outdated. > - #dbgetlist("noforce"); > - > +=09 > open(FILE, "<$Conf::dbdir/lists/packages_list.db"); > my @db =3D ; > close(FILE); >=20 > - if ("$filter" eq "upgrade") { > - if ("$forweb" ne "forweb" && "$forweb" ne "notice" ) {getcoredb("noforce= ");} > - eval(`grep "core_" $Conf::dbdir/lists/core-list.db`); > - if ("$core_release" > "$Conf::core_mine") { > - if ("$forweb" eq "forweb") { > - print "\n"; > - } > - elsif ("$forweb" eq "notice") { > - print "


$Lang::tr{'core notic= e 1'} $Conf::core_mine $Lang::tr{'core notice 2'} $core_release $Lang::tr{'co= re notice 3'}"; > - } else { > - my $command =3D "Core-Update $Conf::version\nRelease: $Conf::core_mine= -> $core_release\n"; > - if ("$Pakfire::enable_colors" eq "1") { > - print "$color{'lila'}$command$color{'normal'}\n"; > - } else { > - print "$command\n"; > - } > - } > - } > - > + if ("$filter" ne "notinstalled") { > opendir(DIR,"$Conf::dbdir/installed"); > my @files =3D readdir(DIR); > closedir(DIR); > + > foreach $file (@files) { > next if ( $file eq "." ); > next if ( $file eq ".." ); > next if ( $file =3D~ /^old/ ); > %metadata =3D parsemetafile("$Conf::dbdir/installed/$file"); >=20 > - foreach $prog (@db) { > - @templine =3D split(/\;/,$prog); > - if (("$metadata{'Name'}" eq "$templine[0]") && ("$metadata{'Release'}"= < "$templine[2]" && "$forweb" ne "notice")) { > - push(@updatepaks,$metadata{'Name'}); > - if ("$forweb" eq "forweb") { > - print "\n"; > - } else { > - my $command =3D "Update: $metadata{'Name'}\nVersion: $metadata{'Prog= Version'} -> $templine[1]\nRelease: $metadata{'Release'} -> $templine[2]\n"; > - if ("$Pakfire::enable_colors" eq "1") { > - print "$color{'lila'}$command$color{'normal'}\n"; > - } else { > - print "$command\n"; > - } > - } > + foreach $line (@db) { > + next unless ($line =3D~ /.*;.*;.*;/ ); > + @templine =3D split(/\;/,$line); > + if (("$metadata{'Name'}" eq "$templine[0]") && ("$metadata{'Release'}"= < "$templine[2]")) { #&& "$forweb" ne "notice")) { > + # Add all upgradable paks to list > + $paklist{"$metadata{'Name'}"} =3D { > + ProgVersion =3D> $metadata{'ProgVersion'}, > + Release =3D> $metadata{'Release'}, > + AvailableProgVersion =3D> $templine[1], > + AvailableRelease =3D> $templine[2], > + Installed =3D> "yes" > + }; > + last; > + } elsif (("$metadata{'Name'}" eq "$templine[0]") && ("$filter" ne "upg= rade")) { > + # Add installed paks without an upgrade available to list > + $paklist{"$metadata{'Name'}"} =3D { > + ProgVersion =3D> $metadata{'ProgVersion'}, > + Release =3D> $metadata{'Release'}, > + Installed =3D> "yes" > + }; > + last; > } > } > } > - return @updatepaks; > - } else { > - my $line; > - my $use_color; > - my @templine; > - my $count; > - foreach $line (sort @db) { > + } > + > + # Add all not installed paks to list > + if (("$filter" ne "upgrade") && ("$filter" ne "installed")) { > + foreach $line (@db) { > next unless ($line =3D~ /.*;.*;.*;/ ); > - $use_color =3D ""; > @templine =3D split(/\;/,$line); > - if ("$filter" eq "notinstalled") { > - next if ( -e "$Conf::dbdir/installed/meta-$templine[0]" ); > - } elsif ("$filter" eq "installed") { > - next unless ( -e "$Conf::dbdir/installed/meta-$templine[0]" ); > - } > - $count++; > - if ("$forweb" eq "forweb") > - { > - if ("$filter" eq "notinstalled") { > - print "\n"; > - } else { > - print "\n"; > - } > - } else { > - if ("$Pakfire::enable_colors" eq "1") { > - if (&isinstalled("$templine[0]")) { > - $use_color =3D "$color{'red'}" > - } else { > - $use_color =3D "$color{'green'}" > - } > - } > - print "${use_color}Name: $templine[0]\nProgVersion: $templine[1]\nRele= ase: $templine[2]$color{'normal'}\n\n"; > - } > + next if ((defined $paklist{"$templine[0]"}) || (&isinstalled($templine[= 0]) =3D=3D 0)); > + > + $paklist{"$templine[0]"} =3D { > + ProgVersion =3D> "$templine[1]", > + Release =3D> "$templine[2]", > + Installed =3D> "no" > + }; > } > - print "$count packages total.\n" unless ("$forweb" eq "forweb"); > } > + > + return %paklist; > } This all looks good to me. > sub resolvedeps_one { > @@ -896,10 +881,10 @@ sub progress_bar { >=20 > sub updates_available { > # Get packets with updates available > - my @upgradepaks =3D &Pakfire::dblist("upgrade", "noweb"); > + my %upgradepaks =3D &Pakfire::dblist("upgrade"); >=20 > - # Get the length of the returned array > - my $updatecount =3D scalar @upgradepaks; > + # Get the length of the returned hash > + my $updatecount =3D keys %upgradepaks; >=20 > return "$updatecount"; > } > diff --git a/src/pakfire/pakfire b/src/pakfire/pakfire > index 6c77695c8..b4930e85d 100644 > --- a/src/pakfire/pakfire > +++ b/src/pakfire/pakfire > @@ -270,14 +270,25 @@ > &Pakfire::getcoredb("$force"); >=20 > } elsif ("$ARGV[0]" eq "upgrade") { > + my $use_color =3D ""; > + my $reset_color =3D ""; > + > + if ("$Pakfire::enable_colors" eq "1") { > + $reset_color =3D "$Pakfire::color{'normal'}"; > + $use_color =3D "$Pakfire::color{'lightpurple'}"; > + } > + > &Pakfire::upgradecore(); > - my @upgradepaks =3D &Pakfire::dblist("upgrade", "noweb"); > + =09 > my @deps =3D (); > - > - if (@upgradepaks) { > + if (my %upgradepaks =3D &Pakfire::dblist("upgrade")) { > # Resolve the dependencies of the to be upgraded packages > - @deps =3D &Pakfire::resolvedeps_recursive(@upgradepaks); > + @deps =3D &Pakfire::resolvedeps_recursive(keys %upgradepaks); >=20 > + foreach $pak (sort keys %upgradepaks) { > + print "${use_color}Update: $pak\nVersion: $upgradepaks{$pak}{'ProgVers= ion'} -> $upgradepaks{$pak}{'AvailableProgVersion'}\n"; > + print "Release: $upgradepaks{$pak}{'Release'} -> $upgradepaks{$pak}{'A= vailableRelease'}${reset_color}\n"; > + } > &Pakfire::message(""); > &Pakfire::message("PAKFIRE UPGR: We are going to install all packages li= sted above."); > if ($interactive) { > @@ -290,36 +301,78 @@ > exit 1; > } > } > - } > + =09 > + # Download packages > + foreach $pak (sort keys %upgradepaks) { > + &Pakfire::getpak("$pak", ""); > + } >=20 > - # Download packages > - foreach $pak (@upgradepaks) { > - &Pakfire::getpak("$pak", ""); > + # Download dependencies > + foreach $pak (@deps) { > + &Pakfire::getpak("$pak", ""); > + } > + > + # Install dependencies first > + foreach $pak (@deps) { > + &Pakfire::setuppak("$pak"); > + } > + > + # Install all upgrades > + foreach $pak (sort keys %upgradepaks) { > + &Pakfire::upgradepak("$pak"); > + } > + } else { > + &Pakfire::message("PAKFIRE WARN: No new package upgrades available."); > } >=20 > - # Download dependencies > - foreach $pak (@deps) { > - &Pakfire::getpak("$pak", ""); > + } elsif ("$ARGV[0]" eq "list") { > + my $count; > + my $use_color =3D ""; > + my $reset_color =3D ""; > + my $filter =3D "all"; > + > + if ("$ARGV[1]" =3D~ /installed|notinstalled/) { > + $filter =3D "$ARGV[1]"; > + } else { > + &Pakfire::message("PAKFIRE WARN: Not a known option $ARGV[1]") if ($ARG= V[1]);=20 > } >=20 > - # Install dependencies first > - foreach $pak (@deps) { > - &Pakfire::setuppak("$pak"); > + my $pak; > + my %paklist =3D &Pakfire::dblist($filter); > + > + if ("$Pakfire::enable_colors" eq "1") { > + $reset_color =3D "$Pakfire::color{'normal'}"; > + $use_color =3D "$Pakfire::color{'lightgreen'}"; > } >=20 > - # Install all upgrades > - foreach $pak (@upgradepaks) { > - &Pakfire::upgradepak("$pak"); > + foreach $pak (sort keys %paklist) { > + if ("$Pakfire::enable_colors" eq "1") { > + if ("$paklist{$pak}{'Installed'}" eq "yes") { > + if (defined $paklist{$pak}{'AvailableProgVersion'}) { > + $use_color =3D "$Pakfire::color{'lightgreen'}"; > + } else { > + $use_color =3D "$Pakfire::color{'green'}"; > + } > + } else { > + $use_color =3D "$Pakfire::color{'red'}";=20 > + } > + } > + > + print "${use_color}Name: $pak\nProgVersion: $paklist{$pak}{'ProgVersion= '}\n"; > + print "Release: $paklist{$pak}{'Release'}\nInstalled: $paklist{$pak}{'I= nstalled'}\n"; > + if (defined $paklist{$pak}{'AvailableProgVersion'}) { > + print "Update available:\n Version: $paklist{$pak}{'ProgVersion'} -> $= paklist{$pak}{'AvailableProgVersion'}\n Release: $paklist{$pak}{'Release'} ->= $paklist{$pak}{'AvailableRelease'}\n"; > + } > + print "${reset_color}\n"; > + =09 > } >=20 > - } elsif ("$ARGV[0]" eq "list") { > - if ("$ARGV[1]" =3D~ /installed|notinstalled/) { > - &Pakfire::dblist("$ARGV[1]", "noweb"); > + $count =3D keys %paklist; > + if ($count > 0) { > + print "$count packages total.\n"; > } else { > - &Pakfire::message("PAKFIRE WARN: Not a known option $ARGV[1]") if ($ARG= V[1]); > - &Pakfire::dblist("all", "noweb"); > + &Pakfire::message("PAKFIRE WARN: No packages where found using filter $= filter."); > } > - > } elsif ("$ARGV[0]" eq "resolvedeps") { > foreach (@ARGV) { > next if ("$_" eq "resolvedeps"); > --=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 -Michael --===============5325901878139201024==--