From mboxrd@z Thu Jan  1 00:00:00 1970
From: Bernhard Bitsch <Bernhard.Bitsch@gmx.de>
To: development@lists.ipfire.org
Subject: Aw: Re: [PATCH 1/5] location-functions.pl: Use a single script-wide
 db_handle.
Date: Mon, 09 Nov 2020 13:13:09 +0100
Message-ID:
 <trinity-288f6ee2-6121-43a1-b303-28876bf25c1a-1604923989908@3c-app-gmx-bap38>
In-Reply-To: <F718D09B-0F25-4F23-9A0D-CCBAF8EC376E@ipfire.org>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="===============1032260398782017309=="
List-Id: <development.lists.ipfire.org>

--===============1032260398782017309==
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

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) liblo=
c. Which leaves a memory leak.
>
> Bernhard, could you please elaborate on how this memory leak is still exist=
ing?
>
> I also do not understand what you mean by unbalanced.
>
> As far as I understand the code right now, the database is being opened onc=
e and the handle is being stored internally. All functions that are being cal=
led will no longer have to hold their own database handle. Therefore the maxi=
mum 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 shoul=
d be released in the moment of references=3D=3D0.
Yes, the new code ( inspired by my work-around ) minimizes the number of hand=
les 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 readi=
ng 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 sy=
stem with big memory resources.
The patchset is mainly my work-around, thus it is in test since I found the e=
rror.

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/locat=
ion-functions.pl
> >> index 2cfe7f908..9b1d0bfb5 100644
> >> --- a/config/cfgroot/location-functions.pl
> >> +++ b/config/cfgroot/location-functions.pl
> >> @@ -55,6 +55,9 @@ our $keyfile =3D "$location_dir/signing-key.pem";
> >> # Directory which contains the exported databases.
> >> our $xt_geoip_db_directory =3D "/usr/share/xt_geoip/";
> >>
> >> +# Create libloc database handle.
> >> +my $db_handle =3D &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) =3D @_;
> >> +	my ($address) =3D @_;
> >>
> >> 	# Lookup the given address.
> >> 	my $country_code =3D &Location::lookup_country_code($db_handle, $addres=
s);
> >> @@ -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 =3D &init();
> >> -
> >> 	# Get locations which are stored in the location database.
> >> 	my @database_locations =3D &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 =3D &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
> >>
> >>
>
>

--===============1032260398782017309==--