[1/4] pakfire.cgi: Extend the lockfile test

Message ID 20211202153955.1126-1-hofmann@leo-andres.de
State Accepted
Commit d255e2d1c291915ca20e0f57f9d55bc0d7d8dd87
Headers show
Series [1/4] pakfire.cgi: Extend the lockfile test | expand

Commit Message

Leo-Andres Hofmann Dec. 2, 2021, 3:39 p.m. UTC
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@leo-andres.de>
---
 html/cgi-bin/pakfire.cgi | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

Comments

Leo-Andres Hofmann Dec. 2, 2021, 3:41 p.m. UTC | #1
Hi all,

this is my attempt at an AJAX log viewer for Pakfire, as I promised here:
https://lists.ipfire.org/pipermail/development/2021-November/011585.html

Stefan's patch must be applied beforehand for this to work!
https://patchwork.ipfire.org/patch/4842

In the last patch of this series, I remove the "sleep" calls.
If someone has a very slow system, please test both variants, to see if "sleep" is still necessary there.

Happy testing!

Best regards
Leo

Am 02.12.2021 um 16:39 schrieb Leo-Andres Hofmann:
> 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@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'} = '';
>   $cgiparams{'INSPAKS'} = '';
>   $cgiparams{'DELPAKS'} = '';
>   
> -sub refreshpage{&Header::openbox( 'Waiting', 1, "<meta http-equiv='refresh' content='1;'>" );print "<center><img src='/images/clock.gif' alt='' /><br/><font color='red'>$Lang::tr{'pagerefresh'}</font></center>";&Header::closebox();}
> -
>   &Header::getcgihash(\%cgiparams);
>   
>   &General::readhash("${General::swroot}/main/settings", \%mainsettings);
> @@ -54,7 +52,7 @@ sub refreshpage{&Header::openbox( 'Waiting', 1, "<meta http-equiv='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 = 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 = split(/\|/, $cgiparams{'DELPAKS'});
>   	if ("$cgiparams{'FORCE'}" eq "on") {
>   		&General::system_background("/usr/local/bin/pakfire", "remove", "--non-interactive", "--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 = `pidof pakfire`;
> -
> -if ($pid) {
> +if (&_is_pakfire_busy()) {
>   	&Header::openbox( 'Waiting', 1, "<meta http-equiv='refresh' content='10;'>" );
>   	print <<END;
>   	<table>
> @@ -203,7 +197,6 @@ END
>   	&Header::closebigbox();
>   	&Header::closepage();
>   	exit;
> -	refreshpage();
>   }
>   
>   my $core_release = `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 computed.)
> +	my $pakfire_pid = `pidof -s /usr/local/bin/pakfire`;
> +	chomp($pakfire_pid);
> +
> +	# Test presence of PID or lockfile
> +	return (($pakfire_pid) || (-e "$Pakfire::lockfile"));
> +}
Michael Tremer Dec. 2, 2021, 3:52 p.m. UTC | #2
Hello,

This patch looks good to me. I only have a small thought...

> On 2 Dec 2021, at 15:39, Leo-Andres Hofmann <hofmann@leo-andres.de> wrote:
> 
> 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@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'} = '';
> $cgiparams{'INSPAKS'} = '';
> $cgiparams{'DELPAKS'} = '';
> 
> -sub refreshpage{&Header::openbox( 'Waiting', 1, "<meta http-equiv='refresh' content='1;'>" );print "<center><img src='/images/clock.gif' alt='' /><br/><font color='red'>$Lang::tr{'pagerefresh'}</font></center>";&Header::closebox();}
> -
> &Header::getcgihash(\%cgiparams);
> 
> &General::readhash("${General::swroot}/main/settings", \%mainsettings);
> @@ -54,7 +52,7 @@ sub refreshpage{&Header::openbox( 'Waiting', 1, "<meta http-equiv='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 = 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 = split(/\|/, $cgiparams{'DELPAKS'});
> 	if ("$cgiparams{'FORCE'}" eq "on") {
> 		&General::system_background("/usr/local/bin/pakfire", "remove", "--non-interactive", "--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 = `pidof pakfire`;
> -
> -if ($pid) {
> +if (&_is_pakfire_busy()) {
> 	&Header::openbox( 'Waiting', 1, "<meta http-equiv='refresh' content='10;'>" );
> 	print <<END;
> 	<table>
> @@ -203,7 +197,6 @@ END
> 	&Header::closebigbox();
> 	&Header::closepage();
> 	exit;
> -	refreshpage();
> }
> 
> my $core_release = `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 computed.)
> +	my $pakfire_pid = `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.

-Michael

> +}
> -- 
> 2.27.0.windows.1
>
Leo-Andres Hofmann Dec. 2, 2021, 4:09 p.m. UTC | #3
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@leo-andres.de> wrote:
>>
>> 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@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'} = '';
>> $cgiparams{'INSPAKS'} = '';
>> $cgiparams{'DELPAKS'} = '';
>>
>> -sub refreshpage{&Header::openbox( 'Waiting', 1, "<meta http-equiv='refresh' content='1;'>" );print "<center><img src='/images/clock.gif' alt='' /><br/><font color='red'>$Lang::tr{'pagerefresh'}</font></center>";&Header::closebox();}
>> -
>> &Header::getcgihash(\%cgiparams);
>>
>> &General::readhash("${General::swroot}/main/settings", \%mainsettings);
>> @@ -54,7 +52,7 @@ sub refreshpage{&Header::openbox( 'Waiting', 1, "<meta http-equiv='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 = 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 = split(/\|/, $cgiparams{'DELPAKS'});
>> 	if ("$cgiparams{'FORCE'}" eq "on") {
>> 		&General::system_background("/usr/local/bin/pakfire", "remove", "--non-interactive", "--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 = `pidof pakfire`;
>> -
>> -if ($pid) {
>> +if (&_is_pakfire_busy()) {
>> 	&Header::openbox( 'Waiting', 1, "<meta http-equiv='refresh' content='10;'>" );
>> 	print <<END;
>> 	<table>
>> @@ -203,7 +197,6 @@ END
>> 	&Header::closebigbox();
>> 	&Header::closepage();
>> 	exit;
>> -	refreshpage();
>> }
>>
>> my $core_release = `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 computed.)
>> +	my $pakfire_pid = `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
>
>> +}
>> -- 
>> 2.27.0.windows.1
>>

Patch

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'} = '';
 $cgiparams{'INSPAKS'} = '';
 $cgiparams{'DELPAKS'} = '';
 
-sub refreshpage{&Header::openbox( 'Waiting', 1, "<meta http-equiv='refresh' content='1;'>" );print "<center><img src='/images/clock.gif' alt='' /><br/><font color='red'>$Lang::tr{'pagerefresh'}</font></center>";&Header::closebox();}
-
 &Header::getcgihash(\%cgiparams);
 
 &General::readhash("${General::swroot}/main/settings", \%mainsettings);
@@ -54,7 +52,7 @@  sub refreshpage{&Header::openbox( 'Waiting', 1, "<meta http-equiv='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 = 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 = split(/\|/, $cgiparams{'DELPAKS'});
 	if ("$cgiparams{'FORCE'}" eq "on") {
 		&General::system_background("/usr/local/bin/pakfire", "remove", "--non-interactive", "--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 = `pidof pakfire`;
-
-if ($pid) {
+if (&_is_pakfire_busy()) {
 	&Header::openbox( 'Waiting', 1, "<meta http-equiv='refresh' content='10;'>" );
 	print <<END;
 	<table>
@@ -203,7 +197,6 @@  END
 	&Header::closebigbox();
 	&Header::closepage();
 	exit;
-	refreshpage();
 }
 
 my $core_release = `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 computed.)
+	my $pakfire_pid = `pidof -s /usr/local/bin/pakfire`;
+	chomp($pakfire_pid);
+
+	# Test presence of PID or lockfile
+	return (($pakfire_pid) || (-e "$Pakfire::lockfile"));
+}