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

fix: (Util.cpp) use static_cast<int> on FICLONE, port downstream patch form alpine linux #960

Closed
wants to merge 3 commits into from

Conversation

MaxPeal
Copy link

@MaxPeal MaxPeal commented Nov 5, 2021

port downstream patch form alpine linux
enable support for file cloning on ppc64le
"musl uses an int instead of a unsigend long for the ioctl function
prototype, contrary to glibc, since POSIX mandates the former. This
causes a spurious error on ppc64le which can be silenced by casting to
int explicitly.

See https://www.openwall.com/lists/musl/2020/01/20/2"
https://gitlab.alpinelinux.org/alpine/aports/-/blob/de8bce08376a189ad14809d0272bb4c02e74cdf0/main/ccache/ioctl.patch

port downstream patch form alpine linux
enable support for file cloning on ppc64le
"musl uses an `int` instead of a `unsigend long` for the ioctl function
prototype, contrary to glibc, since POSIX mandates the former. This
causes a spurious error on ppc64le which can be silenced by casting to
int explicitly.

See https://www.openwall.com/lists/musl/2020/01/20/2"
https://gitlab.alpinelinux.org/alpine/aports/-/blob/de8bce08376a189ad14809d0272bb4c02e74cdf0/main/ccache/ioctl.patch
@jrosdahl jrosdahl added improvement Improvement that is not a bug fix or new feature portability Affects portability of building or using Ccache labels Nov 6, 2021
@jrosdahl
Copy link
Member

jrosdahl commented Nov 6, 2021

Looks good to me. We're generally using static_cast<int>(...) rather than (int)..., though.

@MaxPeal
Copy link
Author

MaxPeal commented Nov 9, 2021

@jrosdahl Thank you, PR updated as suggested.
must i squash this PR?

@MaxPeal MaxPeal changed the title fix: (Util.cpp) use int on FICLONE, port downstream patch form alpine linux fix: (Util.cpp) use static_cast<int> on FICLONE, port downstream patch form alpine linux Nov 9, 2021
@MaxPeal MaxPeal marked this pull request as ready for review November 9, 2021 11:14
@jrosdahl
Copy link
Member

jrosdahl commented Nov 9, 2021

must i squash this PR?

No, that's not necessary.


I had a closer look now and I'm not yet convinced that the cast is appropriate. The original patch description says "spurious error". Does that mean a real error or a compile-time warning?

On all glibc-based Linux dists the second argument to ioctl should be unsigned long, not int. Is there a risk of errors or warnings in a non-musl case if the value is cast to int?

If there is need to cast to int for musl it spontaneously feels like it should be done only when compiling with musl.

@andypost
Copy link

btw Linux using unsigned int https://github.com/torvalds/linux/blob/master/fs/ioctl.c#L823

@jrosdahl jrosdahl added the awaiting feedback Waiting for response from issue/PR author label Feb 21, 2022
@ccache-bot
Copy link

ccache-bot bot commented Mar 21, 2022

This issue has been automatically closed due to no response to our request for more information from the original author. Please feel free to leave a comment to reopen the issue if you have more information.

@ccache-bot ccache-bot bot closed this Mar 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting feedback Waiting for response from issue/PR author improvement Improvement that is not a bug fix or new feature portability Affects portability of building or using Ccache
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants