Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adding a handful of linux fanotify data types. #3695

Merged
merged 1 commit into from
Sep 3, 2024

Conversation

devnexen
Copy link
Contributor

@devnexen devnexen commented May 11, 2024

close #3688

here the involved struct

this one, this one and this one.

@rustbot
Copy link
Collaborator

rustbot commented May 11, 2024

r? @JohnTitor

rustbot has assigned @JohnTitor.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@devnexen devnexen force-pushed the gh3688 branch 4 times, most recently from d9c6f5a to 6cc82b3 Compare May 11, 2024 14:19
@tgross35
Copy link
Contributor

I think this looks good, but can you confirm that fanotify_event_info_pidfd and fanotify_event_info_error are GNU-only? Also needs a rebase.

@tgross35
Copy link
Contributor

@rustbot author

@devnexen
Copy link
Contributor Author

I think this looks good, but can you confirm that fanotify_event_info_pidfd and fanotify_event_info_error are GNU-only? Also needs a rebase.

yes I confirm no presence in other libcs

@devnexen devnexen force-pushed the gh3688 branch 3 times, most recently from 96bb2f7 to 8de6537 Compare August 15, 2024 19:12
@tgross35
Copy link
Contributor

I think this should be

@rustbot ready

If you have a link to the headers / docs that would be great, trying to get them linked in every PR so we can find them again when needed.

Comment on lines 67 to 68
#[repr(align(2))]
pub struct fanotify_event_info_header {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/torvalds/linux/blob/0c3836482481200ead7b416ca80c68a29cfdaabd/include/uapi/linux/fanotify.h#L153 doesn't seem to have an alignment attribute, and I think the default alignment should naturally be 16 bits. Why is it needed here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I think this would cause a problem building on ancient versions with alignment since this doesn't have a fallback

Comment on lines +457 to +504
pub struct fanotify_event_info_pidfd {
pub hdr: ::fanotify_event_info_header,
pub pidfd: ::__s32,
}

pub struct fanotify_event_info_error {
pub hdr: ::fanotify_event_info_header,
pub error: ::__s32,
pub error_count: ::__u32,
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these GNU-only? I don't see them reexported in glibc, only uapi

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it s a bit of a mess on musl. with glibc it s enough to include sys/fanotify.h which in turn include linux/fanotify.h so you have all you need for this. in musl sys/fanotify.h which does not include linux/fanotify.h and if you do include both there are couple of type redefinition conflicts.

@tgross35
Copy link
Contributor

tgross35 commented Sep 3, 2024

@rustbot author

@devnexen
Copy link
Contributor Author

devnexen commented Sep 3, 2024

@rustbot review

Copy link
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm, could you squash please?

@bors
Copy link
Contributor

bors commented Sep 3, 2024

☔ The latest upstream changes (presumably #3480) made this pull request unmergeable. Please resolve the merge conflicts.

@tgross35 tgross35 enabled auto-merge September 3, 2024 20:46
@tgross35 tgross35 added stable-nominated This PR should be considered for cherry-pick to libc's stable release branch S-waiting-on-ci and removed S-waiting-on-review labels Sep 3, 2024
@tgross35 tgross35 added this pull request to the merge queue Sep 3, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 3, 2024
@tgross35 tgross35 added this pull request to the merge queue Sep 3, 2024
Merged via the queue into rust-lang:main with commit 72c4000 Sep 3, 2024
41 checks passed
@tgross35 tgross35 mentioned this pull request Oct 15, 2024
tgross35 pushed a commit to tgross35/rust-libc that referenced this pull request Oct 15, 2024
tgross35 pushed a commit to tgross35/rust-libc that referenced this pull request Oct 15, 2024
tgross35 pushed a commit to tgross35/rust-libc that referenced this pull request Oct 15, 2024
@tgross35 tgross35 added stable-applied This PR has been cherry-picked to libc's stable release branch and removed stable-nominated This PR should be considered for cherry-pick to libc's stable release branch labels Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-ci stable-applied This PR has been cherry-picked to libc's stable release branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add struct fanotify_event_info_fid
5 participants