[pve-devel] [PATCH-SERIES librados2-perl] improve fork behavior

[pve-devel] [PATCH-SERIES librados2-perl] improve fork behavior

[Fiona Ebner]

Reset some inherited signal handlers and use POSIX::_exit instead of
exit in the child worker to avoid potentially unexpected destructor
and END block execution.

Fiona Ebner (3):
  refactor pverados worker into dedicated function
  reset inherited signal handlers in child worker
  use POSIX::_exit instead of exit in forked child

 PVE/RADOS.pm | 143 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 79 insertions(+), 64 deletions(-)

-- 
2.39.2

[pve-devel] [PATCH librados2-perl 1/3] refactor pverados worker into dedicated function

[Fiona Ebner]

No functional change intended.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

Better viewed with
git diff --color-moved=zebra --color-moved-ws=ignore-all-space
or similar

 PVE/RADOS.pm | 116 +++++++++++++++++++++++++++------------------------
 1 file changed, 61 insertions(+), 55 deletions(-)

diff --git a/PVE/RADOS.pm b/PVE/RADOS.pm
index aba6653..162e7db 100644
--- a/PVE/RADOS.pm
+++ b/PVE/RADOS.pm
@@ -105,6 +105,66 @@ my $sendcmd = sub {
     return $raw;
 };
 
