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.

Sign up for GitHub

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

Jump to bottom

Add 64bit rdev APIs #329

Merged
merged 1 commit into from
Sep 6, 2024
Merged

Add 64bit rdev APIs #329

merged 1 commit into from
Sep 6, 2024

Conversation

cgwalters
Copy link
Contributor

I stumbled across the fact that on Linux/glibc at least dev_t is 64 bits, but we were truncating it to 32 bits.

Note that the type used in the mkcomposefs binary was already uint64_t but because C happily truncates/converts integers this was missed.

cgwalters added a commit to cgwalters/composefs that referenced this pull request Sep 4, 2024
@cgwalters
rust: Accept 64 bit values for rdev
30aabdf 
Pairs with containers#329
which adapts the C side to properly accept these too.

This breaks semver, we'll need to bump that.

Signed-off-by: Colin Walters <walters@verbum.org>
@cgwalters cgwalters mentioned this pull request Sep 4, 2024
Merged
allisonkarlitskaya
allisonkarlitskaya reviewed Sep 5, 2024
View reviewed changes
Copy link
Collaborator

@allisonkarlitskaya allisonkarlitskaya left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I wonder if we could use a macro to direct users towards the new variant on recompile, dropping the old declaration from the headers (but keeping it defined to avoid breaking ABI). Would save some churn...

@cgwalters
Copy link
Contributor Author

I wonder if we could use a macro to direct users towards the new variant on recompile, dropping the old declaration from the headers (but keeping it defined to avoid breaking ABI). Would save some churn...

But if any callers had been interacting with this via uint32_t (as would be reasonable to do) and they also weren't using -Wconversion they'd not know that they need to take action. I think forcing the churn is the right thing here.

@cgwalters
Add 64bit rdev APIs
ca6cf4e 
I stumbled across the fact that on Linux/glibc at least `dev_t` is
64 bits, but we were truncating it to 32 bits.

Note that the type used in the `mkcomposefs` binary was already uint64_t
but because C happily truncates/converts integers this was missed.

Signed-off-by: Colin Walters <walters@verbum.org>
allisonkarlitskaya
allisonkarlitskaya approved these changes Sep 6, 2024
View reviewed changes
Copy link
Collaborator

@allisonkarlitskaya allisonkarlitskaya left a comment

Choose a reason for hiding this comment

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

lgtm

@cgwalters cgwalters merged commit f8bb72a into containers:main Sep 6, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@allisonkarlitskaya allisonkarlitskaya allisonkarlitskaya approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cgwalters @allisonkarlitskaya