killall command fails with: no can_run function in Process.pm

[rwp0]

Describe the bug

Running killall command on a FreeBSD server results in:

ERROR - Undefined subroutine &Rex::Commands::Process::can_run called

at /home/regular/perl5/perlbrew/perls/perl-5.36.0/lib/site_perl/5.36.0/Rex/Commands/Process.pm line 85.

killall is defined as follows:

sub killall {
  my ( $process, $sig ) = @_;
  $sig ||= "";

  if ( can_run("killall") ) {
    i_run( "killall $sig $process", fail_ok => 1 );
    if ( $? != 0 ) {
      die("Error killing $process");
    }
  }
  else {
    die("Can't execute killall.");
  }
}

Seems like there's no can_run function in Process.pm module.

Expected behavior

killing process by name successfully

How to reproduce it

Run the code example.

Code example

task "kill_top", sub {
  killall "top";
};

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

[rwp0]

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.");
  }
}

[ferki]

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.

[rwp0]

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

[ferki]

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:

1. 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.

2. 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:

   • make sure we send signal 0 to the process with our killall call to just test if we could send a signal if we want to
   • we may fork a child process that does nothing (e.g. sleeps for a few seconds), and use that as the target for the killall test (and then perhaps also test for process existence before/after the killall call to validate functionality)
   • perhaps SKIP this test on operating systems that are not compatible with forking this way or does not have killall available (or catch the exception, and call it a pass, if that's normal on these OSes)