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

move AF_XDP structs and constants to linux/mod.rs #4163

Merged
merged 1 commit into from
Nov 28, 2024

Conversation

arctic-alpaca
Copy link
Contributor

Description

As discussed in #3956, the AF_XDP structs and constants are all uapi and as such should go into src/unix/linux_like/linux/mod.rs. This PR moves all AF_XDP structs and constants there.

The _v1 versions are for backwards compatibility with older kernel versions. With v1.0, they could probably be removed, since the newer versions will work (with new fields not being taken into account by the kernel). If that is desirable, I can crate a follow-up PR to remove them or remove them here.

Sources

  • All structs and constants not explicitly listed are defined in if_xdp.h.
  • xdp_ring_offset_v1 & xdp_mmap_offsets_v1 in xsk.h.
  • xdp_umem_reg_v1 & xdp_statistics_v1 in xsk.c.

Checklist

  • Relevant tests in libc-test/semver have been updated
  • No placeholder or unstable values like *LAST or *MAX are
    included (see #3131)
  • Tested locally (cd libc-test && cargo test --target mytarget);
    especially relevant for platforms that may not be checked in CI

I could not test the the changes locally due to the error described in #3865 (comment)

@rustbot
Copy link
Collaborator

rustbot commented Nov 27, 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

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.

Thanks!

If the _v1 structs aren't intended to be used anymore, could you mark them as deprecated here? Then this can go to 0.2, and they can be removed in a separate PR that only goes to main.

@arctic-alpaca arctic-alpaca force-pushed the unify-af-xdp branch 2 times, most recently from 501b2fc to 2a21a93 Compare November 28, 2024 07:16
@arctic-alpaca
Copy link
Contributor Author

If the CI failure is related to the changes, I cannot tell what caused it.


#[deprecated(
since = "0.2.167",
note = "We consider removing this as the newer version of this struct will work with older kernel versions. If you're using it, please comment on https://github.com/rust-lang/libc/issues/4168"
Copy link
Contributor

Choose a reason for hiding this comment

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

Slight rewording to suggest an alternative, and to rewrap to 100 characters. Could you update the others as well?

Suggested change
note = "We consider removing this as the newer version of this struct will work with older kernel versions. If you're using it, please comment on https://github.com/rust-lang/libc/issues/4168"
note = "Use `xdp_ring_offset` instead, which will still work with old kernel versions. If \
you still need this version, please comment at \
https://github.com/rust-lang/libc/issues/4168"

After this please squash and we should be able to merge.

@tgross35
Copy link
Contributor

Nah CI is just Android getting stuck, that happens for some reason

@arctic-alpaca
Copy link
Contributor Author

I just realized that I made a mistake with the deprecation. The older versions might be used to check sizes for when another version gets added. Basically checking the length the kernel returns against foo_v1, foo_v2 and if it's larger than both, going with foo. Currently there are no _v2 structs, but that's only the case because of the bug mentioned in #3956. So while the up-to-date structs would work, there would be no way to check which version the kernel used if/when _v2 versions get added.

I'm not sure whether you would prefer to keep them in, or to only support the latest version of the Linux Kernel.

Sorry for getting this wrong. If it's fine for you, I'd remove the deprecation info and close the issue so that this PR only contains the move to the src/unix/linux_like/linux/mod.rs file.

@tgross35
Copy link
Contributor

I think I need to double check what the kernel headers/docs say to get a better understanding. Feel free to drop deprecation from this PR, we can revisit later.

@arctic-alpaca
Copy link
Contributor Author

Thanks, sorry again for the trouble.

@tgross35
Copy link
Contributor

No trouble at all :)

@tgross35 tgross35 added this pull request to the merge queue Nov 28, 2024
Merged via the queue into rust-lang:main with commit 6c26c2a Nov 28, 2024
45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants