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

Thomas Lamprecht t.lamprecht at proxmox.com
Thu Jun 2 17:50:27 CEST 2016



Am 02.06.2016 um 17:06 schrieb Dietmar Maurer:
>> I can see the reason to use waitpid instead of check_process_running(),
>> but why do you change the rest of the code?
>>
>> Can we have minimal patches, where each patch states the reason for the change
>> in the commit log?
> 
> I thought about something like this:
> 
> diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
> index a25efff..ce5774e 100644
> --- a/PVE/QemuMigrate.pm
> +++ b/PVE/QemuMigrate.pm
> @@ -71,6 +71,8 @@ sub finish_command_pipe {
>      $self->log('info', "ssh tunnel still running - terminating now with
> SIGKILL\n");
>      kill 9, $cpid;
>      sleep 1;
> +
> +    waitpid($cpid); # avoid zombies

That doesn't fixes it :)

You have returns above so this statement is only reached if all fails
and the tunnel has to be killed, in normal cases the child does not get
selected.

Also the writer/reader may get double closed here, i thought about
something like
diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
index a25efff..d869fa3 100644
--- a/PVE/QemuMigrate.pm
+++ b/PVE/QemuMigrate.pm
@@ -42,37 +42,53 @@ 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 $waitpid;
     if ($timeout) {
        for (my $i = 0; $i < $timeout; $i++) {
-           return if !PVE::ProcFSTools::check_process_running($cpid);
+           $waitpid = waitpid($cpid, WNOHANG);
+           if (defined($waitpid) && $waitpid == $cpid) {
+               delete $cmdpipe->{cpid};
+               return;
+           }
            sleep(1);
        }
     }

     $self->log('info', "ssh tunnel still running - terminating now with
SIGTERM\n");
     kill(15, $cpid);

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

     $self->log('info', "ssh tunnel still running - terminating now with
SIGKILL\n");
     kill 9, $cpid;
     sleep 1;
+
+    $waitpid = waitpid($cpid, WNOHANG);
+    $self->log('err', "Tunnel process (PID $cpid) could not be
collected after kill")
+       if (!(defined($waitpid) && $waitpid == $cpid));
+
+    delete $cmdpipe->{cpid};
 }


>  }
> 
>  sub fork_tunnel {
> 



More information about the pve-devel mailing list