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

mi.h: Use ccan/endian/endian.h instead of endian.h #835

Merged
merged 1 commit into from
May 6, 2024

Conversation

bsdimp
Copy link
Contributor

@bsdimp bsdimp commented May 3, 2024

[[ edited ]]
When both endian.h and ccan/endian/endian.h are included, we can have
__{BIG,LITTLE}_ENDIAN redefined when compiling with clang on FreeBSD.
Clang and gcc have moved to a predefine for endian orders. glibc defines
these the same as they are defied here, but that's an unsafe assumption
to make. Instead, only define them when __LITTLE_ENDIAN not defined as a
fallback to when the host does not define them in the standard system
headers.

Copy link
Contributor

@tbzatek tbzatek left a comment

Choose a reason for hiding this comment

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

Unfortunately <mi.h> is part of public API as included from <libnvme-mi.h> while ccan is a private in-tree library.

@bsdimp
Copy link
Contributor Author

bsdimp commented May 6, 2024

OK. Then I'll update ccan/endian/endian.h to only define these if they aren't already defined. glibc already defines them, but there's no warning because it's the same thing. FreeBSD has migrated to using the clang/gcc builtin values, and so gets a redefinition warning. Thanks for the feedback.

When both endian.h and ccan/endian/endian.h are included, we can have
__{BIG,LITTLE}_ENDIAN redefined when compiling with clang on FreeBSD.
Clang and gcc have moved to a predefine for endian orders. glibc defines
these the same as they are defied here, but that's an unsafe assumption
to make. Instead, only define them when __LITTLE_ENDIAN not defined as a
fallback to when the host does not define them in the standard system
headers.

Signed-off-by: Warner Losh <[email protected]>
@igaw
Copy link
Collaborator

igaw commented May 6, 2024

The fix looks good as far I understand. Though we should try to get this upstream to the ccan project eventually.

@igaw igaw merged commit 26b0eaf into linux-nvme:master May 6, 2024
14 checks passed
@bsdimp
Copy link
Contributor Author

bsdimp commented May 6, 2024

The fix looks good as far I understand. Though we should try to get this upstream to the ccan project eventually.

Do you have a pointer to ccan? Google is returning results about 'can' and not so much where to find the upstream. If it's an open project, I'd be happy to do the legwork to push it upstream there.

@martin-belanger
Copy link
Contributor

martin-belanger commented May 6, 2024

@bsdimp - I think this is the ccan repo location:
https://github.com/rustyrussell/ccan

@bsdimp
Copy link
Contributor Author

bsdimp commented May 6, 2024

Thanks! For your tracking purposes:
rustyrussell/ccan#114

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants