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

preload: wrap fstatfs() and statfs() on musl #224

Closed
wants to merge 1 commit into from

Conversation

alyssais
Copy link

musl has both of these functions (though not the separate "64" variants that glibc), and since systemd 251, a wrapped fstatfs() is required for udevmock's own test suite to pass, unless SYSTEMD_DEVICE_VERIFY_SYSFS is set to 0.

musl has both of these functions (though not the separate "64"
variants that glibc), and since systemd 251, a wrapped fstatfs() is
required for udevmock's own test suite to pass, unless
SYSTEMD_DEVICE_VERIFY_SYSFS is set to 0.
@martinpitt
Copy link
Owner

Thanks! But why did you move that code? First of all, it makes it very hard to see what actually changed (I don't see the musl fix, for example), and second it's now in a wrong place. If you want to move the code for aesthetic reasons, please do that as a separate commit, and then one with just the musl fix. Cheers!

@alyssais
Copy link
Author

I didn't move it for aesthetic reasons, I moved it so it wasn't in the #ifdef __GLIBC__. Because I moved most of the code that was previously in there out, the way GitHub renders the diff is the #ifdef moving rather than the code that was previously inside it, as that results in a smaller diff.

@martinpitt
Copy link
Owner

I see, thanks. However, I still don't like ripping apart the fstatfs{,64} and statfs{,64} declarations like that, they should stay together -- with the 64 variants being ifdeff'ed for glibc. I sent PR #225 with what I had in mind, which passes tests and seems to work well. If you care at all about having your name in "Author:" instead of just the commit message, please feel free to grab that diff, massage it a little bit if you like, and push it here -- I'm totally fine with that 😁

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.

2 participants