+sub pve_rados_work {
+    my ($self, $parent, $timeout, %params) = @_;
+
+    my $conn;
+    eval {
+	my $ceph_user = delete $params{userid} || $ceph_default_user;
+	$conn = pve_rados_create($ceph_user) ||
+	    die "unable to create RADOS object\n";
+
+	if (defined($params{ceph_conf}) && (!-e $params{ceph_conf})) {
+	    die "Supplied ceph config doesn't exist, $params{ceph_conf}\n";
+	}
+
+	my $ceph_conf = delete $params{ceph_conf} || $ceph_default_conf;
+
+	if (-e $ceph_conf) {
+	    pve_rados_conf_read_file($conn, $ceph_conf);
+	}
+
+	pve_rados_conf_set($conn, 'client_mount_timeout', $timeout);
+
+	foreach my $k (keys %params) {
+	    pve_rados_conf_set($conn, $k, $params{$k});
+	}
+
+	pve_rados_connect($conn);
+    };
+    if (my $err = $@) {
+	&$writedata($parent, 'E', $err);
+	die $err;
+    }
+    &$writedata($parent, 'S');
+
+    $self->{conn} = $conn;
+
+    for (;;) {
+	my ($cmd, $data) = &$readdata($parent, 1);
+
+	last if !$cmd || $cmd eq 'Q';
+
+	my $res;
+	eval {
+	    if ($cmd eq 'M') { # rados monitor commands
+		$res = encode_json(pve_rados_mon_command($self->{conn}, [ $data ]));
+	    } elsif ($cmd eq 'C') { # class methods
+		my $aref = decode_json($data);
+		my $method = shift @$aref;
+		$res = encode_json($self->$method(@$aref));
+	    } else {
+		die "invalid command\n";
+	    }
+	};
+	if (my $err = $@) {
+	    &$writedata($parent, 'E', $err);
+	    die $err;
+	}
+	&$writedata($parent, '>', $res);
+    }
+}
+
 sub new {
     my ($class, %params) = @_;
 
@@ -145,61 +205,7 @@ sub new {
 
 	close $child;
 
-	my $conn;
-	eval {
-	    my $ceph_user = delete $params{userid} || $ceph_default_user;
-	    $conn = pve_rados_create($ceph_user) ||
-		die "unable to create RADOS object\n";
-
-	    if (defined($params{ceph_conf}) && (!-e $params{ceph_conf})) {
-		die "Supplied ceph config doesn't exist, $params{ceph_conf}\n";
-	    }
-
-	    my $ceph_conf = delete $params{ceph_conf} || $ceph_default_conf;
-
-	    if (-e $ceph_conf) {
-		pve_rados_conf_read_file($conn, $ceph_conf);
-	    }
-
-	    pve_rados_conf_set($conn, 'client_mount_timeout', $timeout);
-
-	    foreach my $k (keys %params) {
-		pve_rados_conf_set($conn, $k, $params{$k});
-	    }
-
-	    pve_rados_connect($conn);
-	};
-	if (my $err = $@) {
-	    &$writedata($parent, 'E', $err);
-	    die $err;
-	}
-	&$writedata($parent, 'S');
-
-	$self->{conn} = $conn;
-
-	for (;;) {
-	    my ($cmd, $data) = &$readdata($parent, 1);
-
-	    last if !$cmd || $cmd eq 'Q';
-
-	    my $res;
-	    eval {
-		if ($cmd eq 'M') { # rados monitor commands
-		    $res = encode_json(pve_rados_mon_command($self->{conn}, [ $data ]));
-		} elsif ($cmd eq 'C') { # class methods
-		    my $aref = decode_json($data);
-		    my $method = shift @$aref;
-		    $res = encode_json($self->$method(@$aref));
-		} else {
-		    die "invalid command\n";
-		}
-	    };
-	    if (my $err = $@) {
-		&$writedata($parent, 'E', $err);
-		die $err;
-	    }
-	    &$writedata($parent, '>', $res);
-	}
+	$self->pve_rados_work($parent, $timeout, %params);
 
 	exit(0);
     }
-- 
2.39.2

[pve-devel] [PATCH librados2-perl 2/3] reset inherited signal handlers in child worker

[Fiona Ebner]

For example, if pvestatd is the parent, the inherited signal handlers
from pvestatd, i.e. PVE/Daemon.pm's handlers would be invoked by the
child.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 PVE/RADOS.pm | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/PVE/RADOS.pm b/PVE/RADOS.pm
index 162e7db..41b9c39 100644
--- a/PVE/RADOS.pm
+++ b/PVE/RADOS.pm
@@ -201,6 +201,11 @@ sub new {
 	    &$atfork();
 	}
 
+	# override signal handlers inherited from the parent
+	local $SIG{HUP} = $SIG{INT} = $SIG{QUIT} = $SIG{TERM} = sub {
+	    exit(1);
+	};
+
 	# fixme: timeout?
 
 	close $child;
-- 
2.39.2

[pve-devel] [PATCH librados2-perl 3/3] use POSIX::_exit instead of exit in forked child

[Fiona Ebner]

This avoids calling the END blocks and destructor routines most of
which have nothing to do with the fork, so it cannot be assumed to be
safe to do from the fork.

Move the call to pve_rados_shutdown from the destructor routine,
which will not be called by the child anymore. Make sure the cleanup
is also done when the worker dies.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

Better viewed with -w

 PVE/RADOS.pm | 38 +++++++++++++++++++++-----------------
 1 file changed, 21 insertions(+), 17 deletions(-)

diff --git a/PVE/RADOS.pm b/PVE/RADOS.pm
index 41b9c39..f09d890 100644
--- a/PVE/RADOS.pm
+++ b/PVE/RADOS.pm
@@ -7,6 +7,7 @@ use warnings;
 
 use Carp;
 use JSON;
+use POSIX;
 use Socket;
 
 use PVE::Tools;
@@ -195,24 +196,30 @@ sub new {
     } else { # child
 	$0 = 'pverados';
 
-	PVE::INotify::inotify_close();
+	eval {
+	    PVE::INotify::inotify_close();
 
-	if (my $atfork = $rpcenv->{atfork}) {
-	    &$atfork();
-	}
+	    if (my $atfork = $rpcenv->{atfork}) {
+		&$atfork();
+	    }
 
-	# override signal handlers inherited from the parent
-	local $SIG{HUP} = $SIG{INT} = $SIG{QUIT} = $SIG{TERM} = sub {
-	    exit(1);
+	    # override signal handlers inherited from the parent
+	    local $SIG{HUP} = $SIG{INT} = $SIG{QUIT} = $SIG{TERM} = sub {
+		pve_rados_shutdown($self->{conn}) if $self->{conn};
+		POSIX::_exit(1);
+	    };
+
+	    # fixme: timeout?
+
+	    close $child;
+
+	    $self->pve_rados_work($parent, $timeout, %params);
 	};
+	my $err = $@;
+	warn $err if $err;
 
-	# fixme: timeout?
-
-	close $child;
-
-	$self->pve_rados_work($parent, $timeout, %params);
-
-	exit(0);
+	pve_rados_shutdown($self->{conn}) if $self->{conn};
+	POSIX::_exit($err ? 1 : 0);
     }
 
     return $self;
@@ -233,9 +240,6 @@ sub DESTROY {
 	#print "$$: DESTROY WAIT0\n";
 	&$kill_worker($self);
 	#print "$$: DESTROY WAIT\n";
-    } else {
-	#print "$$: DESTROY SHUTDOWN\n";
-	pve_rados_shutdown($self->{conn}) if $self->{conn};
     }
 }
 
-- 
2.39.2

[pve-devel] applied-series: [PATCH-SERIES librados2-perl] improve fork behavior

[Wolfgang Bumiller]

applied series, thanks