[1/9] pakfire: Refactor dblist seperating UI and logic

Message ID 20220309225655.4472-2-robin.roevens@disroot.org
State Superseded
Headers
Series pakfire: remove dup. code + seperate ui/logic |

Commit Message

Robin Roevens March 9, 2022, 10:56 p.m. UTC
  - Removed UI code from dblist function and refactor it making it return
  a hash representing the pak db for easier handling of this data.
- 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.

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(-)
  

Comments

Michael Tremer March 21, 2022, 4:18 p.m. UTC | #1
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.

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

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

> +
> +
> 	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?

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{'Name'});
> -					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'},
> +						AvailableProgVersion => $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
  
Robin Roevens March 22, 2022, 12:39 p.m. UTC | #2
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
> 
>
  
Robin Roevens March 23, 2022, 7:18 p.m. UTC | #3
Hi Michael

Robin Roevens schreef op di 22-03-2022 om 13:39 [+0100]:
> > > 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
> 

I looked in the translation files, and currently there doesn't seem to
be a translation string for "Core Update" and "Release". Should I just
add it (to English and Dutch, I can)? Or did I overlook it ?

Regards

Robin
  
Michael Tremer March 23, 2022, 8:49 p.m. UTC | #4
Hello,

> On 23 Mar 2022, at 19:18, Robin Roevens <robin.roevens@disroot.org> wrote:
> 
> Hi Michael
> 
> Robin Roevens schreef op di 22-03-2022 om 13:39 [+0100]:
>>>> 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
>> 
> 
> I looked in the translation files, and currently there doesn't seem to
> be a translation string for "Core Update" and "Release". Should I just
> add it (to English and Dutch, I can)? Or did I overlook it ?

Yes, please just add the strings that you will need.

There are plenty of people who are native Germans here. Please get one of them to contribute the German strings. I don’t know who else we have on this list, but of course other languages are welcome too :)

> 
> Regards
> 
> Robin
> 
> -- 
> Dit bericht is gescanned op virussen en andere gevaarlijke
> inhoud door MailScanner en lijkt schoon te zijn.
>
  

Patch

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";
+	}
+
+	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";
+	}
+
+
 	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`);
+
+	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{'Name'});
-					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'},
+						AvailableProgVersion => $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;
 }
 
 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");