Hi Michael
Michael Tremer schreef op ma 21-03-2022 om 16:18 [+0000]:
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 code with a lot of changes, which cannot introduce any regressions, because if something breaks we cannot fix it in an update...
On 9 Mar 2022, at 22:56, Robin Roevens robin.roevens@disroot.org wrote:
- 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 changes.
I'm doing my best :-) .. but I'm not sure how I should have split this up more ? Assuming every patch should leave the system in a fully functioning state ? By reworking the dblist function, I had to include the changes to the calls to that function, and due to the removal of the ui parts from it, I also had to include the ui additions in other places I only see those small visual enhancements in the ui parts that are now outside of the dblist function as candidate changes for separate patches, but then, as that code is already new in this patch, just changing exactly what is displayed UI-wise didn't look invasive enough to separate it ?
Signed-off-by: Robin Roevens robin.roevens@disroot.org
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(-)
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(); }
-&Pakfire::dblist("upgrade", "notice"); +my %coredb = &Pakfire::coredbinfo(); +if (defined $coredb{'AvailableRelease'}) { + print "<br /><br /><br /><a href='pakfire.cgi'>$Lang::tr{'core notice 1'} $coredb{'Release'} $Lang::tr{'core notice 2'} $coredb{'AvailableRelease'} $Lang::tr{'core notice 3'}</a>"; +}
if ( -e "/var/run/need_reboot" ) { print "<div style='text-align:center; color:red;'>"; print "<br/><br/>$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 <<END; <select name="UPDPAKS" class="pflist" size="5" disabled> END
- &Pakfire::dblist("upgrade", "forweb"); + my %coredb = &Pakfire::coredbinfo(); + if (defined $coredb{'AvailableRelease'}) { + print "<option value="core">Core-Update -- $coredb{'CoreVersion'} -- Release: $coredb{'Release'} -> $coredb{'AvailableRelease'}</option>\n"; + }
The strings “Core-Update” (without the dash) and “Release” should be translated so that they make sense in other languages. You can do that with $Lang::tr (I am sure you know).
Agreed, I don't think it was originally in this place as this came out of the previous dblist function. But you are right, I will use Lang::tr
+ my %upgradelist = &Pakfire::dblist("upgrade"); + foreach my $pak (sort keys %upgradelist) { + print "<option value="$pak">Update: $pak -- Version: $upgradelist{$pak}{'ProgVersion'} -> $upgradelist{$pak}{'AvailableProgVersion'} -- Release: $upgradelist{$pak}{'Release'} -> $upgradelist{$pak}{'AvailableRelease'}</option>\n"; + }
Same again here.
As a UI element, <select> is a very stupid idea. I have no idea why I ever picked that - presumably because there was the intention that users can pick what they want to install and what not. We should move away from this I believe.
Completely agree :-).. I was already trying to completely redesign the look of the pakfire webui page, to for example also include the summary now available in pak metadata. But I did not yet have a satisfying design in mind .. but I will probably propose something for this here with some mockup screenshots soon. So for now I kept it like it was.
print <<END; </select> <input type='hidden' name='ACTION' value='upgrade' /> @@ -386,7 +396,11 @@ END <select name="INSPAKS" class="pflist" size="10" multiple> END
- &Pakfire::dblist("notinstalled", "forweb"); + my %notinstalledlist = &Pakfire::dblist("notinstalled"); + foreach my $pak (sort keys %notinstalledlist) { + print "<option value="$pak">$pak- $notinstalledlist{$pak}{'ProgVersion'}- $notinstalledlist{$pak}{'Release'}</option>\n"; + }
print <<END; </select> <input type='hidden' name='ACTION' value='install' /> @@ -398,7 +412,11 @@ END <select name="DELPAKS" class="pflist" size="10" multiple> END
- &Pakfire::dblist("installed", "forweb"); + my %installedlist = &Pakfire::dblist("installed"); + foreach my $pak (sort keys %installedlist) { + print "<option value="$pak">$pak- $installedlist{$pak}{'ProgVersion'}- $installedlist{$pak}{'Release'}</option>\n"; + }
print <<END; </select> <input type='hidden' name='ACTION' value='remove' /> 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 info@ipfire.org # +# Copyright (C) 2007-2022 IPFire Team info@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 # @@ -44,7 +44,7 @@ my @VALID_KEY_FINGERPRINTS = ( );
# A small color-hash :D -my %color; +our %color; $color{'normal'} = "\033[0m"; $color{'black'} = "\033[0;30m"; $color{'darkgrey'} = "\033[1;30m"; @@ -426,108 +426,93 @@ sub dbgetlist { } }
+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 this code now, can we remove this and just parse the line that we are looking for?
Done in patch 4/9. Due to splitting up my changes in separate patches as much as I thought possible :-)
Robin
I consider this a security problem as it allows code injection.
+ my %coredb = ( + CoreVersion => $Conf::version, + Release => $Conf::core_mine, + );
+ $coredb{'AvailableRelease'} = $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 hash. + # It uses the currently cached version of packages_list. To ensure latest + # data, run Pakfire::dbgetlist first. + # You may also pass a filter: &Pakfire::dblist(filter) + # Usage is always with one argument. + # filter may be: all, notinstalled, installed, upgrade my $filter = shift; - my $forweb = shift; - my @updatepaks; + my %paklist = (); my $file; my $line; - my $prog; my %metadata; my @templine;
- ### Make sure that the list is not outdated. - #dbgetlist("noforce");
+ open(FILE, "<$Conf::dbdir/lists/packages_list.db"); my @db = <FILE>; close(FILE);
- 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 "<option value="core">Core- Update -- $Conf::version -- Release: $Conf::core_mine -> $core_release</option>\n"; - } - elsif ("$forweb" eq "notice") { - print "<br /><br /><br /><a href='pakfire.cgi'>$Lang::tr{'core notice 1'} $Conf::core_mine $Lang::tr{'core notice 2'} $core_release $Lang::tr{'core notice 3'}</a>"; - } else { - my $command = "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 = readdir(DIR); closedir(DIR);
foreach $file (@files) { next if ( $file eq "." ); next if ( $file eq ".." ); next if ( $file =~ /^old/ ); %metadata = parsemetafile("$Conf::dbdir/installed/$file");
- foreach $prog (@db) { - @templine = split(/;/,$prog); - if (("$metadata{'Name'}" eq "$templine[0]") && ("$metadata{'Release'}" < "$templine[2]" && "$forweb" ne "notice")) {
push(@updatepaks,$metadata{'N ame'}); - if ("$forweb" eq "forweb") { - print "<option value="$metadata{'Name'}">Update: $metadata{'Name'} -- Version: $metadata{'ProgVersion'} -> $templine[1] -- Release: $metadata{'Release'} -> $templine[2]</option>\n"; - } else { - my $command = "Update: $metadata{'Name'}\nVersion: $metadata{'ProgVersion'} -> $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 =~ /.*;.*;.*;/ ); + @templine = split(/;/,$line); + if (("$metadata{'Name'}" eq "$templine[0]") && ("$metadata{'Release'}" < "$templine[2]")) { #&& "$forweb" ne "notice")) { + # Add all upgradable paks to list + $paklist{"$metadata{'Name'} "} = { + ProgVersion => $metadata{'ProgVersion'}, + Release => $metadata{'Release'}, + AvailableProgVersio n => $templine[1], + AvailableRelease => $templine[2], + Installed => "yes" + }; + last; + } elsif (("$metadata{'Name'}" eq "$templine[0]") && ("$filter" ne "upgrade")) { + # Add installed paks without an upgrade available to list + $paklist{"$metadata{'Name'} "} = { + ProgVersion => $metadata{'ProgVersion'}, + Release => $metadata{'Release'}, + Installed => "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 =~ /.*;.*;.*;/ ); - $use_color = ""; @templine = 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 "<option value="$templine[0]">$templine[0]-$templine[1]- $templine[2]</option>\n"; - } else { - print "<option value="$templine[0]">$templine[0]</option>\n"; - } - } else { - if ("$Pakfire::enable_colors" eq "1") { - if (&isinstalled("$templine[0]")) { - $use_color = "$color{'red'}" - } else { - $use_color = "$color{'green'}" - } - } - print "${use_color}Name: $templine[0]\nProgVersion: $templine[1]\nRelease: $templine[2]$color{'normal'}\n\n"; - } + next if ((defined $paklist{"$templine[0]"}) || (&isinstalled($templine[0]) == 0));
+ $paklist{"$templine[0]"} = { + ProgVersion => "$templine[1]", + Release => "$templine[2]", + Installed => "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 {
sub updates_available { # Get packets with updates available - my @upgradepaks = &Pakfire::dblist("upgrade", "noweb"); + my %upgradepaks = &Pakfire::dblist("upgrade");
- # Get the length of the returned array - my $updatecount = scalar @upgradepaks; + # Get the length of the returned hash + my $updatecount = keys %upgradepaks;
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");
} elsif ("$ARGV[0]" eq "upgrade") { + my $use_color = ""; + my $reset_color = "";
+ if ("$Pakfire::enable_colors" eq "1") { + $reset_color = "$Pakfire::color{'normal'}"; + $use_color = "$Pakfire::color{'lightpurple'}"; + }
&Pakfire::upgradecore(); - my @upgradepaks = &Pakfire::dblist("upgrade", "noweb"); + my @deps = ();
- if (@upgradepaks) { + if (my %upgradepaks = &Pakfire::dblist("upgrade")) { # Resolve the dependencies of the to be upgraded packages - @deps = &Pakfire::resolvedeps_recursive(@upgradepaks); + @deps = &Pakfire::resolvedeps_recursive(keys %upgradepaks);
+ foreach $pak (sort keys %upgradepaks) { + print "${use_color}Update: $pak\nVersion: $upgradepaks{$pak}{'ProgVersion'} -> $upgradepaks{$pak}{'AvailableProgVersion'}\n"; + print "Release: $upgradepaks{$pak}{'Release'} -> $upgradepaks{$pak}{'AvailableRelease'}${reset_color}\n"; + } &Pakfire::message(""); &Pakfire::message("PAKFIRE UPGR: We are going to install all packages listed above."); if ($interactive) { @@ -290,36 +301,78 @@ exit 1; } } - } + + # Download packages + foreach $pak (sort keys %upgradepaks) { + &Pakfire::getpak("$pak", ""); + }
- # 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."); }
- # Download dependencies - foreach $pak (@deps) { - &Pakfire::getpak("$pak", ""); + } elsif ("$ARGV[0]" eq "list") { + my $count; + my $use_color = ""; + my $reset_color = ""; + my $filter = "all";
+ if ("$ARGV[1]" =~ /installed|notinstalled/) { + $filter = "$ARGV[1]"; + } else { + &Pakfire::message("PAKFIRE WARN: Not a known option $ARGV[1]") if ($ARGV[1]); }
- # Install dependencies first - foreach $pak (@deps) { - &Pakfire::setuppak("$pak"); + my $pak; + my %paklist = &Pakfire::dblist($filter);
+ if ("$Pakfire::enable_colors" eq "1") { + $reset_color = "$Pakfire::color{'normal'}"; + $use_color = "$Pakfire::color{'lightgreen'}"; }
- # 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 = "$Pakfire::color{'lightgreen'}"; + } else { + $use_color = "$Pakfire::color{'green'}"; + } + } else { + $use_color = "$Pakfire::color{'red'}"; + } + }
+ print "${use_color}Name: $pak\nProgVersion: $paklist{$pak}{'ProgVersion'}\n"; + print "Release: $paklist{$pak}{'Release'}\nInstalled: $paklist{$pak}{'Installed'}\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"; + }
- } elsif ("$ARGV[0]" eq "list") { - if ("$ARGV[1]" =~ /installed|notinstalled/) { - &Pakfire::dblist("$ARGV[1]", "noweb"); + $count = keys %paklist; + if ($count > 0) { + print "$count packages total.\n"; } else { - &Pakfire::message("PAKFIRE WARN: Not a known option $ARGV[1]") if ($ARGV[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"); -- 2.34.1
-- Dit bericht is gescanned op virussen en andere gevaarlijke inhoud door MailScanner en lijkt schoon te zijn.
-Michael