logging: remove unnecessary drop on memfd

[dfreese]

This was added in #88, however, drop is already implicitly called on all objects when they go out of scope. This, for memfd happens a couple lines later, so the drop is redundant. The primary problem that #88 solved was changing into_raw_fd() to as_raw_fd(), which no longer transferred ownership, requiring a manual close.

From the std::os::fd::IntoRawFd docs:

This function is typically used to transfer ownership of the underlying file descriptor to the caller. When used in this way, callers are then the unique owners of the file descriptor and must close it once it’s no longer needed.

It seems that the primary problem that close was not being called on memfd when it was consumed into a RawFd.

[swsnr]

I added this drop deliberately, fully aware that it's redundant 😬 , to document clear and plain that we do want to close our side of the FD. The semantics of sending FDs around aren't always obvious [1] 🤯 , so I thought it'd be a good idea to have this explicit, and to have a clear place where we can add a comment stating what the intent is.

Of course it's redundant, but I trust that the compiler can optimize it away, so it doesn't cost us anything to close the FD explicitly and thus have a place where we can explicitly document that we do want do that.

---

[1]: After all, both @lucab and me went through this code a bunch of times and never noticed that we were leaking an FD here 🙈

[swsnr]

I added this drop deliberately, fully aware that it's redundant 😬 , to document clear and plain that we do want to close our side of the FD. The semantics of sending FDs around aren't always obvious [1] 🤯 , so I thought it'd be a good idea to have this explicit, and to have a clear place where we can add a comment stating what the intent is.

Of course it's redundant, but I trust that the compiler can optimize it away, so it doesn't cost us anything to close the FD explicitly and thus have a place where we can explicitly document that we do want do that.

So in a way this drop is just documentation 😇

But I'm also fine with removing it if you folks feel it doesn't add anything 🙂

---

[1]: After all, both @lucab and me went through this code a bunch of times and never noticed that we were leaking an FD here 🙈

[dfreese]

Thanks for the background. Your call on this. I just spent a while trying to understand why it was necessary.

[swsnr]

I just spent a while trying to understand why it was necessary.

Oh dear, I'm sorry. 🙈 😇

Perhaps let's find some middle ground and extend the comment a bit, e.g.

Explicitly drop our side of the FD.

Sending an FD across a socket duplicates the FD in the other process;
it does not consume the FD in this process, so we need to close it explicitly
or we leak one FD for every large payload message.

Technically, this drop is redundant because memfd is dropped when it goes
out of scope at the end of this function, but it serves as a place to put this
comment at.

But perhaps that's a little over the top, so we can also just remove the drop.

Let's get @lucab's opinion here.

[dfreese]

I've added another commit which has an alternative, more type-heavy formulation of this that might help make this more explicit. Let me know what you think. My view would be that adding more to the comment seems to complicate the idea of "std::fs::File automatically closes itself when it goes out of scope, but into_raw_fd breaks guarantee, so use wisely."

[swsnr]

I think that's overkill, and doesn't really match my intention.

I didn't intend to abstract over the FD, I wanted to clarify that we retain ownership of the FD even when we send it across a socket. Journald ends up with an independent copy, so we still need to close our FD. I don't think your change models this. In other words, the point is not to hide the memfd, the point is describe ownership of an FD that's send across a socket.

If the drop bothers you I'd rather prefer to wait until we can bump MSRV to 1.66 and model this with BorrowedFd, i.e. add a new send_one_fd function which takes a BorrowedFd and documents that the FD is duplicated across the socket.

[swsnr]

In other words, I'd like to get rid of as_raw_fd and RawFd which have no notion of ownership, before I remove the drop, because in the current code I think the explicit drop reaffirms that it's really intended that the current process retains owernship of the FD and closes it.

I find this important to clarify, because this information is buried deep in unix(7) and even there it's not stated explicitly that the FD is still owned by the current process 🙈

[dfreese]

In other words, I'd like to get rid of as_raw_fd and RawFd which have no notion of ownership, before I remove the drop

RawFd is used in the nix interface, so I'm closing this.