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 <[email protected]> >> --- >> >> 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 [email protected] https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
