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

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

On Tue, Oct 07, 2025 at 02:25:00PM +0200, Fiona Ebner wrote:
> Implement a pure Perl reimplementation of systemd's sd_notify() as
> defined in systemd/sd-daemon.h, see also 'man 3 sd_notify'.
> 
> The initial user of this helper is intended to be the pve-dbus-vmstate
> service, so it can notify startup completion only once the
> dbus-vmstate QEMU object is ready to be used.
> 
> EAGAIN is not checked for, because it does not occur for blocking
> Unix domain sockets, see 'man 2 send'.
> 
> Co-developed-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
> 
> Changes in v3:
> * Expand commit message.
> * Use $socket->{send,shutdown) methods.
> * Print $IO::Socket::errstr in case of error.
> * Unset NOTIFY_SOCKET environment variable only after sending the
>   message.
> 
>  src/PVE/Systemd.pm | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/src/PVE/Systemd.pm b/src/PVE/Systemd.pm
> index e6d6f88..d0a291d 100644
> --- a/src/PVE/Systemd.pm
> +++ b/src/PVE/Systemd.pm
> @@ -3,9 +3,12 @@ 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 POSIX qw(EINTR);
> +use Socket qw(SOCK_DGRAM);
>  
>  use PVE::Tools qw(file_set_contents file_get_contents trim);
>  
> @@ -282,4 +285,32 @@ sub write_ini {
>      file_set_contents($filename, $content);
>  }
>  
> +# This is a pure Perl reimplementation of systemd's sd_notify() as defined in systemd/sd-daemon.h
> +sub sd_notify {
> +    my ($unset_environment, $state) = @_;
> +
> +    my $socket_path = $ENV{NOTIFY_SOCKET};

Technically this could be an abstract socket. Should be enough to just

    $socket_path =~ s/^@/\0/;

> +
> +    my $socket = IO::Socket::UNIX->new(
> +        Type => SOCK_DGRAM(),
> +        Peer => $socket_path,
> +    ) or die "unable to connect to socket $socket_path to notify systemd - $IO::Socket::errstr\n";
> +
> +    # we won't be reading from the socket
> +    $socket->shutdown(SHUT_RD);
> +
> +    my $sent = 0;
> +    my $total = length($state);
> +    while ($sent < $total) {
> +        my $res = $socket->send($state);
> +        die "sending to $socket_path failed - $!" if !$res && $! != EINTR;
> +        $sent += $res if $res;

^ This is a datagram socket. Systemd expects a single datagram.
The code sort of makes it look like you're trying doing a `write_all()`
style send (without actually changing what it sent in between calls
which wouldn't return zero).
Trying to continue sending in a fragmented way won't work anyway.
(Otherwise it would be rather cumbersome, since the protocol also allows
adding things like file descriptors to store in the fd registry; data
and metadata need to come in one nice bundle)

The example code in the referenced man page errors out with `-EPROTO` if
the length does not match, so we could do that as well.

So basically, only an EINTR loop makes sense here.

> +    }
> +    $socket->flush();

This should not be necessary, this is not buffered I/O.

> +
> +    close($socket);
> +
> +    delete($ENV{NOTIFY_SOCKET}) if $unset_environment;

Why is this part of this function, though?

> +}
> +
>  1;
> -- 
> 2.47.3


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel