[pve-devel] [PATCH common v2 1/2] systemd: add sd_notify() helper

[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. ^^

> 
> Signed-off-by: Fiona Ebner <f.ebner at proxmox.com>
> ---
> 
> 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;