Duplicate all public functions with `_and_unset_env` #13

tbu- · 2025-01-15T13:55:46Z

They need to be unsafe because std::env::remove_var is unsafe.

Fixes #9.

lnicola · 2025-01-15T15:03:15Z

Thanks! I'm not sure I agree that the unset_env should not be set. Let's split the function into sd_notify and sd_notify_and_unset_env (or similar).

tbu- · 2025-01-15T15:11:06Z

Do you wish me to make a breaking change (to be released in a new major version) or should I simply add more functions and mark the current one as deprecated?

lnicola · 2025-01-15T15:14:27Z

Yeah, we can go to 0.5.0, it's not such a big change anyway.

tbu- · 2025-01-15T16:03:26Z

Roughly like this? I'll add the missing documentation and fix the tests if this is an okay API.

mbuesch

Looks good :)

All new unsafe functions should get a proper docstring which references the not-unset-env function and describes what unset-env does.

README.md

mbuesch · 2025-01-15T16:51:14Z

src/lib.rs

@@ -156,12 +161,16 @@ pub fn notify(unset_env: bool, state: &[NotifyState]) -> io::Result<()> {
    }
 }

+pub unsafe fn notify_and_unset_env(state: &[NotifyState]) -> io::Result<()> {
+    let result = notify(state);
+    unsafe {


I think older Rust versions throw a warning "unnecessary unsafe" here and we might want to add a warning suppression if we want to support them.

What old rust versions are you worried about? Users who depend on this crate will never see a warning because warnings are not shown for dependencies. This crate seems to run with the latest stable in CI. The Rust compiler does not warn for unnecessary unsafe here since rust-lang/rust#124636, Rust 1.80 (which would only be relevant for developers, not users of this crate).

What old rust versions are you worried about?

I guess you gave the answer to yourself?

So you're worried about developers of this crate who use rust versions older than 1.80 seeing warnings?

This all boils down to the question which MSRV this project supports.
And if this throws a warning with this MSRV, the warning should be fixed IMO.
Otherwise the build logs are cluttered with useless warnings on the test builds that are necessary before release.

I don't think it matters. IIRC, when you build a crate, you don't get warnings for its dependencies, so this doesn't affect the users.

As for this crate itself, it's currently missing a rust-version declaration (I'm not opposed to adding one). But warnings on the MSRV shouldn't block changes, it targets stable.

mbuesch · 2025-01-15T16:53:19Z

src/lib.rs

+    fds: &[BorrowedFd<'_>],
+) -> io::Result<()> {
+    let result = notify_with_fds(state, fds);
+    unsafe {


mbuesch · 2025-01-15T16:54:17Z

src/lib.rs

-    let sock = UnixDatagram::unbound()?;
-    sock.connect(socket_path)?;
-    Ok(Some(sock))
+fn connected_unix_datagram(path: &OsStr) -> io::Result<UnixDatagram> {


What about Path/PathBuf for paths in general? Path/PathBuf implement AsRef<OsStr>

It's a private function so we only have to support what we pass. Generics increase code bloat, because whole functions get duplicated.

Generics increase code bloat, because whole functions get duplicated.

No?

From the short answer, I can't exactly tell what you're disagreeing with.

In Rust, the way generics work is that the function will be compiled once for each set of type parameters it is called with. So if we have a function fn conected_unix_datagram<T: AsRef<OsStr>>(path: &T) -> … (which is the same as fn connected_unix_datagram(path: &impl AsRef<OsStr>) -> …) and call it with &OsStr and once with &OsString, it'll end up being in the compiled binary twice. This is what I meant with "code bloat". The Rust standard library mitigates that by having each function that takes an AsRef generic be a thin wrapper around a function without generics, like e.g. in https://github.com/rust-lang/rust/blob/0c2c096e1ac471b0c34629f9820a7cb1e6d4695d/library/std/src/fs.rs#L3008-L3014. Since the function is non-public, we control all the call sites and all the call sites just want to pass a &OsStr, I thought it'd be unnecessary to do the helper function.

Since this is your crate, I can implement this in whatever way you'd like.

Generics with helper function (like std does for public functions).

Generics without helper function (like I interpreted your first comment).

No generics (like it's currently implemented).

Which way would you like?

From the short answer, I can't exactly tell what you're disagreeing with.

I disagree that using &Path instead of &OsStr causes bloat or duplication of any kind.

Path is the ideomatic type for paths. That's why I suggested it.

So if we have a function fn conected_unix_datagram<T: AsRef>(path: &T)

I did not suggest that.

I also misunderstood initially, you're proposing for that function to take &Path. That would work, but it's not super important either way, it's a private helper with one or two callers.

mbuesch · 2025-01-15T16:56:23Z

src/lib.rs

@@ -297,19 +297,21 @@ fn listen_fds_internal(unset_env: bool) -> io::Result<impl ExactSizeIterator<Ite
    Ok(listen_fds)
 }

+pub unsafe fn listen_fds_and_unset_env() -> io::Result<impl ExactSizeIterator<Item = RawFd>> {
+    let result = listen_fds();
+    unsafe {


... old Rust versions warning. See above.

mbuesch · 2025-01-15T16:57:31Z

src/lib.rs

+
+pub unsafe fn listen_fds_with_names_and_unset_env() -> io::Result<impl ExactSizeIterator<Item = (RawFd, String)>> {
+    let result = listen_fds_with_names();
+    unsafe {


... old Rust versions warning. See above.

mbuesch · 2025-01-15T16:58:43Z

src/lib.rs

@@ -445,6 +418,15 @@ pub fn watchdog_enabled(unset_env: bool, usec: &mut u64) -> bool {
    }
 }

+pub unsafe fn watchdog_enabled_and_unset_env(usec: &mut u64) -> bool {
+    let result = watchdog_enabled(usec);
+    unsafe {


... old Rust versions warning. See above.

This does not fix the actual issue but alerts users to this unsoundness issue. Also changes the examples so that the parameter isn't set to `true` mindlessly. CC lnicola#9

They need to be `unsafe` because `std::env::remove_var` is unsafe. Fixes lnicola#9.

mbuesch suggested changes Jan 15, 2025

View reviewed changes

tbu- force-pushed the pr_unset_env_unsafe branch from d9ed7a9 to 33917ab Compare January 16, 2025 10:49

tbu- changed the title ~~Note that using the unset_env parameter is unsafe~~ Duplicate all public functions with _and_unset_env Jan 16, 2025

tbu- added 2 commits January 16, 2025 11:55

Note that using the unset_env parameter is unsafe

7ea57d0

This does not fix the actual issue but alerts users to this unsoundness issue. Also changes the examples so that the parameter isn't set to `true` mindlessly. CC lnicola#9

Duplicate all public functions with _and_unset_env

affc143

They need to be `unsafe` because `std::env::remove_var` is unsafe. Fixes lnicola#9.

tbu- force-pushed the pr_unset_env_unsafe branch from 33917ab to affc143 Compare January 16, 2025 10:56

tbu- mentioned this pull request Jan 16, 2025

Change watchdog_enabled to use Duration #14

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Duplicate all public functions with `_and_unset_env` #13

Duplicate all public functions with `_and_unset_env` #13

tbu- commented Jan 15, 2025 •

edited

Loading

lnicola commented Jan 15, 2025

tbu- commented Jan 15, 2025

lnicola commented Jan 15, 2025

tbu- commented Jan 15, 2025

mbuesch left a comment

mbuesch Jan 15, 2025

tbu- Jan 16, 2025

mbuesch Jan 16, 2025

tbu- Jan 17, 2025

mbuesch Jan 17, 2025

lnicola Jan 17, 2025

mbuesch Jan 15, 2025

mbuesch Jan 15, 2025

tbu- Jan 16, 2025

mbuesch Jan 16, 2025

tbu- Jan 17, 2025 •

edited

Loading

mbuesch Jan 17, 2025

lnicola Jan 17, 2025

mbuesch Jan 15, 2025

mbuesch Jan 15, 2025

mbuesch Jan 15, 2025

Duplicate all public functions with _and_unset_env #13

Are you sure you want to change the base?

Duplicate all public functions with _and_unset_env #13

Conversation

tbu- commented Jan 15, 2025 • edited Loading

lnicola commented Jan 15, 2025

tbu- commented Jan 15, 2025

lnicola commented Jan 15, 2025

tbu- commented Jan 15, 2025

mbuesch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbu- Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Duplicate all public functions with `_and_unset_env` #13

Duplicate all public functions with `_and_unset_env` #13

tbu- commented Jan 15, 2025 •

edited

Loading

tbu- Jan 17, 2025 •

edited

Loading