[bkueng]

This goes together with this libc PR: rust-lang/libc#5555

[rustbot]

Thanks for the pull request, and welcome! The Rust Project has assigned @LawnGnome (or someone else) to review your changes, you should hear from them (or someone else) within the next two weeks.

Please see the contribution instructions and our LLM policy for more information.

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: @ChrisDenton, libs
• @ChrisDenton, libs expanded to 13 candidates
• Random selection from 7 candidates

[LawnGnome]

Just catching up on the last month of notifications, sorry. I don't have any context on nuttx, so I'm going to re-roll this.

@rustbot reroll

[hanna-kruppe]

I don't know anything about nuttx either, cc @no1wudi as target maintainer

From a quick glance it looks like rust-lang/libc#5555 was merged now, but only into the main branch, so it's only in the libc 1.0 pre-release, not in any 0.2.x release. My understanding is that more steps are needed before std can use the updated libc definitions:

1. Backport the libc change to the 0.2 branch
2. Wait until a new libc 0.2.x release includes the change
3. Update the standard library workspace to use that libc version

However, looking at the actual changes here, I'm not sure if this is the right way. Duplicating various struct literals just to add fields which are internal to the libc implementation on some platform doesn't seem great for maintainability. Perhaps there's a way to rewrite the affected code that works the same on all platforms including NuttX?

[bkueng]

so it's only in the libc 1.0 pre-release, not in any 0.2.x release. My understanding is that more steps are needed before std can use the updated libc definitions:

I'm not sure what the update process for libc is, I just wanted to provide the required changes upfront so it won't break.

However, looking at the actual changes here, I'm not sure if this is the right way. Duplicating various struct literals just to add fields which are internal to the libc implementation on some platform doesn't seem great for maintainability.

I agree, do you see a better solution? The primary alternative I see is using Default:

let mut pollfd = libc::pollfd {
            fd: self.as_raw_fd(),
            events: libc::POLLOUT,
            revents: 0,
            ..Default::default()
    };

Which would require implementing Default for pollfd.

[hanna-kruppe]

I would suggest the pattern of creating the C struct with mem::zeroed() and setting the relevant fields by assigning afterwards. Similar to what's done here, for example:

rust/library/std/src/sys/process/unix/pidfd.rs

Lines 31 to 32 in 96a18bf

	let mut pidfd_info: libc::pidfd_info = unsafe { crate::mem::zeroed() };
	pidfd_info.mask = libc::PIDFD_INFO_PID as u64;

Then we should be able to merge this PR independently of when the libc fix makes it into a libc 0.2.x release and that release is adopted by the standard library.

@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[bkueng]

I would suggest the pattern of creating the C struct with mem::zeroed() and setting the relevant fields by assigning afterwards.

That works too (if unsafe is acceptable), and I updated accordingly.

[hanna-kruppe]

Thanks, r=me after removing the need for allow(unused_imports).

View changes since this review

[hanna-kruppe]

Instead of allowing unused imports, please either use the full path without import, or move the import into the cfg block where it's needed (like the use crate::sys::io::errno).

[bkueng]

I changed it to the full path.

[hanna-kruppe]

Setting events to zero explicitly seems a bit redundant in this context. But I could see the argument that it's more consistent to always set the "input" parts explicitly. So I'm fine with either, just wanted to ask if this is an intentional choice.

[bkueng]

Yes I considered both and ended up setting it so that someone reading it does not get the impression it was forgotten (pfds[0].fd = 0; could also be removed).

[hanna-kruppe]

@bors r+ rollup

[rust-bors]

📌 Commit 2cab4ca has been approved by hanna-kruppe

It is now in the queue for this repository.