-
Notifications
You must be signed in to change notification settings - Fork 1k
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
add ptp_clock_caps
#4128
add ptp_clock_caps
#4128
Conversation
51e3d56
to
a2478a1
Compare
src/unix/linux_like/linux/mod.rs
Outdated
@@ -1206,6 +1206,81 @@ s! { | |||
} | |||
} | |||
|
|||
cfg_if! { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so yeah, this looks like the best I can do...
Defining a PTP_CLOCK_CAPS_RSV_LEN
const just does not seem to work. not with cfg_if, a vanilly block with an if expression, defining a const fn
and calling that: they all run into unreachable code in garando.
Next it tusrns out that loongarch
is weird and special (or just has a later version maybe?), so that further complicates the situation.
It looks like I can bring it down to three branches by just making the conditionals more complex (that almost adds as many lines as it saves...) Any other ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where do all these definitions even come from? I don't see it in glibc or musl source so I assume it should be uapi source, but I only see the one definition with no conditions https://github.com/torvalds/linux/blob/43fb83c17ba2d63dfb798f0be7453ed55ca3f9c2/include/uapi/linux/ptp_clock.h#L94-L107
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, all the special cases are older versions where they don't have new fields and have more elements in that rsv
array. So it might be possible to just have the latest version and just ignore all targets/CI configurations where that does not (yet) work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh all of this was just accounting for what we happen to test in CI rather than actual sources? Why didn't you just say so :) we should definitely only implement the latest and skip tests on platforms where we can explain the failure.
If this explains the platform-specific config for the other PTP structs (I think there was something for sparc
), mind putting up a PR to drop that as well?
I suggested a few changes here but think this should mostly be good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the rest of the ptp structs are clean (no cfg's or anything like that). For the ioctl const functions there is some powerpc/sparc/mips-specific logic, but that really is because they define constants with different values. There are links to those values there so I think it's all good now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM, I must have been thinking of something else. Thanks for the followup.
72b85fb
to
1d29316
Compare
1d29316
to
38318cd
Compare
thanks for the help here @tgross35, that was a journey! |
(backport <rust-lang#4128>) (cherry picked from commit 38318cd)
(backport <rust-lang#4128>) (cherry picked from commit 38318cd)
(backport <rust-lang#4128>) (cherry picked from commit 38318cd)
(backport <rust-lang#4128>) (cherry picked from commit 38318cd)
Description
The final missing pieces of #3865
Sources
Checklist
libc-test/semver
have been updated*LAST
or*MAX
areincluded (see #3131)
cd libc-test && cargo test --target mytarget
);especially relevant for platforms that may not be checked in CI
r? @tgross35