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

[Fiona Ebner <f.ebner@proxmox.com>]

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 <f.ebner@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
> 

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