[pve-devel] [PATCH v4 1/3] migrate: collect migration tunnel child process

[Dietmar Maurer]

> I did the loop exactly because of that reason, I had to insert the same
> code two times exactly the same and another time the waitpid, as this
> seems unnecessary to me I used a simple loop, anybody sees what happens

You loop has serious drawbacks, for example you run into strange side effects
when you want to change the timeout (timeout is no longer a parameter, so you
have
to change hard coded values at several places).

> and  it works without writing everything tree times... I is surely not
> the nicest solution, but imo better that tripple code.

I guess there are other ways to avoid code duplication, for example:


diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
index a25efff..d5dcc55 100644
--- a/PVE/QemuMigrate.pm
+++ b/PVE/QemuMigrate.pm
@@ -2,6 +2,7 @@ package PVE::QemuMigrate;

 use strict;
 use warnings;
+use POSIX qw( WNOHANG );
 use PVE::AbstractMigrate;
 use IO::File;
 use IPC::Open2;
@@ -44,17 +45,28 @@ sub fork_command_pipe {
 sub finish_command_pipe {
     my ($self, $cmdpipe, $timeout) = @_;

+    my $cpid = $cmdpipe->{pid};
+    return if !defined($cpid);
+
     my $writer = $cmdpipe->{writer};
     my $reader = $cmdpipe->{reader};

     $writer->close();
     $reader->close();

-    my $cpid = $cmdpipe->{pid};
+    my $check_process_running = sub {
+       my $res = waitpid($cpid, WNOHANG);
+       if (defined($res) && ($res == $cpid)) {
+           delete $cmdpipe->{cpid};
+           return 1;
+       } else {
+           return 0;
+       }
+    };

     if ($timeout) {
        for (my $i = 0; $i < $timeout; $i++) {
-           return if !PVE::ProcFSTools::check_process_running($cpid);
+           return if !&$check_process_running();
            sleep(1);
        }
     }
@@ -64,13 +76,15 @@ sub finish_command_pipe {

     # wait again
     for (my $i = 0; $i < 10; $i++) {
-       return if !PVE::ProcFSTools::check_process_running($cpid);
+       return if !&$check_process_running();
        sleep(1);
     }

     $self->log('info', "ssh tunnel still running - terminating now with
SIGKILL\n");
     kill 9, $cpid;
     sleep 1;
+
+    &$check_process_running();
 }

 sub fork_tunnel {

This it totally untested - I just want to show what I talk about.