lnicola / sd-notify

Apache License 2.0
45 stars 6 forks source link

Fix the FD_CLOEXEC bit #10

Closed mbuesch closed 6 months ago

mbuesch commented 6 months ago

FD_CLOEXEC is not a bit position. It's a bit mask already.

See the definition in the kernel: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/asm-generic/fcntl.h

and the use of that constant in: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/fcntl.c

The current code in sd-notify is a no-op. Bit 1 is not looked at by the kernel. sd-notify does not touch bit 0.

Fix this by changing the mask.

lnicola commented 6 months ago

Ouch, this is very embarrassing, thanks!

Can you also add a changelog entry?

mbuesch commented 6 months ago

Thanks for your review. I added a changelog entry.

mbuesch commented 6 months ago

Meh, the changelog should have said Fixed instead of Changed, I guess. Sorry. Feel free to fix that when bumping the version number.

mbuesch commented 6 months ago

One more question: Do you plan to do a new release with this fix applied?

Thanks a lot for your time. :)

lnicola commented 6 months ago

Yes, I wanted to include #8, but I'll probably have to ping the author.

mbuesch commented 6 months ago

Cool. Thanks. Take your time.

lnicola commented 6 months ago

In the meanwhile, another pair of eyes on that PR would be appreciated, just saying 😅.

mbuesch commented 4 months ago

Hi

Is it possible to make a release with just this fix applied? I'd like to release a dependent program. But that's not possible, if I'm on a git-dependency. So I would have to release without the fix. But I would like to include this fix, too.

Thanks a lot :+1:

lnicola commented 4 months ago

Right, sorry. I'll try to do it tomorrow.

lnicola commented 4 months ago

Done, and sorry for the delays: https://crates.io/crates/sd-notify

mbuesch commented 4 months ago

Thank you very much for your work. This really is an important and useful crate, IMO.

lnicola commented 1 month ago

Thanks, btw!