From mboxrd@z Thu Jan  1 00:00:00 1970
From: Leo Hofmann <hofmann@leo-andres.de>
To: development@lists.ipfire.org
Subject: Re: [PATCH 1/4] pakfire.cgi: Extend the lockfile test
Date: Thu, 02 Dec 2021 17:09:16 +0100
Message-ID: <d2df1733-bb79-a937-e865-eb5d7d52ac3b@leo-andres.de>
In-Reply-To: <A3CA3CFD-7E50-4159-8A89-E2F6D566572E@ipfire.org>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="===============5776242209519101684=="
List-Id: <development.lists.ipfire.org>

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

Hi,

Thank you for checking the patches!

Am 02.12.2021 um 16:52 schrieb Michael Tremer:
> Hello,
>
> This patch looks good to me. I only have a small thought...
>
>> On 2 Dec 2021, at 15:39, Leo-Andres Hofmann <hofmann(a)leo-andres.de> wrot=
e:
>>
>> This implements a function to determine if Pakfire is already running.
>> It tests the PID and lockfile and can be expanded easily later.
>> 'pidof' checks the full path to avoid confusion.
>>
>> Removes the unreachable function "refreshpage".
>>
>> Signed-off-by: Leo-Andres Hofmann <hofmann(a)leo-andres.de>
>> ---
>> html/cgi-bin/pakfire.cgi | 30 ++++++++++++++++++------------
>> 1 file changed, 18 insertions(+), 12 deletions(-)
>>
>> diff --git a/html/cgi-bin/pakfire.cgi b/html/cgi-bin/pakfire.cgi
>> index 4d6eee284..7957bc154 100644
>> --- a/html/cgi-bin/pakfire.cgi
>> +++ b/html/cgi-bin/pakfire.cgi
>> @@ -44,8 +44,6 @@ $cgiparams{'VALID'} =3D '';
>> $cgiparams{'INSPAKS'} =3D '';
>> $cgiparams{'DELPAKS'} =3D '';
>>
>> -sub refreshpage{&Header::openbox( 'Waiting', 1, "<meta http-equiv=3D'refr=
esh' content=3D'1;'>" );print "<center><img src=3D'/images/clock.gif' alt=3D'=
' /><br/><font color=3D'red'>$Lang::tr{'pagerefresh'}</font></center>";&Heade=
r::closebox();}
>> -
>> &Header::getcgihash(\%cgiparams);
>>
>> &General::readhash("${General::swroot}/main/settings", \%mainsettings);
>> @@ -54,7 +52,7 @@ sub refreshpage{&Header::openbox( 'Waiting', 1, "<meta h=
ttp-equiv=3D'refresh' cont
>> &Header::openpage($Lang::tr{'pakfire configuration'}, 1);
>> &Header::openbigbox('100%', 'left', '', $errormessage);
>>
>> -if (($cgiparams{'ACTION'} eq 'install') && (! -e $Pakfire::lockfile)) {
>> +if (($cgiparams{'ACTION'} eq 'install') && (! &_is_pakfire_busy())) {
>> 	my @pkgs =3D split(/\|/, $cgiparams{'INSPAKS'});
>> 	if ("$cgiparams{'FORCE'}" eq "on") {
>> 		&General::system_background("/usr/local/bin/pakfire", "install", "--non-=
interactive", "--no-colors", @pkgs);
>> @@ -92,7 +90,7 @@ END
>> 		&Header::closepage();
>> 		exit;
>> 	}
>> -} elsif (($cgiparams{'ACTION'} eq 'remove') && (! -e $Pakfire::lockfile))=
 {
>> +} elsif (($cgiparams{'ACTION'} eq 'remove') && (! &_is_pakfire_busy())) {
>> 	my @pkgs =3D split(/\|/, $cgiparams{'DELPAKS'});
>> 	if ("$cgiparams{'FORCE'}" eq "on") {
>> 		&General::system_background("/usr/local/bin/pakfire", "remove", "--non-i=
nteractive", "--no-colors", @pkgs);
>> @@ -131,10 +129,10 @@ END
>> 		exit;
>> 	}
>>
>> -} elsif (($cgiparams{'ACTION'} eq 'update') && (! -e $Pakfire::lockfile))=
 {
>> +} elsif (($cgiparams{'ACTION'} eq 'update') && (! &_is_pakfire_busy())) {
>> 	&General::system_background("/usr/local/bin/pakfire", "update", "--force"=
, "--no-colors");
>> 	sleep(1);
>> -} elsif (($cgiparams{'ACTION'} eq 'upgrade') && (!-e $Pakfire::lockfile))=
 {
>> +} elsif (($cgiparams{'ACTION'} eq 'upgrade') && (! &_is_pakfire_busy())) {
>> 	&General::system_background("/usr/local/bin/pakfire", "upgrade", "-y", "-=
-no-colors");
>> 	sleep(1);
>> } elsif ($cgiparams{'ACTION'} eq "$Lang::tr{'save'}") {
>> @@ -173,11 +171,7 @@ if ($errormessage) {
>> }
>>
>> # Check if pakfire is already running.
>> -#
>> -# The system backpipe command is safe, because no user input is computed.
>> -my $pid =3D `pidof pakfire`;
>> -
>> -if ($pid) {
>> +if (&_is_pakfire_busy()) {
>> 	&Header::openbox( 'Waiting', 1, "<meta http-equiv=3D'refresh' content=3D'=
10;'>" );
>> 	print <<END;
>> 	<table>
>> @@ -203,7 +197,6 @@ END
>> 	&Header::closebigbox();
>> 	&Header::closepage();
>> 	exit;
>> -	refreshpage();
>> }
>>
>> my $core_release =3D `cat /opt/pakfire/db/core/mine 2>/dev/null`;
>> @@ -314,3 +307,16 @@ END
>> &Header::closebox();
>> &Header::closebigbox();
>> &Header::closepage();
>> +
>> +###--- Internal functions ---###
>> +
>> +# Check if pakfire is already running (extend test here if necessary)
>> +sub _is_pakfire_busy {
>> +	# Get PID of a running pakfire instance
>> +	# (The system backpipe command is safe, because no user input is compute=
d.)
>> +	my $pakfire_pid =3D `pidof -s /usr/local/bin/pakfire`;
>> +	chomp($pakfire_pid);
>> +
>> +	# Test presence of PID or lockfile
>> +	return (($pakfire_pid) || (-e "$Pakfire::lockfile"));
> As we would normally expect the lock file, it might make sense to move this=
 first as it will be a lot more efficient to check for a file than launching =
a sub-process.

That makes sense, I'll split it into two "return" statements.

Regards
Leo

>
> -Michael
>
>> +}
>> --=20
>> 2.27.0.windows.1
>>

--===============5776242209519101684==--