[PATCH 1/5] location-functions.pl: Use a single script-wide db_handle.

[PATCH 1/5] location-functions.pl: Use a single script-wide db_handle.

[Stefan Schantl]

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

Create and use a single script-wide database handle for libloc to
prevent from creating multiple ones.

This helps saving memory, especially on small systems.

Reference #12515.

Signed-off-by: Stefan Schantl <stefan.schantl(a)ipfire.org>
---
 config/cfgroot/location-functions.pl | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/config/cfgroot/location-functions.pl b/config/cfgroot/location-functions.pl
index 2cfe7f908..9b1d0bfb5 100644
--- a/config/cfgroot/location-functions.pl
+++ b/config/cfgroot/location-functions.pl
@@ -55,6 +55,9 @@ our $keyfile = "$location_dir/signing-key.pem";
 # Directory which contains the exported databases.
 our $xt_geoip_db_directory = "/usr/share/xt_geoip/";
 
+# Create libloc database handle.
+my $db_handle = &init();
+
 #
 ## Tiny function to init the location database.
 #
@@ -86,7 +89,7 @@ sub verify ($) {
 ## Function to the the country code of a given address.
 #
 sub lookup_country_code($$) {
-	my ($db_handle, $address) = @_;
+	my ($address) = @_;
 
 	# Lookup the given address.
 	my $country_code = &Location::lookup_country_code($db_handle, $address);
@@ -174,9 +177,6 @@ sub get_full_country_name($) {
 
 # Function to get all available locations.
 sub get_locations() {
-	# Create libloc database handle.
-	my $db_handle = &init();
-
 	# Get locations which are stored in the location database.
 	my @database_locations = &Location::database_countries($db_handle);
 
@@ -197,9 +197,6 @@ sub address_has_flags($) {
 	# Array to store the flags of the address.
 	my @flags;
 
-	# Init libloc database handle.
-	my $db_handle = &init();
-
 	# Loop through the hash of possible network flags.
 	foreach my $flag (keys(%network_flags)) {
 		# Check if the address has the current flag.
-- 
2.20.1

[PATCH 2/5] location-functions.pl: Add END block to release the database handle.

[Stefan Schantl]

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

Reference #12515.

Signed-off-by: Stefan Schantl <stefan.schantl(a)ipfire.org>
---
 config/cfgroot/location-functions.pl | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/config/cfgroot/location-functions.pl b/config/cfgroot/location-functions.pl
index 9b1d0bfb5..483703016 100644
--- a/config/cfgroot/location-functions.pl
+++ b/config/cfgroot/location-functions.pl
@@ -218,4 +218,14 @@ sub address_has_flags($) {
 	return @flags;
 }
 
+# Custom END declaration which will be executed when perl
+# ends, to release the database handle to libloc.
+END {
+	# Check if a database handle exists.
+	if ($db_handle) {
+		# Destroy libloc database handle.
+		&Location::DESTROY($db_handle);
+	}
+}
+
 1;
-- 
2.20.1

[PATCH 3/5] location-functions.pl: Add get_continent_code() function.

[Stefan Schantl]

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

This tiny function is used to get the continent code for a given
country code.

Signed-off-by: Stefan Schantl <stefan.schantl(a)ipfire.org>
---
 config/cfgroot/location-functions.pl | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/config/cfgroot/location-functions.pl b/config/cfgroot/location-functions.pl
index 483703016..e8db4ba9b 100644
--- a/config/cfgroot/location-functions.pl
+++ b/config/cfgroot/location-functions.pl
@@ -190,6 +190,16 @@ sub get_locations() {
 	return @sorted_locations;
 }
 
+# Function to get the continent code of a given country code.
+sub get_continent_code($) {
+	my ($country_code) = @_;
+
+	# Use location module to grab the continent code.
+	my $continent_code = &Location::get_continent_code($db_handle, $country_code);
+
+	return $continent_code;
+}
+
 # Function to check if a given address has one ore more special flags.
 sub address_has_flags($) {
 	my ($address) = @_;
-- 
2.20.1

[PATCH 4/5] locations-functions.pl: Allow get_locations() function to skip special locations.

[Stefan Schantl]

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

When adding "no_special_locations" to the function call as argument
the special locations liks "A1, A2, A3 etc" will not be added to the
returned array as available locations.

Signed-off-by: Stefan Schantl <stefan.schantl(a)ipfire.org>
---
 config/cfgroot/location-functions.pl | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/config/cfgroot/location-functions.pl b/config/cfgroot/location-functions.pl
index e8db4ba9b..fdf2c6efe 100644
--- a/config/cfgroot/location-functions.pl
+++ b/config/cfgroot/location-functions.pl
@@ -177,16 +177,24 @@ sub get_full_country_name($) {
 
 # Function to get all available locations.
 sub get_locations() {
+	my ($mode) = @_;
+
+	# Set default mode to add_special_locations.
+	$mode = $mode ? $mode : "add_special_locations";
+
 	# Get locations which are stored in the location database.
-	my @database_locations = &Location::database_countries($db_handle);
+	my @locations = &Location::database_countries($db_handle);
 
-	# Merge special locations array and the database locations array.
-	my @locations = (@special_locations, @database_locations);
+	# Check if the special locations should be added.
+	if ($mode ne "no_special_locations") {
+		# Merge special locations array and the database locations array.
+		@locations = (@special_locations, @locations);
+	}
 
 	# Sort locations array in alphabetical order.
 	my @sorted_locations = sort(@locations);
 
-	# Return the array..
+	# Return the array.
 	return @sorted_locations;
 }
 
-- 
2.20.1

[PATCH 5/5] Adjust CGI files to work with latest location-function.pl changes.

[Stefan Schantl]

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

Fixes #12515.

Signed-off-by: Stefan Schantl <stefan.schantl(a)ipfire.org>
---
 html/cgi-bin/connections.cgi                     | 7 ++-----
 html/cgi-bin/country.cgi                         | 5 +----
 html/cgi-bin/dns.cgi                             | 5 +----
 html/cgi-bin/ipinfo.cgi                          | 5 ++---
 html/cgi-bin/logs.cgi/firewalllog.dat            | 6 ++----
 html/cgi-bin/logs.cgi/firewalllogcountry.dat     | 5 +----
 html/cgi-bin/logs.cgi/firewalllogip.dat          | 6 ++----
 html/cgi-bin/logs.cgi/showrequestfromcountry.dat | 7 ++-----
 html/cgi-bin/ovpnmain.cgi                        | 5 +----
 html/cgi-bin/remote.cgi                          | 6 +-----
 html/cgi-bin/tor.cgi                             | 7 ++-----
 11 files changed, 17 insertions(+), 47 deletions(-)

diff --git a/html/cgi-bin/connections.cgi b/html/cgi-bin/connections.cgi
index 6c55bd7a0..8613b9d9b 100644
--- a/html/cgi-bin/connections.cgi
+++ b/html/cgi-bin/connections.cgi
@@ -86,9 +86,6 @@ if ( $debug ){
 my @dummy = ( ${Header::table1colour} );
 undef (@dummy);
 
-# Init libloc database connection.
-my $libloc_db_handle = &Location::Functions::init();
-
 # check sorting arguments
 if ( $cgiin{'sort_field'} ~~ [ '1','2','3','4','5','6','7','8','9' ] ) {
 	$SORT_FIELD = $cgiin{'sort_field'};
@@ -554,9 +551,9 @@ foreach my $line (@conntrack) {
 	my $bytes_out = format_bytes($bytes[1]);
 
 	# enumerate location information
-	my $srcccode = &Location::Functions::lookup_country_code($libloc_db_handle, $sip_ret);
+	my $srcccode = &Location::Functions::lookup_country_code($sip_ret);
 	my $src_flag_icon = &Location::Functions::get_flag_icon($srcccode);
-	my $dstccode = &Location::Functions::lookup_country_code($libloc_db_handle, $dip_ret);
+	my $dstccode = &Location::Functions::lookup_country_code($dip_ret);
 	my $dst_flag_icon = &Location::Functions::get_flag_icon($dstccode);
 
 	# Format TTL
diff --git a/html/cgi-bin/country.cgi b/html/cgi-bin/country.cgi
index b519d89b3..b1c72bb22 100644
--- a/html/cgi-bin/country.cgi
+++ b/html/cgi-bin/country.cgi
@@ -52,11 +52,8 @@ print<<END;
 	</tr>
 END
 
-# Init libloc database connection.
-my $db_handle = &Location::Functions::init();
-
 # Get a list of all supported country codes.
-my @countries = &Location::database_countries($db_handle);
+my @countries = &Location::database_countries();
 
 # Loop through whole country list.
 foreach my $country (@countries) {
diff --git a/html/cgi-bin/dns.cgi b/html/cgi-bin/dns.cgi
index 13dd9d6a0..762e77ff1 100755
--- a/html/cgi-bin/dns.cgi
+++ b/html/cgi-bin/dns.cgi
@@ -269,9 +269,6 @@ my %dns_servers = ();
 # Read-in config file.
 &General::readhasharray("$servers_file", \%dns_servers);
 
-# Libloc database handle
-my $libloc_db_handle = &Location::Functions::init();
-
 &Header::openpage($Lang::tr{'dns title'}, 1, '');
 
 &Header::openbigbox('100%', 'left', '', $errormessage);
@@ -598,7 +595,7 @@ END
 				}
 
 				# collect more information about name server (rDNS, country code)
-				my $ccode = &Location::Functions::lookup_country_code($libloc_db_handle, $nameserver);
+				my $ccode = &Location::Functions::lookup_country_code($nameserver);
 				my $flag_icon = &Location::Functions::get_flag_icon($ccode);
 
 				my $rdns;
diff --git a/html/cgi-bin/ipinfo.cgi b/html/cgi-bin/ipinfo.cgi
index d8cb6c6b7..ecc7c436c 100644
--- a/html/cgi-bin/ipinfo.cgi
+++ b/html/cgi-bin/ipinfo.cgi
@@ -62,12 +62,11 @@ if (&General::validip($addr)) {
 	if (!$hostname) { $hostname = $Lang::tr{'lookup failed'}; }
 
 	# enumerate location information for IP address...
-	my $db_handle = &Location::Functions::init();
-	my $ccode = &Location::Functions::lookup_country_code($db_handle, $addr);
+	my $ccode = &Location::Functions::lookup_country_code($addr);
 	my @network_flags = &Location::Functions::address_has_flags($addr);
 
 	# Try to get the continent of the country code.
-	my $continent = &Location::get_continent_code($db_handle, $ccode);
+	my $continent = &Location::Functions::get_continent_code($ccode);
 
 	# Check if a whois server for the continent is known.
 	if($whois_servers_by_continent{$continent}) {
diff --git a/html/cgi-bin/logs.cgi/firewalllog.dat b/html/cgi-bin/logs.cgi/firewalllog.dat
index 2b690e35b..361bf0432 100644
--- a/html/cgi-bin/logs.cgi/firewalllog.dat
+++ b/html/cgi-bin/logs.cgi/firewalllog.dat
@@ -24,9 +24,6 @@ require "${General::swroot}/location-functions.pl";
 require "${General::swroot}/lang.pl";
 require "${General::swroot}/header.pl";
 
-# Libloc database handle.
-my $libloc_db_handle = &Location::Functions::init();
-
 my %color = ();
 my %mainsettings = ();
 &General::readhash("${General::swroot}/main/settings", \%mainsettings);
@@ -354,7 +351,8 @@ foreach $_ (@log)
 	$srcport=$1 if $packet =~ /SPT=(\d+)/;
 	$dstport=$1 if $packet =~ /DPT=(\d+)/;
 
-	my $ccode = &Location::Functions::lookup_country_code($libloc_db_handle, $srcaddr);
+	# Get the country code.
+	my $ccode = &Location::Functions::lookup_country_code($srcaddr);
 
 	my $servi = uc(getservbyport($srcport, lc($proto)));
 	if ($servi ne '' && $srcport < 1024) {
diff --git a/html/cgi-bin/logs.cgi/firewalllogcountry.dat b/html/cgi-bin/logs.cgi/firewalllogcountry.dat
index 701abab2c..e3901b945 100644
--- a/html/cgi-bin/logs.cgi/firewalllogcountry.dat
+++ b/html/cgi-bin/logs.cgi/firewalllogcountry.dat
@@ -22,9 +22,6 @@ require "${General::swroot}/location-functions.pl";
 require "${General::swroot}/lang.pl";
 require "${General::swroot}/header.pl";
 
-# Libloc database handle.
-my $libloc_db_handle = &Location::Functions::init();
-
 use POSIX();
 
 my %cgiparams=();
@@ -311,7 +308,7 @@ foreach $_ (@log)
 		# Traffic from red
 		if($srcaddr ne '') {
 			# srcaddr is set
-			my $ccode = &Location::Functions::lookup_country_code($libloc_db_handle, $srcaddr);
+			my $ccode = &Location::Functions::lookup_country_code($srcaddr);
 			if ($ccode eq '') {
 				$ccode = 'unknown';
 			}
diff --git a/html/cgi-bin/logs.cgi/firewalllogip.dat b/html/cgi-bin/logs.cgi/firewalllogip.dat
index 670d72a52..6de4081af 100644
--- a/html/cgi-bin/logs.cgi/firewalllogip.dat
+++ b/html/cgi-bin/logs.cgi/firewalllogip.dat
@@ -22,9 +22,6 @@ require "${General::swroot}/location-functions.pl";
 require "${General::swroot}/lang.pl";
 require "${General::swroot}/header.pl";
 
-# Libloc database handle.
-my $libloc_db_handle = &Location::Functions::init();
-
 use POSIX();
 
 my %cgiparams=();
@@ -438,7 +435,8 @@ for($s=0;$s<$lines;$s++)
 	$col="bgcolor='$color{\"color$colorIndex\"}'";
 	print "<tr>";
 
-	my $ccode = &Location::Functions::lookup_country_code($libloc_db_handle, $key[$s]);
+	# Get country code.
+	my $ccode = &Location::Functions::lookup_country_code($key[$s]);
   
 	$color++;
 	print "<td align='center' $col><form method='post' action='showrequestfromip.dat'><input type='hidden' name='MONTH' value='$cgiparams{'MONTH'}'> <input type='hidden' name='DAY' value='$cgiparams{'DAY'}'> <input type='hidden' name='ip' value='$key[$s]'> <input type='submit' value='$Lang::tr{'details'}'></form></td>";
diff --git a/html/cgi-bin/logs.cgi/showrequestfromcountry.dat b/html/cgi-bin/logs.cgi/showrequestfromcountry.dat
index 4d80e77a4..2a246ec60 100644
--- a/html/cgi-bin/logs.cgi/showrequestfromcountry.dat
+++ b/html/cgi-bin/logs.cgi/showrequestfromcountry.dat
@@ -19,9 +19,6 @@ require "${General::swroot}/location-functions.pl";
 require "${General::swroot}/lang.pl";
 require "${General::swroot}/header.pl";
 
-# Libloc database handle.
-my $libloc_db_handle = &Location::Functions::init();
-
 use POSIX();
 
 #workaround to suppress a warning when a variable is used only once
@@ -181,7 +178,7 @@ if (!$skip)
 			}
 			elsif($srcaddr ne '') {
 				# or srcaddr matches country code
-				my $ccode = &Location::Functions::lookup_country_code($libloc_db_handle, $srcaddr);
+				my $ccode = &Location::Functions::lookup_country_code($srcaddr);
 				if($ccode eq uc($country)){
 					$log[$lines] = $_;
 					$lines++;
@@ -352,7 +349,7 @@ foreach $_ (@slice)
 	if($iface eq $country || $srcaddr ne '') {
 		my $ccode='';
 		if($iface ne $country) {
-			$ccode = &Location::Functions::lookup_country_code($libloc_db_handle, $srcaddr);
+			$ccode = &Location::Functions::lookup_country_code($srcaddr);
 		}
 		if($iface eq $country || $ccode eq uc($country)) {
 			my $chain = '';
diff --git a/html/cgi-bin/ovpnmain.cgi b/html/cgi-bin/ovpnmain.cgi
index e7bc505e7..8626a94ca 100644
--- a/html/cgi-bin/ovpnmain.cgi
+++ b/html/cgi-bin/ovpnmain.cgi
@@ -3002,9 +3002,6 @@ END
 	&Header::openbigbox('100%', 'LEFT', '', '');
     &Header::openbox('100%', 'LEFT', $Lang::tr{'ovpn con stat'});
 
-    # Libloc database handle.
-    my $libloc_db_handle = &Location::Functions::init();
-
 #
 #	<td><b>$Lang::tr{'protocol'}</b></td>
 # protocol temp removed 
@@ -3055,7 +3052,7 @@ END
 		    $users[$uid]{'Proto'} = $proto;
 
 		    # get country code for "RealAddress"...
-		    my $ccode = &Location::Functions::lookup_country_code($libloc_db_handle, (split ':', $users[$uid]{'RealAddress'})[0]);
+		    my $ccode = &Location::Functions::lookup_country_code((split ':', $users[$uid]{'RealAddress'})[0]);
 		    my $flag_icon = &Location::Functions::get_flag_icon($ccode);
 		    $users[$uid]{'Country'} = "<a href='country.cgi#$ccode'><img src='$flag_icon' border='0' align='absmiddle' alt='$ccode' title='$ccode' /></a>";
 		    $uid++;
diff --git a/html/cgi-bin/remote.cgi b/html/cgi-bin/remote.cgi
index 9c742669b..a27e10de9 100644
--- a/html/cgi-bin/remote.cgi
+++ b/html/cgi-bin/remote.cgi
@@ -277,10 +277,6 @@ sub printactivelogins()
 		print "<tr bgcolor='$table_colour'><td colspan='5'>$Lang::tr{'ssh no active logins'}</td></tr>\n";
 	} else {
 		# list active logins...
-
-		# Libloc database handle.
-		my $libloc_db_handle = &Location::Functions::init();
-
 		foreach my $line (@output)
 		{
 			my @arry = split(/\ +/, $line);
@@ -291,7 +287,7 @@ sub printactivelogins()
 			$remoteip =~ s/[()]//g;
 
 			# display more information about that IP adress...
-			my $ccode = &Location::Functions::lookup_country_code($libloc_db_handle, $remoteip);
+			my $ccode = &Location::Functions::lookup_country_code($remoteip);
 			my $flag_icon = &Location::Functions::get_flag_icon($ccode);
 
 			# get rDNS...
diff --git a/html/cgi-bin/tor.cgi b/html/cgi-bin/tor.cgi
index 14bfcfe90..983bb30c9 100644
--- a/html/cgi-bin/tor.cgi
+++ b/html/cgi-bin/tor.cgi
@@ -30,9 +30,6 @@ require "${General::swroot}/location-functions.pl";
 require "${General::swroot}/lang.pl";
 require "${General::swroot}/header.pl";
 
-# Init libloc database connection.
-my $db_handle = &Location::Functions::init();
-
 #workaround to suppress a warning when a variable is used only once
 my @dummy = ( ${Header::colouryellow} );
 undef (@dummy);
@@ -322,7 +319,7 @@ END
 					<select name='TOR_EXIT_COUNTRY'>
 						<option value=''>- $Lang::tr{'tor exit country any'} -</option>
 END
-		my @country_codes = &Location::database_countries($db_handle);
+		my @country_codes = &Location::Functions::get_locations("no_special_locations");
 		foreach my $country_code (@country_codes) {
 			# Convert country code into upper case format.
 			$country_code = uc($country_code);
@@ -912,7 +909,7 @@ sub TorNodeDescription() {
 			$node->{'address'} = $3;
 			$node->{'port'}    = $4;
 
-			my $country_code = &Location::Functions::lookup_country_code($db_handle, $node->{'address'});
+			my $country_code = &Location::Functions::lookup_country_code($node->{'address'});
 			$node->{'country_code'} = $country_code;
 
 		# Flags
-- 
2.20.1

Aw: [PATCH 1/5] location-functions.pl: Use a single script-wide db_handle.

[Bernhard Bitsch]

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

This means, we stay with the unbalanced memory allocation in (Perl) libloc. Which leaves a memory leak.

> Gesendet: Samstag, 07. November 2020 um 19:47 Uhr
> Von: "Stefan Schantl" <stefan.schantl(a)ipfire.org>
> An: development(a)lists.ipfire.org
> Betreff: [PATCH 1/5] location-functions.pl: Use a single script-wide db_handle.
>
> Create and use a single script-wide database handle for libloc to
> prevent from creating multiple ones.
>
> This helps saving memory, especially on small systems.
>
> Reference #12515.
>

The error can be produced easily with small memory, but it is present in all systems.
Therefore I've posted this solution as work-around only!

- Bernhard


> Signed-off-by: Stefan Schantl <stefan.schantl(a)ipfire.org>
> ---
>  config/cfgroot/location-functions.pl | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/config/cfgroot/location-functions.pl b/config/cfgroot/location-functions.pl
> index 2cfe7f908..9b1d0bfb5 100644
> --- a/config/cfgroot/location-functions.pl
> +++ b/config/cfgroot/location-functions.pl
> @@ -55,6 +55,9 @@ our $keyfile = "$location_dir/signing-key.pem";
>  # Directory which contains the exported databases.
>  our $xt_geoip_db_directory = "/usr/share/xt_geoip/";
>
> +# Create libloc database handle.
> +my $db_handle = &init();
> +
>  #
>  ## Tiny function to init the location database.
>  #
> @@ -86,7 +89,7 @@ sub verify ($) {
>  ## Function to the the country code of a given address.
>  #
>  sub lookup_country_code($$) {
> -	my ($db_handle, $address) = @_;
> +	my ($address) = @_;
>
>  	# Lookup the given address.
>  	my $country_code = &Location::lookup_country_code($db_handle, $address);
> @@ -174,9 +177,6 @@ sub get_full_country_name($) {
>
>  # Function to get all available locations.
>  sub get_locations() {
> -	# Create libloc database handle.
> -	my $db_handle = &init();
> -
>  	# Get locations which are stored in the location database.
>  	my @database_locations = &Location::database_countries($db_handle);
>
> @@ -197,9 +197,6 @@ sub address_has_flags($) {
>  	# Array to store the flags of the address.
>  	my @flags;
>
> -	# Init libloc database handle.
> -	my $db_handle = &init();
> -
>  	# Loop through the hash of possible network flags.
>  	foreach my $flag (keys(%network_flags)) {
>  		# Check if the address has the current flag.
> --
> 2.20.1
>
>

Re: [PATCH 1/5] location-functions.pl: Use a single script-wide db_handle.

[Michael Tremer]

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

Hello,

Thank you Stefan for submitting this patchset.

> On 8 Nov 2020, at 19:36, Bernhard Bitsch <Bernhard.Bitsch(a)gmx.de> wrote:
> 
> This means, we stay with the unbalanced memory allocation in (Perl) libloc. Which leaves a memory leak.

Bernhard, could you please elaborate on how this memory leak is still existing?

I also do not understand what you mean by unbalanced.

As far as I understand the code right now, the database is being opened once and the handle is being stored internally. All functions that are being called will no longer have to hold their own database handle. Therefore the maximum amount of handles open is one.

> 
>> Gesendet: Samstag, 07. November 2020 um 19:47 Uhr
>> Von: "Stefan Schantl" <stefan.schantl(a)ipfire.org>
>> An: development(a)lists.ipfire.org
>> Betreff: [PATCH 1/5] location-functions.pl: Use a single script-wide db_handle.
>> 
>> Create and use a single script-wide database handle for libloc to
>> prevent from creating multiple ones.
>> 
>> This helps saving memory, especially on small systems.
>> 
>> Reference #12515.
>> 
> 
> The error can be produced easily with small memory, but it is present in all systems.
> Therefore I've posted this solution as work-around only!

Did you test this patchset or did you come to your conclusion by only reading the code?

Best,
-Michael

> 
> - Bernhard
> 
> 
>> Signed-off-by: Stefan Schantl <stefan.schantl(a)ipfire.org>
>> ---
>> config/cfgroot/location-functions.pl | 11 ++++-------
>> 1 file changed, 4 insertions(+), 7 deletions(-)
>> 
>> diff --git a/config/cfgroot/location-functions.pl b/config/cfgroot/location-functions.pl
>> index 2cfe7f908..9b1d0bfb5 100644
>> --- a/config/cfgroot/location-functions.pl
>> +++ b/config/cfgroot/location-functions.pl
>> @@ -55,6 +55,9 @@ our $keyfile = "$location_dir/signing-key.pem";
>> # Directory which contains the exported databases.
>> our $xt_geoip_db_directory = "/usr/share/xt_geoip/";
>> 
>> +# Create libloc database handle.
>> +my $db_handle = &init();
>> +
>> #
>> ## Tiny function to init the location database.
>> #
>> @@ -86,7 +89,7 @@ sub verify ($) {
>> ## Function to the the country code of a given address.
>> #
>> sub lookup_country_code($$) {
>> -	my ($db_handle, $address) = @_;
>> +	my ($address) = @_;
>> 
>> 	# Lookup the given address.
>> 	my $country_code = &Location::lookup_country_code($db_handle, $address);
>> @@ -174,9 +177,6 @@ sub get_full_country_name($) {
>> 
>> # Function to get all available locations.
>> sub get_locations() {
>> -	# Create libloc database handle.
>> -	my $db_handle = &init();
>> -
>> 	# Get locations which are stored in the location database.
>> 	my @database_locations = &Location::database_countries($db_handle);
>> 
>> @@ -197,9 +197,6 @@ sub address_has_flags($) {
>> 	# Array to store the flags of the address.
>> 	my @flags;
>> 
>> -	# Init libloc database handle.
>> -	my $db_handle = &init();
>> -
>> 	# Loop through the hash of possible network flags.
>> 	foreach my $flag (keys(%network_flags)) {
>> 		# Check if the address has the current flag.
>> --
>> 2.20.1
>> 
>> 

Re: [PATCH 4/5] locations-functions.pl: Allow get_locations() function to skip special locations.

[Michael Tremer]

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

Hi,

Where is this being used?

-Michael

> On 7 Nov 2020, at 18:47, Stefan Schantl <stefan.schantl(a)ipfire.org> wrote:
> 
> When adding "no_special_locations" to the function call as argument
> the special locations liks "A1, A2, A3 etc" will not be added to the
> returned array as available locations.
> 
> Signed-off-by: Stefan Schantl <stefan.schantl(a)ipfire.org>
> ---
> config/cfgroot/location-functions.pl | 16 ++++++++++++----
> 1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/config/cfgroot/location-functions.pl b/config/cfgroot/location-functions.pl
> index e8db4ba9b..fdf2c6efe 100644
> --- a/config/cfgroot/location-functions.pl
> +++ b/config/cfgroot/location-functions.pl
> @@ -177,16 +177,24 @@ sub get_full_country_name($) {
> 
> # Function to get all available locations.
> sub get_locations() {
> +	my ($mode) = @_;
> +
> +	# Set default mode to add_special_locations.
> +	$mode = $mode ? $mode : "add_special_locations";
> +
> 	# Get locations which are stored in the location database.
> -	my @database_locations = &Location::database_countries($db_handle);
> +	my @locations = &Location::database_countries($db_handle);
> 
> -	# Merge special locations array and the database locations array.
> -	my @locations = (@special_locations, @database_locations);
> +	# Check if the special locations should be added.
> +	if ($mode ne "no_special_locations") {
> +		# Merge special locations array and the database locations array.
> +		@locations = (@special_locations, @locations);
> +	}
> 
> 	# Sort locations array in alphabetical order.
> 	my @sorted_locations = sort(@locations);
> 
> -	# Return the array..
> +	# Return the array.
> 	return @sorted_locations;
> }
> 
> -- 
> 2.20.1
> 

Aw: Re: [PATCH 1/5] location-functions.pl: Use a single script-wide db_handle.

[Bernhard Bitsch]

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

Hello,

> Gesendet: Montag, 09. November 2020 um 12:35 Uhr
> Von: "Michael Tremer" <michael.tremer(a)ipfire.org>
> An: "Bernhard Bitsch" <Bernhard.Bitsch(a)gmx.de>
> Cc: development(a)lists.ipfire.org
> Betreff: Re: [PATCH 1/5] location-functions.pl: Use a single script-wide db_handle.
>
> Hello,
>
> Thank you Stefan for submitting this patchset.
>
> > On 8 Nov 2020, at 19:36, Bernhard Bitsch <Bernhard.Bitsch(a)gmx.de> wrote:
> >
> > This means, we stay with the unbalanced memory allocation in (Perl) libloc. Which leaves a memory leak.
>
> Bernhard, could you please elaborate on how this memory leak is still existing?
>
> I also do not understand what you mean by unbalanced.
>
> As far as I understand the code right now, the database is being opened once and the handle is being stored internally. All functions that are being called will no longer have to hold their own database handle. Therefore the maximum amount of handles open is one.
>

The init() function allocates the handle, which is not really destroyed. The END block is just a safety process. The allocated memory for the handle should be released in the moment of references==0.
Yes, the new code ( inspired by my work-around ) minimizes the number of handles to one. But this handle is persistent also ( this doesn't hurt because it is used persistent for the time of module use ).

> >
> >> Gesendet: Samstag, 07. November 2020 um 19:47 Uhr
> >> Von: "Stefan Schantl" <stefan.schantl(a)ipfire.org>
> >> An: development(a)lists.ipfire.org
> >> Betreff: [PATCH 1/5] location-functions.pl: Use a single script-wide db_handle.
> >>
> >> Create and use a single script-wide database handle for libloc to
> >> prevent from creating multiple ones.
> >>
> >> This helps saving memory, especially on small systems.
> >>
> >> Reference #12515.
> >>
> >
> > The error can be produced easily with small memory, but it is present in all systems.
> > Therefore I've posted this solution as work-around only!
>
> Did you test this patchset or did you come to your conclusion by only reading the code?
>
> Best,
> -Michael
>

The sentence about "small systems" refers to the original comment. It is not a real memory saving, the patch cures a 'memory wasting'. It is clear, that a massively use of malloc without free crashes a small system faster than a system with big memory resources.
The patchset is mainly my work-around, thus it is in test since I found the error.

Best,
-Bernhard

> >
> > - Bernhard
> >
> >
> >> Signed-off-by: Stefan Schantl <stefan.schantl(a)ipfire.org>
> >> ---
> >> config/cfgroot/location-functions.pl | 11 ++++-------
> >> 1 file changed, 4 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/config/cfgroot/location-functions.pl b/config/cfgroot/location-functions.pl
> >> index 2cfe7f908..9b1d0bfb5 100644
> >> --- a/config/cfgroot/location-functions.pl
> >> +++ b/config/cfgroot/location-functions.pl
> >> @@ -55,6 +55,9 @@ our $keyfile = "$location_dir/signing-key.pem";
> >> # Directory which contains the exported databases.
> >> our $xt_geoip_db_directory = "/usr/share/xt_geoip/";
> >>
> >> +# Create libloc database handle.
> >> +my $db_handle = &init();
> >> +
> >> #
> >> ## Tiny function to init the location database.
> >> #
> >> @@ -86,7 +89,7 @@ sub verify ($) {
> >> ## Function to the the country code of a given address.
> >> #
> >> sub lookup_country_code($$) {
> >> -	my ($db_handle, $address) = @_;
> >> +	my ($address) = @_;
> >>
> >> 	# Lookup the given address.
> >> 	my $country_code = &Location::lookup_country_code($db_handle, $address);
> >> @@ -174,9 +177,6 @@ sub get_full_country_name($) {
> >>
> >> # Function to get all available locations.
> >> sub get_locations() {
> >> -	# Create libloc database handle.
> >> -	my $db_handle = &init();
> >> -
> >> 	# Get locations which are stored in the location database.
> >> 	my @database_locations = &Location::database_countries($db_handle);
> >>
> >> @@ -197,9 +197,6 @@ sub address_has_flags($) {
> >> 	# Array to store the flags of the address.
> >> 	my @flags;
> >>
> >> -	# Init libloc database handle.
> >> -	my $db_handle = &init();
> >> -
> >> 	# Loop through the hash of possible network flags.
> >> 	foreach my $flag (keys(%network_flags)) {
> >> 		# Check if the address has the current flag.
> >> --
> >> 2.20.1
> >>
> >>
>
>

Re: [PATCH 1/5] location-functions.pl: Use a single script-wide db_handle.

[Michael Tremer]

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

Hello,

> On 9 Nov 2020, at 12:13, Bernhard Bitsch <Bernhard.Bitsch(a)gmx.de> wrote:
> 
> Hello,
> 
>> Gesendet: Montag, 09. November 2020 um 12:35 Uhr
>> Von: "Michael Tremer" <michael.tremer(a)ipfire.org>
>> An: "Bernhard Bitsch" <Bernhard.Bitsch(a)gmx.de>
>> Cc: development(a)lists.ipfire.org
>> Betreff: Re: [PATCH 1/5] location-functions.pl: Use a single script-wide db_handle.
>> 
>> Hello,
>> 
>> Thank you Stefan for submitting this patchset.
>> 
>>> On 8 Nov 2020, at 19:36, Bernhard Bitsch <Bernhard.Bitsch(a)gmx.de> wrote:
>>> 
>>> This means, we stay with the unbalanced memory allocation in (Perl) libloc. Which leaves a memory leak.
>> 
>> Bernhard, could you please elaborate on how this memory leak is still existing?
>> 
>> I also do not understand what you mean by unbalanced.
>> 
>> As far as I understand the code right now, the database is being opened once and the handle is being stored internally. All functions that are being called will no longer have to hold their own database handle. Therefore the maximum amount of handles open is one.
>> 
> 
> The init() function allocates the handle, which is not really destroyed. The END block is just a safety process. The allocated memory for the handle should be released in the moment of references==0.

The handle is being initialised when location-functions.pl is being loaded and it is freed when the Perl interpreter ends.

It remains referenced all the time.

> Yes, the new code ( inspired by my work-around ) minimizes the number of handles to one. But this handle is persistent also ( this doesn't hurt because it is used persistent for the time of module use ).

It is only being initialised when location-functions.pl is being loaded. That only happens in code that uses those functions.

I do not see the problem with this. Constantly closing and re-opening the database is not an option since the connection tracking list can have tens of thousands of lookups and this will make the page - simply - slow.

I cannot see an option that is more resource-friendly than this.

> 
>>> 
>>>> Gesendet: Samstag, 07. November 2020 um 19:47 Uhr
>>>> Von: "Stefan Schantl" <stefan.schantl(a)ipfire.org>
>>>> An: development(a)lists.ipfire.org
>>>> Betreff: [PATCH 1/5] location-functions.pl: Use a single script-wide db_handle.
>>>> 
>>>> Create and use a single script-wide database handle for libloc to
>>>> prevent from creating multiple ones.
>>>> 
>>>> This helps saving memory, especially on small systems.
>>>> 
>>>> Reference #12515.
>>>> 
>>> 
>>> The error can be produced easily with small memory, but it is present in all systems.
>>> Therefore I've posted this solution as work-around only!
>> 
>> Did you test this patchset or did you come to your conclusion by only reading the code?
>> 
>> Best,
>> -Michael
>> 
> 
> The sentence about "small systems" refers to the original comment. It is not a real memory saving, the patch cures a 'memory wasting'. It is clear, that a massively use of malloc without free crashes a small system faster than a system with big memory resources.

Opening the database requires pretty much nothing (maybe 4K) of RSS memory and about 50MB of VIRT memory. This due to mmap() which helps us reading the database a lot fast. I think this is very very reasonable. The more data is being read from the database the more memory it will use.

If you wish to change that (and that comes with a performance penalty) you could submit a patch that removes mmap() if the user wishes to.

But I do not see why this is a problem at all. The whole system won’t run well on systems with 128 or 256MB of memory. The Linux kernel won’t even boot properly on those any more. Memory is cheap. Our time isn’t.

We have found the most resource-saving way to deal with this now and anything else will simply use considerable amount of time with no benefit to 99% of our users.

Best,
-Michael

> The patchset is mainly my work-around, thus it is in test since I found the error.
> 
> Best,
> -Bernhard
> 
>>> 
>>> - Bernhard
>>> 
>>> 
>>>> Signed-off-by: Stefan Schantl <stefan.schantl(a)ipfire.org>
>>>> ---
>>>> config/cfgroot/location-functions.pl | 11 ++++-------
>>>> 1 file changed, 4 insertions(+), 7 deletions(-)
>>>> 
>>>> diff --git a/config/cfgroot/location-functions.pl b/config/cfgroot/location-functions.pl
>>>> index 2cfe7f908..9b1d0bfb5 100644
>>>> --- a/config/cfgroot/location-functions.pl
>>>> +++ b/config/cfgroot/location-functions.pl
>>>> @@ -55,6 +55,9 @@ our $keyfile = "$location_dir/signing-key.pem";
>>>> # Directory which contains the exported databases.
>>>> our $xt_geoip_db_directory = "/usr/share/xt_geoip/";
>>>> 
>>>> +# Create libloc database handle.
>>>> +my $db_handle = &init();
>>>> +
>>>> #
>>>> ## Tiny function to init the location database.
>>>> #
>>>> @@ -86,7 +89,7 @@ sub verify ($) {
>>>> ## Function to the the country code of a given address.
>>>> #
>>>> sub lookup_country_code($$) {
>>>> -	my ($db_handle, $address) = @_;
>>>> +	my ($address) = @_;
>>>> 
>>>> 	# Lookup the given address.
>>>> 	my $country_code = &Location::lookup_country_code($db_handle, $address);
>>>> @@ -174,9 +177,6 @@ sub get_full_country_name($) {
>>>> 
>>>> # Function to get all available locations.
>>>> sub get_locations() {
>>>> -	# Create libloc database handle.
>>>> -	my $db_handle = &init();
>>>> -
>>>> 	# Get locations which are stored in the location database.
>>>> 	my @database_locations = &Location::database_countries($db_handle);
>>>> 
>>>> @@ -197,9 +197,6 @@ sub address_has_flags($) {
>>>> 	# Array to store the flags of the address.
>>>> 	my @flags;
>>>> 
>>>> -	# Init libloc database handle.
>>>> -	my $db_handle = &init();
>>>> -
>>>> 	# Loop through the hash of possible network flags.
>>>> 	foreach my $flag (keys(%network_flags)) {
>>>> 		# Check if the address has the current flag.
>>>> --
>>>> 2.20.1

Aw: Re: [PATCH 1/5] location-functions.pl: Use a single script-wide db_handle.

[Bernhard Bitsch]

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

Hello,

> Gesendet: Montag, 09. November 2020 um 15:04 Uhr
> Von: "Michael Tremer" <michael.tremer(a)ipfire.org>
> An: "Bernhard Bitsch" <Bernhard.Bitsch(a)gmx.de>
> Cc: development(a)lists.ipfire.org
> Betreff: Re: [PATCH 1/5] location-functions.pl: Use a single script-wide db_handle.
>
> Hello,
> 
> > On 9 Nov 2020, at 12:13, Bernhard Bitsch <Bernhard.Bitsch(a)gmx.de> wrote:
> > 
> > Hello,
> > 
> >> Gesendet: Montag, 09. November 2020 um 12:35 Uhr
> >> Von: "Michael Tremer" <michael.tremer(a)ipfire.org>
> >> An: "Bernhard Bitsch" <Bernhard.Bitsch(a)gmx.de>
> >> Cc: development(a)lists.ipfire.org
> >> Betreff: Re: [PATCH 1/5] location-functions.pl: Use a single script-wide db_handle.
> >> 
> >> Hello,
> >> 
> >> Thank you Stefan for submitting this patchset.
> >> 
> >>> On 8 Nov 2020, at 19:36, Bernhard Bitsch <Bernhard.Bitsch(a)gmx.de> wrote:
> >>> 
> >>> This means, we stay with the unbalanced memory allocation in (Perl) libloc. Which leaves a memory leak.
> >> 
> >> Bernhard, could you please elaborate on how this memory leak is still existing?
> >> 
> >> I also do not understand what you mean by unbalanced.
> >> 
> >> As far as I understand the code right now, the database is being opened once and the handle is being stored internally. All functions that are being called will no longer have to hold their own database handle. Therefore the maximum amount of handles open is one.
> >> 
> > 
> > The init() function allocates the handle, which is not really destroyed. The END block is just a safety process. The allocated memory for the handle should be released in the moment of references==0.
> 
> The handle is being initialised when location-functions.pl is being loaded and it is freed when the Perl interpreter ends.
> 
> It remains referenced all the time.
> 

This the new state!
The init() function builds a memory block to access the location data base and returns a handle to it. This memory block is not released by the memory management. I suppose there are some deficiencies in handling the reference counters.
A call to init() in each function of Location::Functions leaves this memory block. That was the originatings issue.

> > Yes, the new code ( inspired by my work-around ) minimizes the number of handles to one. But this handle is persistent also ( this doesn't hurt because it is used persistent for the time of module use ).
> 
> It is only being initialised when location-functions.pl is being loaded. That only happens in code that uses those functions.
> 
> I do not see the problem with this. Constantly closing and re-opening the database is not an option since the connection tracking list can have tens of thousands of lookups and this will make the page - simply - slow.
> 
> I cannot see an option that is more resource-friendly than this.
> 

It is not the open/close of the data base which produces the memory leak, but the Perl interface ( see above ). 
In terms of performance, you are right. So my "mini" system showed as the way to a better performing system.

> > 
> >>> 
> >>>> Gesendet: Samstag, 07. November 2020 um 19:47 Uhr
> >>>> Von: "Stefan Schantl" <stefan.schantl(a)ipfire.org>
> >>>> An: development(a)lists.ipfire.org
> >>>> Betreff: [PATCH 1/5] location-functions.pl: Use a single script-wide db_handle.
> >>>> 
> >>>> Create and use a single script-wide database handle for libloc to
> >>>> prevent from creating multiple ones.
> >>>> 
> >>>> This helps saving memory, especially on small systems.
> >>>> 
> >>>> Reference #12515.
> >>>> 
> >>> 
> >>> The error can be produced easily with small memory, but it is present in all systems.
> >>> Therefore I've posted this solution as work-around only!
> >> 
> >> Did you test this patchset or did you come to your conclusion by only reading the code?
> >> 
> >> Best,
> >> -Michael
> >> 
> > 
> > The sentence about "small systems" refers to the original comment. It is not a real memory saving, the patch cures a 'memory wasting'. It is clear, that a massively use of malloc without free crashes a small system faster than a system with big memory resources.
> 
> Opening the database requires pretty much nothing (maybe 4K) of RSS memory and about 50MB of VIRT memory. This due to mmap() which helps us reading the database a lot fast. I think this is very very reasonable. The more data is being read from the database the more memory it will use.
> 
> If you wish to change that (and that comes with a performance penalty) you could submit a patch that removes mmap() if the user wishes to.
> 
> But I do not see why this is a problem at all. The whole system won’t run well on systems with 128 or 256MB of memory. The Linux kernel won’t even boot properly on those any more. Memory is cheap. Our time isn’t.
> 
> We have found the most resource-saving way to deal with this now and anything else will simply use considerable amount of time with no benefit to 99% of our users.
> 
> Best,
> -Michael
> 

Further I think, we have found the most straight-forward way to interface to libloc ( which is a great work! ). The first approach remembered me a bit at good old times when M$ took MS-DOS and some code upon and called it Windows. More and more layers put on this building requested faster processors and more memory, without real gain in functionality. Just the experience of an "old" computer scientist, which saw each step in this evolution. ;)

Best,
-Bernhard
> > The patchset is mainly my work-around, thus it is in test since I found the error.
> > 
> > Best,
> > -Bernhard
> > 
> >>> 
> >>> - Bernhard
> >>> 
> >>> 
> >>>> Signed-off-by: Stefan Schantl <stefan.schantl(a)ipfire.org>
> >>>> ---
> >>>> config/cfgroot/location-functions.pl | 11 ++++-------
> >>>> 1 file changed, 4 insertions(+), 7 deletions(-)
> >>>> 
> >>>> diff --git a/config/cfgroot/location-functions.pl b/config/cfgroot/location-functions.pl
> >>>> index 2cfe7f908..9b1d0bfb5 100644
> >>>> --- a/config/cfgroot/location-functions.pl
> >>>> +++ b/config/cfgroot/location-functions.pl
> >>>> @@ -55,6 +55,9 @@ our $keyfile = "$location_dir/signing-key.pem";
> >>>> # Directory which contains the exported databases.
> >>>> our $xt_geoip_db_directory = "/usr/share/xt_geoip/";
> >>>> 
> >>>> +# Create libloc database handle.
> >>>> +my $db_handle = &init();
> >>>> +
> >>>> #
> >>>> ## Tiny function to init the location database.
> >>>> #
> >>>> @@ -86,7 +89,7 @@ sub verify ($) {
> >>>> ## Function to the the country code of a given address.
> >>>> #
> >>>> sub lookup_country_code($$) {
> >>>> -	my ($db_handle, $address) = @_;
> >>>> +	my ($address) = @_;
> >>>> 
> >>>> 	# Lookup the given address.
> >>>> 	my $country_code = &Location::lookup_country_code($db_handle, $address);
> >>>> @@ -174,9 +177,6 @@ sub get_full_country_name($) {
> >>>> 
> >>>> # Function to get all available locations.
> >>>> sub get_locations() {
> >>>> -	# Create libloc database handle.
> >>>> -	my $db_handle = &init();
> >>>> -
> >>>> 	# Get locations which are stored in the location database.
> >>>> 	my @database_locations = &Location::database_countries($db_handle);
> >>>> 
> >>>> @@ -197,9 +197,6 @@ sub address_has_flags($) {
> >>>> 	# Array to store the flags of the address.
> >>>> 	my @flags;
> >>>> 
> >>>> -	# Init libloc database handle.
> >>>> -	my $db_handle = &init();
> >>>> -
> >>>> 	# Loop through the hash of possible network flags.
> >>>> 	foreach my $flag (keys(%network_flags)) {
> >>>> 		# Check if the address has the current flag.
> >>>> --
> >>>> 2.20.1
> 
>

Re: [PATCH 1/5] location-functions.pl: Use a single script-wide db_handle.

[Michael Tremer]

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

Hello,

> On 9 Nov 2020, at 15:16, Bernhard Bitsch <Bernhard.Bitsch(a)gmx.de> wrote:
> 
> Hello,
> 
>> Gesendet: Montag, 09. November 2020 um 15:04 Uhr
>> Von: "Michael Tremer" <michael.tremer(a)ipfire.org>
>> An: "Bernhard Bitsch" <Bernhard.Bitsch(a)gmx.de>
>> Cc: development(a)lists.ipfire.org
>> Betreff: Re: [PATCH 1/5] location-functions.pl: Use a single script-wide db_handle.
>> 
>> Hello,
>> 
>>> On 9 Nov 2020, at 12:13, Bernhard Bitsch <Bernhard.Bitsch(a)gmx.de> wrote:
>>> 
>>> Hello,
>>> 
>>>> Gesendet: Montag, 09. November 2020 um 12:35 Uhr
>>>> Von: "Michael Tremer" <michael.tremer(a)ipfire.org>
>>>> An: "Bernhard Bitsch" <Bernhard.Bitsch(a)gmx.de>
>>>> Cc: development(a)lists.ipfire.org
>>>> Betreff: Re: [PATCH 1/5] location-functions.pl: Use a single script-wide db_handle.
>>>> 
>>>> Hello,
>>>> 
>>>> Thank you Stefan for submitting this patchset.
>>>> 
>>>>> On 8 Nov 2020, at 19:36, Bernhard Bitsch <Bernhard.Bitsch(a)gmx.de> wrote:
>>>>> 
>>>>> This means, we stay with the unbalanced memory allocation in (Perl) libloc. Which leaves a memory leak.
>>>> 
>>>> Bernhard, could you please elaborate on how this memory leak is still existing?
>>>> 
>>>> I also do not understand what you mean by unbalanced.
>>>> 
>>>> As far as I understand the code right now, the database is being opened once and the handle is being stored internally. All functions that are being called will no longer have to hold their own database handle. Therefore the maximum amount of handles open is one.
>>>> 
>>> 
>>> The init() function allocates the handle, which is not really destroyed. The END block is just a safety process. The allocated memory for the handle should be released in the moment of references==0.
>> 
>> The handle is being initialised when location-functions.pl is being loaded and it is freed when the Perl interpreter ends.
>> 
>> It remains referenced all the time.
>> 
> 
> This the new state!
> The init() function builds a memory block to access the location data base and returns a handle to it. This memory block is not released by the memory management. I suppose there are some deficiencies in handling the reference counters.

Okay, where are those?

Libloc internally uses reference counting and we have some unit tests to check if those are working okay.

If you know that we are handling this somewhere incorrectly, let me know and we will be able to reduce the footprint of libloc even more.

> A call to init() in each function of Location::Functions leaves this memory block. That was the originatings issue.
> 
>>> Yes, the new code ( inspired by my work-around ) minimizes the number of handles to one. But this handle is persistent also ( this doesn't hurt because it is used persistent for the time of module use ).
>> 
>> It is only being initialised when location-functions.pl is being loaded. That only happens in code that uses those functions.
>> 
>> I do not see the problem with this. Constantly closing and re-opening the database is not an option since the connection tracking list can have tens of thousands of lookups and this will make the page - simply - slow.
>> 
>> I cannot see an option that is more resource-friendly than this.
>> 
> 
> It is not the open/close of the data base which produces the memory leak, but the Perl interface ( see above ). 

Okay, but where? In which function?

> In terms of performance, you are right. So my "mini" system showed as the way to a better performing system.
> 
>>> 
>>>>> 
>>>>>> Gesendet: Samstag, 07. November 2020 um 19:47 Uhr
>>>>>> Von: "Stefan Schantl" <stefan.schantl(a)ipfire.org>
>>>>>> An: development(a)lists.ipfire.org
>>>>>> Betreff: [PATCH 1/5] location-functions.pl: Use a single script-wide db_handle.
>>>>>> 
>>>>>> Create and use a single script-wide database handle for libloc to
>>>>>> prevent from creating multiple ones.
>>>>>> 
>>>>>> This helps saving memory, especially on small systems.
>>>>>> 
>>>>>> Reference #12515.
>>>>>> 
>>>>> 
>>>>> The error can be produced easily with small memory, but it is present in all systems.
>>>>> Therefore I've posted this solution as work-around only!
>>>> 
>>>> Did you test this patchset or did you come to your conclusion by only reading the code?
>>>> 
>>>> Best,
>>>> -Michael
>>>> 
>>> 
>>> The sentence about "small systems" refers to the original comment. It is not a real memory saving, the patch cures a 'memory wasting'. It is clear, that a massively use of malloc without free crashes a small system faster than a system with big memory resources.
>> 
>> Opening the database requires pretty much nothing (maybe 4K) of RSS memory and about 50MB of VIRT memory. This due to mmap() which helps us reading the database a lot fast. I think this is very very reasonable. The more data is being read from the database the more memory it will use.
>> 
>> If you wish to change that (and that comes with a performance penalty) you could submit a patch that removes mmap() if the user wishes to.
>> 
>> But I do not see why this is a problem at all. The whole system won’t run well on systems with 128 or 256MB of memory. The Linux kernel won’t even boot properly on those any more. Memory is cheap. Our time isn’t.
>> 
>> We have found the most resource-saving way to deal with this now and anything else will simply use considerable amount of time with no benefit to 99% of our users.
>> 
>> Best,
>> -Michael
>> 
> 
> Further I think, we have found the most straight-forward way to interface to libloc ( which is a great work! ). The first approach remembered me a bit at good old times when M$ took MS-DOS and some code upon and called it Windows. More and more layers put on this building requested faster processors and more memory, without real gain in functionality. Just the experience of an "old" computer scientist, which saw each step in this evolution. ;)

To be totally honest, we wanted to keep the Perl interface as easy as possible. The Python interface is much easier to integrate and will gain many more users. Perl is on its way out, but of course the current web user interface is written in perl. We started with only a few functions but it grew quickly and therefore it became messy.

The native perl part is only there because the perl C bindings are less fun to use than putting hot wax into my eyes.

-Michael

> 
> Best,
> -Bernhard
>>> The patchset is mainly my work-around, thus it is in test since I found the error.
>>> 
>>> Best,
>>> -Bernhard
>>> 
>>>>> 
>>>>> - Bernhard
>>>>> 
>>>>> 
>>>>>> Signed-off-by: Stefan Schantl <stefan.schantl(a)ipfire.org>
>>>>>> ---
>>>>>> config/cfgroot/location-functions.pl | 11 ++++-------
>>>>>> 1 file changed, 4 insertions(+), 7 deletions(-)
>>>>>> 
>>>>>> diff --git a/config/cfgroot/location-functions.pl b/config/cfgroot/location-functions.pl
>>>>>> index 2cfe7f908..9b1d0bfb5 100644
>>>>>> --- a/config/cfgroot/location-functions.pl
>>>>>> +++ b/config/cfgroot/location-functions.pl
>>>>>> @@ -55,6 +55,9 @@ our $keyfile = "$location_dir/signing-key.pem";
>>>>>> # Directory which contains the exported databases.
>>>>>> our $xt_geoip_db_directory = "/usr/share/xt_geoip/";
>>>>>> 
>>>>>> +# Create libloc database handle.
>>>>>> +my $db_handle = &init();
>>>>>> +
>>>>>> #
>>>>>> ## Tiny function to init the location database.
>>>>>> #
>>>>>> @@ -86,7 +89,7 @@ sub verify ($) {
>>>>>> ## Function to the the country code of a given address.
>>>>>> #
>>>>>> sub lookup_country_code($$) {
>>>>>> -	my ($db_handle, $address) = @_;
>>>>>> +	my ($address) = @_;
>>>>>> 
>>>>>> 	# Lookup the given address.
>>>>>> 	my $country_code = &Location::lookup_country_code($db_handle, $address);
>>>>>> @@ -174,9 +177,6 @@ sub get_full_country_name($) {
>>>>>> 
>>>>>> # Function to get all available locations.
>>>>>> sub get_locations() {
>>>>>> -	# Create libloc database handle.
>>>>>> -	my $db_handle = &init();
>>>>>> -
>>>>>> 	# Get locations which are stored in the location database.
>>>>>> 	my @database_locations = &Location::database_countries($db_handle);
>>>>>> 
>>>>>> @@ -197,9 +197,6 @@ sub address_has_flags($) {
>>>>>> 	# Array to store the flags of the address.
>>>>>> 	my @flags;
>>>>>> 
>>>>>> -	# Init libloc database handle.
>>>>>> -	my $db_handle = &init();
>>>>>> -
>>>>>> 	# Loop through the hash of possible network flags.
>>>>>> 	foreach my $flag (keys(%network_flags)) {
>>>>>> 		# Check if the address has the current flag.
>>>>>> --
>>>>>> 2.20.1

Aw: [PATCH 5/5] Adjust CGI files to work with latest location-function.pl changes.

[Bernhard Bitsch]

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

country.cgi should use Location::Functions::get_locations()

> Gesendet: Samstag, 07. November 2020 um 19:47 Uhr
> Von: "Stefan Schantl" <stefan.schantl(a)ipfire.org>
> An: development(a)lists.ipfire.org
> Betreff: [PATCH 5/5] Adjust CGI files to work with latest location-function.pl changes.
>
> Fixes #12515.
>
> Signed-off-by: Stefan Schantl <stefan.schantl(a)ipfire.org>
...
> diff --git a/html/cgi-bin/country.cgi b/html/cgi-bin/country.cgi
> index b519d89b3..b1c72bb22 100644
> --- a/html/cgi-bin/country.cgi
> +++ b/html/cgi-bin/country.cgi
> @@ -52,11 +52,8 @@ print<<END;
>  	</tr>
>  END
>
> -# Init libloc database connection.
> -my $db_handle = &Location::Functions::init();
> -
>  # Get a list of all supported country codes.
> -my @countries = &Location::database_countries($db_handle);
> +my @countries = &Location::database_countries();
>

+my @countries = Location::Funvtions:get_locations();

>  # Loop through whole country list.
>  foreach my $country (@countries) {
> diff --git a/html/cgi-bin/dns.cgi b/html/cgi-bin/dns.cgi
> index 13dd9d6a0..762e77ff1 100755
> --- a/html/cgi-bin/dns.cgi
> +++ b/html/cgi-bin/dns.cgi
> @@ -269,9 +269,6 @@ my %dns_servers = ();
>  # Read-in config file.
>  &General::readhasharray("$servers_file", \%dns_servers);
>
> -# Libloc database handle
> -my $libloc_db_handle = &Location::Functions::init();
> -
>  &Header::openpage($Lang::tr{'dns title'}, 1, '');
>
>  &Header::openbigbox('100%', 'left', '', $errormessage);
> @@ -598,7 +595,7 @@ END
>  				}
>
>  				# collect more information about name server (rDNS, country code)
> -				my $ccode = &Location::Functions::lookup_country_code($libloc_db_handle, $nameserver);
> +				my $ccode = &Location::Functions::lookup_country_code($nameserver);
>  				my $flag_icon = &Location::Functions::get_flag_icon($ccode);
>
>  				my $rdns;
> diff --git a/html/cgi-bin/ipinfo.cgi b/html/cgi-bin/ipinfo.cgi
> index d8cb6c6b7..ecc7c436c 100644
> --- a/html/cgi-bin/ipinfo.cgi
> +++ b/html/cgi-bin/ipinfo.cgi
> @@ -62,12 +62,11 @@ if (&General::validip($addr)) {
>  	if (!$hostname) { $hostname = $Lang::tr{'lookup failed'}; }
>
>  	# enumerate location information for IP address...
> -	my $db_handle = &Location::Functions::init();
> -	my $ccode = &Location::Functions::lookup_country_code($db_handle, $addr);
> +	my $ccode = &Location::Functions::lookup_country_code($addr);
>  	my @network_flags = &Location::Functions::address_has_flags($addr);
>
>  	# Try to get the continent of the country code.
> -	my $continent = &Location::get_continent_code($db_handle, $ccode);
> +	my $continent = &Location::Functions::get_continent_code($ccode);
>
>  	# Check if a whois server for the continent is known.
>  	if($whois_servers_by_continent{$continent}) {
> diff --git a/html/cgi-bin/logs.cgi/firewalllog.dat b/html/cgi-bin/logs.cgi/firewalllog.dat
> index 2b690e35b..361bf0432 100644
> --- a/html/cgi-bin/logs.cgi/firewalllog.dat
> +++ b/html/cgi-bin/logs.cgi/firewalllog.dat
> @@ -24,9 +24,6 @@ require "${General::swroot}/location-functions.pl";
>  require "${General::swroot}/lang.pl";
>  require "${General::swroot}/header.pl";
>
> -# Libloc database handle.
> -my $libloc_db_handle = &Location::Functions::init();
> -
>  my %color = ();
>  my %mainsettings = ();
>  &General::readhash("${General::swroot}/main/settings", \%mainsettings);
> @@ -354,7 +351,8 @@ foreach $_ (@log)
>  	$srcport=$1 if $packet =~ /SPT=(\d+)/;
>  	$dstport=$1 if $packet =~ /DPT=(\d+)/;
>
> -	my $ccode = &Location::Functions::lookup_country_code($libloc_db_handle, $srcaddr);
> +	# Get the country code.
> +	my $ccode = &Location::Functions::lookup_country_code($srcaddr);
>
>  	my $servi = uc(getservbyport($srcport, lc($proto)));
>  	if ($servi ne '' && $srcport < 1024) {
> diff --git a/html/cgi-bin/logs.cgi/firewalllogcountry.dat b/html/cgi-bin/logs.cgi/firewalllogcountry.dat
> index 701abab2c..e3901b945 100644
> --- a/html/cgi-bin/logs.cgi/firewalllogcountry.dat
> +++ b/html/cgi-bin/logs.cgi/firewalllogcountry.dat
> @@ -22,9 +22,6 @@ require "${General::swroot}/location-functions.pl";
>  require "${General::swroot}/lang.pl";
>  require "${General::swroot}/header.pl";
>
> -# Libloc database handle.
> -my $libloc_db_handle = &Location::Functions::init();
> -
>  use POSIX();
>
>  my %cgiparams=();
> @@ -311,7 +308,7 @@ foreach $_ (@log)
>  		# Traffic from red
>  		if($srcaddr ne '') {
>  			# srcaddr is set
> -			my $ccode = &Location::Functions::lookup_country_code($libloc_db_handle, $srcaddr);
> +			my $ccode = &Location::Functions::lookup_country_code($srcaddr);
>  			if ($ccode eq '') {
>  				$ccode = 'unknown';
>  			}
> diff --git a/html/cgi-bin/logs.cgi/firewalllogip.dat b/html/cgi-bin/logs.cgi/firewalllogip.dat
> index 670d72a52..6de4081af 100644
> --- a/html/cgi-bin/logs.cgi/firewalllogip.dat
> +++ b/html/cgi-bin/logs.cgi/firewalllogip.dat
> @@ -22,9 +22,6 @@ require "${General::swroot}/location-functions.pl";
>  require "${General::swroot}/lang.pl";
>  require "${General::swroot}/header.pl";
>
> -# Libloc database handle.
> -my $libloc_db_handle = &Location::Functions::init();
> -
>  use POSIX();
>
>  my %cgiparams=();
> @@ -438,7 +435,8 @@ for($s=0;$s<$lines;$s++)
>  	$col="bgcolor='$color{\"color$colorIndex\"}'";
>  	print "<tr>";
>
> -	my $ccode = &Location::Functions::lookup_country_code($libloc_db_handle, $key[$s]);
> +	# Get country code.
> +	my $ccode = &Location::Functions::lookup_country_code($key[$s]);
>
>  	$color++;
>  	print "<td align='center' $col><form method='post' action='showrequestfromip.dat'><input type='hidden' name='MONTH' value='$cgiparams{'MONTH'}'> <input type='hidden' name='DAY' value='$cgiparams{'DAY'}'> <input type='hidden' name='ip' value='$key[$s]'> <input type='submit' value='$Lang::tr{'details'}'></form></td>";
> diff --git a/html/cgi-bin/logs.cgi/showrequestfromcountry.dat b/html/cgi-bin/logs.cgi/showrequestfromcountry.dat
> index 4d80e77a4..2a246ec60 100644
> --- a/html/cgi-bin/logs.cgi/showrequestfromcountry.dat
> +++ b/html/cgi-bin/logs.cgi/showrequestfromcountry.dat
> @@ -19,9 +19,6 @@ require "${General::swroot}/location-functions.pl";
>  require "${General::swroot}/lang.pl";
>  require "${General::swroot}/header.pl";
>
> -# Libloc database handle.
> -my $libloc_db_handle = &Location::Functions::init();
> -
>  use POSIX();
>
>  #workaround to suppress a warning when a variable is used only once
> @@ -181,7 +178,7 @@ if (!$skip)
>  			}
>  			elsif($srcaddr ne '') {
>  				# or srcaddr matches country code
> -				my $ccode = &Location::Functions::lookup_country_code($libloc_db_handle, $srcaddr);
> +				my $ccode = &Location::Functions::lookup_country_code($srcaddr);
>  				if($ccode eq uc($country)){
>  					$log[$lines] = $_;
>  					$lines++;
> @@ -352,7 +349,7 @@ foreach $_ (@slice)
>  	if($iface eq $country || $srcaddr ne '') {
>  		my $ccode='';
>  		if($iface ne $country) {
> -			$ccode = &Location::Functions::lookup_country_code($libloc_db_handle, $srcaddr);
> +			$ccode = &Location::Functions::lookup_country_code($srcaddr);
>  		}
>  		if($iface eq $country || $ccode eq uc($country)) {
>  			my $chain = '';
> diff --git a/html/cgi-bin/ovpnmain.cgi b/html/cgi-bin/ovpnmain.cgi
> index e7bc505e7..8626a94ca 100644
> --- a/html/cgi-bin/ovpnmain.cgi
> +++ b/html/cgi-bin/ovpnmain.cgi
> @@ -3002,9 +3002,6 @@ END
>  	&Header::openbigbox('100%', 'LEFT', '', '');
>      &Header::openbox('100%', 'LEFT', $Lang::tr{'ovpn con stat'});
>
> -    # Libloc database handle.
> -    my $libloc_db_handle = &Location::Functions::init();
> -
>  #
>  #	<td><b>$Lang::tr{'protocol'}</b></td>
>  # protocol temp removed
> @@ -3055,7 +3052,7 @@ END
>  		    $users[$uid]{'Proto'} = $proto;
>
>  		    # get country code for "RealAddress"...
> -		    my $ccode = &Location::Functions::lookup_country_code($libloc_db_handle, (split ':', $users[$uid]{'RealAddress'})[0]);
> +		    my $ccode = &Location::Functions::lookup_country_code((split ':', $users[$uid]{'RealAddress'})[0]);
>  		    my $flag_icon = &Location::Functions::get_flag_icon($ccode);
>  		    $users[$uid]{'Country'} = "<a href='country.cgi#$ccode'><img src='$flag_icon' border='0' align='absmiddle' alt='$ccode' title='$ccode' /></a>";
>  		    $uid++;
> diff --git a/html/cgi-bin/remote.cgi b/html/cgi-bin/remote.cgi
> index 9c742669b..a27e10de9 100644
> --- a/html/cgi-bin/remote.cgi
> +++ b/html/cgi-bin/remote.cgi
> @@ -277,10 +277,6 @@ sub printactivelogins()
>  		print "<tr bgcolor='$table_colour'><td colspan='5'>$Lang::tr{'ssh no active logins'}</td></tr>\n";
>  	} else {
>  		# list active logins...
> -
> -		# Libloc database handle.
> -		my $libloc_db_handle = &Location::Functions::init();
> -
>  		foreach my $line (@output)
>  		{
>  			my @arry = split(/\ +/, $line);
> @@ -291,7 +287,7 @@ sub printactivelogins()
>  			$remoteip =~ s/[()]//g;
>
>  			# display more information about that IP adress...
> -			my $ccode = &Location::Functions::lookup_country_code($libloc_db_handle, $remoteip);
> +			my $ccode = &Location::Functions::lookup_country_code($remoteip);
>  			my $flag_icon = &Location::Functions::get_flag_icon($ccode);
>
>  			# get rDNS...
> diff --git a/html/cgi-bin/tor.cgi b/html/cgi-bin/tor.cgi
> index 14bfcfe90..983bb30c9 100644
> --- a/html/cgi-bin/tor.cgi
> +++ b/html/cgi-bin/tor.cgi
> @@ -30,9 +30,6 @@ require "${General::swroot}/location-functions.pl";
>  require "${General::swroot}/lang.pl";
>  require "${General::swroot}/header.pl";
>
> -# Init libloc database connection.
> -my $db_handle = &Location::Functions::init();
> -
>  #workaround to suppress a warning when a variable is used only once
>  my @dummy = ( ${Header::colouryellow} );
>  undef (@dummy);
> @@ -322,7 +319,7 @@ END
>  					<select name='TOR_EXIT_COUNTRY'>
>  						<option value=''>- $Lang::tr{'tor exit country any'} -</option>
>  END
> -		my @country_codes = &Location::database_countries($db_handle);
> +		my @country_codes = &Location::Functions::get_locations("no_special_locations");
>  		foreach my $country_code (@country_codes) {
>  			# Convert country code into upper case format.
>  			$country_code = uc($country_code);
> @@ -912,7 +909,7 @@ sub TorNodeDescription() {
>  			$node->{'address'} = $3;
>  			$node->{'port'}    = $4;
>
> -			my $country_code = &Location::Functions::lookup_country_code($db_handle, $node->{'address'});
> +			my $country_code = &Location::Functions::lookup_country_code($node->{'address'});
>  			$node->{'country_code'} = $country_code;
>
>  		# Flags
> --
> 2.20.1
>
>