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

[Thomas Lamprecht]

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
and  it works without writing everything tree times... I is surely not
the nicest solution, but imo better that tripple code.

I can send a patch tomorrow which really strictly only touches the
problematic things, but the single line is not enough.

Am 02.06.2016 um 17:50 schrieb Thomas Lamprecht:
> 
> 
> 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 {
>>
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> http://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>