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

[Michael Tremer <michael.tremer@ipfire.org>]

[-- Attachment #1: Type: text/plain, Size: 17403 bytes --]

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(a)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(a)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(a)ipfire.org>                   #
> +# Copyright (C) 2007-2022   IPFire Team   <info(a)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