Open rwp0 opened 2 years ago
Editing Process.pm
and prefxing with the module name where can_run
is defined solves this issue.
I can submit that PR if the bug is approved.
sub killall {
my ( $process, $sig ) = @_;
$sig ||= "";
if ( Rex::Commands::Run::can_run("killall") ) { # PREFIXED: by module
i_run( "killall $sig $process", fail_ok => 1 );
if ( $? != 0 ) {
die("Error killing $process");
}
}
else {
die("Can't execute killall.");
}
}
Thanks for the report!
can_run
is defined and exported in Rex::Commands::Run
, so it is probably caused by a missing use Rex::Commands::Run;
to import it first. Using the fully qualified name also should fix it like you suggested :+1:
A proper PR would include tests too, but we can't easily call killall
randomly on tester machines :) I wonder how we could test for the presence of this class of bugs in general :thinking: Perhaps there is a good Test::
module or Perl::Critic
policy to do it for us.
A proper PR would include tests too, but we can't easily call killall randomly on tester machines :)
Since it can't be tested, is it okay if I submit PR with the suggestion above? (ie. prefixing the function with Rex::Commands::Run::
)
Thanks
Since it can't be tested
It can be tested in multiple ways, I'm just not sure which way is the best going forward yet, whether others already published useful work we can reuse, and extra care must be taken to not accidentally kill
processes on tester machines.
My ideas so far:
Use a Perl::Critic policy or perhaps a Test:: module from CPAN that scans/tests the code for missing imports.
Perl::Critic::Policy::Dynamic::ValidateAgainstSymbolTable comes close, but that does not fit into static analysis anymore, but it actually compiles the code which is unsafe. I would not like to impose that onto developer/tester machines, but it might be OK to use it in the isolated environments of GitHub Actions. It would highlight all other potential instances of this type of error currently present in the code base, and also would protect us from introducing any further instances of this type of problem. While that sounds good, we would still need proper tests to validate that our code actually works, though.
I did not search for anything in the Test:: namespace yet.
Add a simple new test, like t/process.t
/t/killall.t
/t/issue/1548.t
(and based on e.g. t/cmdb_path.t
boilerplate), which calls killall
with some (fake or simulated) target process. This test should fail now, and would pass after the fix. Which is exactly what we expect from a test ;)
To avoid accidental interference with other processes running on the machine executing the teste we can do one or more of the following:
0
to the process with our killall
call to just test if we could send a signal if we want tokillall
test (and then perhaps also test for process existence before/after the killall
call to validate functionality)killall
available (or catch the exception, and call it a pass, if that's normal on these OSes)
Describe the bug
Running
killall
command on a FreeBSD server results in:killall
is defined as follows:Seems like there's no
can_run
function inProcess.pm
module.Expected behavior
killing process by name successfully
How to reproduce it
Run the code example.
Code example
Additional context
I tested, and the same error occured locally on Debian Linux as well.
Rex version
1.13.4
Perl version
5.36.0
Operating system running rex
Debian
Operating system managed by rex
FreeBSD
How rex was installed?
cpan client