From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 205541FF191 for ; Tue, 7 Oct 2025 12:26:36 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id A80B415A72; Tue, 7 Oct 2025 12:26:38 +0200 (CEST) Message-ID: <9f6f0dac-f673-41f3-b2c7-994e3805bf2c@proxmox.com> Date: Tue, 7 Oct 2025 12:26:34 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Thomas Lamprecht , Proxmox VE development discussion References: <20251006115648.804286-1-f.ebner@proxmox.com> <20251006115648.804286-2-f.ebner@proxmox.com> Content-Language: en-US From: Fiona Ebner In-Reply-To: X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1759832765450 X-SPAM-LEVEL: Spam detection results: 0 AWL -0.021 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH common v2 1/2] systemd: add sd_notify() helper X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" Am 06.10.25 um 3:13 PM schrieb Thomas Lamprecht: > Am 06.10.25 um 13:57 schrieb Fiona Ebner: >> See 'man 3 sd_notify'. > > Such references can be OK to avoid duplicating all details, but not as > full replacement for basic commit message context.. > Would be nice to know that this is a pure perl reimplementation of the > sd_notify interface > > The lack of that made me look quite a bit closer than I'd otherwise have, > so a bit of a pedantic review inline. ^^ Yes, that's fair. I do like getting pedantic reviews. There's more to learn then :) >> >> Signed-off-by: Fiona Ebner >> --- >> >> New in v2. >> >> src/PVE/Systemd.pm | 21 +++++++++++++++++++++ >> 1 file changed, 21 insertions(+) >> >> diff --git a/src/PVE/Systemd.pm b/src/PVE/Systemd.pm >> index e6d6f88..96e7d80 100644 >> --- a/src/PVE/Systemd.pm >> +++ b/src/PVE/Systemd.pm >> @@ -3,9 +3,11 @@ package PVE::Systemd; >> use strict; >> use warnings; >> >> +use IO::Socket::UNIX; >> use Net::DBus qw(dbus_uint32 dbus_uint64 dbus_boolean); >> use Net::DBus::Callback; >> use Net::DBus::Reactor; >> +use Socket qw(SOCK_DGRAM); >> >> use PVE::Tools qw(file_set_contents file_get_contents trim); >> >> @@ -282,4 +284,23 @@ sub write_ini { >> file_set_contents($filename, $content); >> } >> > > Short comment that this is a pure-perl re-implementation of the sd_notify > interface as defined in systemd/sd-daemon.h might be good to have here. > >> +sub sd_notify { >> + my ($unset_environment, $state) = @_; >> + >> + my $socket_path = $unset_environment ? delete($ENV{NOTIFY_SOCKET}) : $ENV{NOTIFY_SOCKET}; > > In systemd's sd_notify, which is just a trivial wrapper around > sd_pid_notify_with_fds [0], the unsetting of the environment happens > after doing the notify. While this should not matter much in practice > given that we do not have threading here, so nothing can really happen > concurrently, it might be still better to use the original's pattern > if only for someone else taking this as source for their implementation > for an environment where this detail might actually matter. > > [0]: https://github.com/systemd/systemd/blob/f0a1b3c183dea90259f3b55ad0fa4bd25b1590a2/src/libsystemd/sd-daemon/sd-daemon.c#L626-L638 > Good point! >> + >> + my $socket = IO::Socket::UNIX->new( >> + Type => SOCK_DGRAM(), >> + Peer => $socket_path, >> + ) or die "unable to connect to socket $socket_path to notify systemd\n"; > > please include the actual error from IO::Socket::UNIX->new in the message, > i.e. "$IO::Socket::errstr" or "$@" as otherwise any error will be harder to > debug. > > https://metacpan.org/pod/IO::Socket::UNIX#new-(-[ARGS]-) > >> + >> + # we won't be reading from the socket >> + shutdown($socket, 0); > > FWIW the `IO::Socket` module would provide shutdown also on the blessed > socket [1], and also the more telling constant names for SHUT_RD, > SHUT_WR or SHUT_RDWR > > [1]: https://metacpan.org/pod/IO::Socket#shutdown > > >> + >> + print {$socket} $state; > > Does print retry automatically on EINTR or when not being able to send > the full message in one go? While both is very unlikely here, it would > still be great if we got hardened resiliency for such important helpers. > If print doesn't gives us any convenience guarantees here, it might be > better to use $socket->send($state) and check for errors and if all was > send out as safety check. > >> + $socket->flush(); >> + >> + close($socket); >> +} >> + >> 1; > I did look at other usages of sockets in our code base, and did not check for these details, which I should have. I didn't find anything explicit in the documentation about the behavior of print in such cases, but at least there is a test case that print will not forward EINTR [0]. Still, better to be explicit. [0]: https://github.com/Perl/perl5/blob/70a73bdabb7ed570cb8451b2d38ec69a4feeb0f6/t/io/eintr_print.t _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel