search

lucab / libsystemd-rs

A pure-Rust client library to work with systemd
https://crates.io/crates/libsystemd
Other
105 stars 19 forks source link

daemon/notify: add support for abstract unix sockets #122

Closed honsunrise closed 1 year ago

honsunrise commented 2 years ago

This is a draft PR for add support for unix socket abstract namespace.

Wating feature unix_socket_abstract stable.

Tracking issues: https://github.com/rust-lang/rust/issues/85410

lucab commented 2 years ago

Thanks for the PR! Could you maybe add some context on how you ended up needing this fix? I know that $NOTIFY_SOCKET technically could be an abstract socket, but I haven't observed systemd setting it up in that way so far.

If you are hitting this bug in some real world scenario, then we can probably assemble a quicker fix using nix, without having to wait for the stdlib feature.

honsunrise commented 2 years ago

Setting it up with unix socket abstract namespace was found in the first release of CentOS 7 7.1.1503 (1505_01) when I was doing compatibility testing for our software.

I belive it can reproduced by this vagrant box https://cloud.centos.org/centos/7/vagrant/x86_64/images/CentOS-7-Vagrant-1505-x86_64-01.LibVirt.box.

lucab commented 1 year ago

Hey @honsunrise, I've pushed a commit on top of your branch to implement the same logic (notify through an abstract Unix socket) without the need of an unstable stdlib feature. It uses existing nix primitives instead. Can you please give a look and try this new code, and let us know if it is ok to merge? Thanks!

lucab commented 1 year ago

@swsnr could you please review this? I'm not sure the original poster is still much active...

swsnr commented 1 year ago

@lucab Your commit looks correct to me with respect to the original changes, but I literally have no idea what this pull request itself does, so I can't really review it as a whole. Added my approval, for the record, but take it with a grain of salt 😇

swsnr commented 1 year ago

@lucab That said, I'd prefer to wait for the OP 🙂 As I understand their comments, they're doing some testing for their (proprietary or commercial?) software, so they'd be in the best position to test your changes, and with a commercial background I'd expect them to take responsibility for this change 😇

lucab commented 1 year ago

@swsnr ack, thanks for the review, I'll wait a bit longer before merging.

honsunrise commented 1 year ago

@lucab @swsnr Don't worry, I'm still alive. And very thanks for your commit. While the commit looks correct to me, I still need more time to test to make sure everything works as expected.

honsunrise commented 1 year ago

@lucab @swsnr

It works very well.

The test result:

Distribution Kernel version Pass
CentOS 7.1.1503 (1505_01) 3.10.0-229.4.2.el7.x86_64 ✔️
CentOS 7.1.1503 (1508_01) 3.10.0-229.11.1.el7.x86_64 ✔️
CentOS 7.1.1503 (1509_01) 3.10.0-229.14.1.el7.x86_64 ✔️
CentOS 7.2.1511 (1601_01) 3.10.0-327.4.5.el7.x86_64 ✔️
CentOS 7.2.1511 (1602_01) 3.10.0-327.10.1.el7.x86_64 ✔️
CentOS 7.2.1511 (1602_02) 3.10.0-327.10.1.el7.x86_64 ✔️
CentOS 7.2.1511 (1603_01) 3.10.0-327.13.1.el7.x86_64 ✔️
CentOS 7.2.1511 (1605_01) 3.10.0-327.18.2.el7.x86_64 ✔️
CentOS 7.2.1511 (1606_01) 3.10.0-327.22.2.el7.x86_64 ✔️
CentOS 7.2.1511 (1608_01) 3.10.0-327.28.3.el7.x86_64 ✔️
CentOS 7.2.1511 (1609_01) 3.10.0-327.36.1.el7.x86_64 ✔️
CentOS 7.2.1511 (1610_01) 3.10.0-327.36.3.el7.x86_64 ✔️
CentOS 7.3.1611 (1611_01) 3.10.0-514.2.2.el7.x86_64 ✔️
CentOS 7.3.1611 (1701_01) 3.10.0-514.6.1.el7.x86_64 ✔️
CentOS 7.3.1611 (1702_01) 3.10.0-514.6.2.el7.x86_64 ✔️
CentOS 7.3.1611 (1703_01) 3.10.0-514.10.2.el7.x86_64 ✔️
CentOS 7.3.1611 (1704_01) 3.10.0-514.16.1.el7.x86_64 ✔️
CentOS 7.3.1611 (1705_01) 3.10.0-514.21.1.el7.x86_64 ✔️
CentOS 7.3.1611 (1705_02) 3.10.0-514.21.2.el7.x86_64 ✔️
CentOS 7.3.1611 (1706_01) 3.10.0-514.26.1.el7.x86_64 ✔️
CentOS 7.3.1611 (1706_02) 3.10.0-514.26.2.el7.x86_64 ✔️
CentOS 7.3.1611 (1707_01) 3.10.0-514.26.2.el7.x86_64 ✔️
CentOS 7.4.1708 (1708_01) 3.10.0-693.2.1.el7.x86_64 ✔️
CentOS 7.4.1708 (1709_01) 3.10.0-693.2.2.el7.x86_64 ✔️
CentOS 7.4.1708 (1710_01) 3.10.0-693.5.2.el7.x86_64 ✔️
CentOS 7.4.1708 (1711_01) 3.10.0-693.5.2.el7.x86_64 ✔️
CentOS 7.4.1708 (1801_02) 3.10.0-693.11.6.el7.x86_64 ✔️
CentOS 7.4.1708 (1802_01) 3.10.0-693.17.1.el7.x86_64 ✔️
CentOS 7.4.1708 (1803_01) 3.10.0-693.21.1.el7.x86_64 ✔️
CentOS 7.5.1804 (1804_02) 3.10.0-862.2.3.el7.x86_64 ✔️
CentOS 7.5.1804 (1805_01) 3.10.0-862.3.2.el7.x86_64 ✔️
CentOS 7.5.1804 (1809_01) 3.10.0-862.14.4.el7.x86_64 ✔️
CentOS 7.6.1810 (1811_01) 3.10.0-957.1.3.el7.x86_64 ✔️
CentOS 7.6.1810 (1811_02) 3.10.0-957.1.3.el7.x86_64 ✔️
CentOS 7.6.1810 (1812_01) 3.10.0-957.1.3.el7.x86_64 ✔️
CentOS 7.6.1810 (1901_01) 3.10.0-957.1.3.el7.x86_64 ✔️
CentOS 7.6.1810 (1902_01) 3.10.0-957.5.1.el7.x86_64 ✔️
CentOS 7.6.1810 (1905_01) 3.10.0-957.12.2.el7.x86_64 ✔️
CentOS 7.6.1810 (1907_01) 3.10.0-957.27.2.el7.x86_64 ✔️
CentOS 7.7.1908 (1910_01) 3.10.0-1062.4.1.el7.x86_64 ✔️
CentOS 7.7.1908 (2001_01) 3.10.0-1062.9.1.el7.x86_64 ✔️
CentOS 7.7.1908 (2002_01) 3.10.0-1062.12.1.el7.x86_64 ✔️
CentOS 7.8.2003 (2004_01) 3.10.0-1127.el7.x86_64 ✔️