From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 5C48F1FF187 for ; Mon, 6 Oct 2025 15:14:03 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 10730492A; Mon, 6 Oct 2025 15:14:05 +0200 (CEST) Message-ID: Date: Mon, 6 Oct 2025 15:14:00 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta To: Proxmox VE development discussion , Fiona Ebner References: <20251006115648.804286-1-f.ebner@proxmox.com> <20251006115648.804286-2-f.ebner@proxmox.com> Content-Language: en-US From: Thomas Lamprecht In-Reply-To: <20251006115648.804286-2-f.ebner@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1759756412271 X-SPAM-LEVEL: Spam detection results: 0 AWL -0.027 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 RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. 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 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. ^^ > > 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 > + > + 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; _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel