util: Detect posix_fallocate() instead of assuming

vasild commented at 10:18 am on March 26, 2020: member

Don’t assume that posix_fallocate() is available on Linux and not available on other operating systems. At least FreeBSD has it and we are not using it.

Properly check whether posix_fallocate() is present and use it if it is.

vasild commented at 10:19 am on March 26, 2020: member

To verify that this works as intended:

Check if you have posix_fallocate()
./configure ... |grep posix_fallocate - the result should coincide with 1.
Put something like this in src/util/system.cpp and try to recompile:

0#if defined(HAVE_POSIX_FALLOCATE)
1#error posix_fallocate is present
2#else
3#error posix_fallocate is not present
4#endif

The printout should coincide with 1. and 2.

fanquake added the label Utils/log/libs on Mar 26, 2020

fanquake commented at 10:43 am on March 26, 2020: member

Concept ACK

laanwj commented at 11:47 am on March 26, 2020: member

ACK 61d4bee72f3800edd00ec565313c820b536954ec

Good catch. Generally spoken the detection of specific features is preferable to OS-specific paths. As it is a POSIX function I suppose we can assume that the functionality and parameters are the same over all platforms that have it?

jonatack commented at 2:19 pm on March 26, 2020: member

ACK 61d4bee72

Concept ACK on feature detection > OS detection. I looked for a Portable Operating System Interface compatibility/portability matrix and came up with https://en.wikipedia.org/wiki/POSIX#POSIX-oriented_operating_systems, https://www.opengroup.org/openbrand/register/, and the second answer in https://stackoverflow.com/questions/1780599/what-is-the-meaning-of-posix. One issue is that many distributions are compliant but don’t want to pay for certification.

Code review, verified man posix_fallocate, reproduced #18437 (comment) with Debian 4.19

0checking for posix_fallocate... yes
1(pr/18437)$ make | grep posix_fallocate
2util/system.cpp:14:2: error: #error posix_fallocate is present
3 #error posix_fallocate is present
4  ^~~~~

vasild commented at 3:00 pm on March 26, 2020: member

Yes, Linux, FreeBSD and NetBSD have posix_fallocate() (maybe others too). The types and semantics of the arguments and return value are the same on all.

I am a bit surprised that MacOS and OpenBSD do not have it. The code handles MacOS separately anyway.

Thanks!

DrahtBot commented at 3:38 pm on March 26, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

practicalswift commented at 5:03 pm on March 26, 2020: contributor

Concept ACK

Thanks for all nice contributions lately @vasild. Nice to have you as a recurring contributor!

Empact commented at 6:06 pm on March 26, 2020: member

Concept ACK

I think you may also need to adjust _POSIX_C_SOURCE in your autoconf test - see: https://github.com/bitcoin/bitcoin/blob/694f4cbd7853ffeb7b4576d42368920b0ef79b01/src/util/system.cpp#L20-L29

Context: https://www.gnu.org/software/libc/manual/html_node/Feature-Test-Macros.html#index-_005fPOSIX_005fC_005fSOURCE http://man7.org/linux/man-pages/man3/posix_fallocate.3.html#CONFORMING_TO

Perhaps we could check the defined version rather than adjusting it ourselves?

vasild force-pushed on Mar 27, 2020

vasild commented at 11:34 am on March 27, 2020: member

@Empact Right, good catch!

The patch had a deficiency that if run on a system that does not provide posix_fallocate() by default but would make it available if the user explicitly asks for it (by defining _POSIX_C_SOURCE to 200112), then on such system the configure check would conclude that the function is not present and so it would not be used in the source code (even though it would have been available if we asked for it).

I updated the configure check to explicitly request for it, exactly like the source code does in src/util/system.cpp.

Perhaps we could check the defined version rather than adjusting it ourselves?

That would miss the function in the above imaginary system (some old Linux?) - if it is available only if one asks for it (define _POSIX_C_SOURCE to 200112 or higher). I think we are doing the right thing on Linux with the _POSIX_C_SOURCE macro - we define it to ask for certain functions, not just peeking at it. From feature_test_macros(7):

Feature test macros allow the programmer to control the definitions that are exposed by system header files

Empact commented at 7:53 pm on March 28, 2020: member

ACK https://github.com/bitcoin/bitcoin/pull/18437/commits/14f11083e4ed733520872fb6da3fe59182745db9

util: Detect posix_fallocate() instead of assuming

Don't assume that `posix_fallocate()` is available on Linux and not
available on other operating systems. At least FreeBSD has it and we
are not using it.

Properly check whether `posix_fallocate()` is present and use it if it
is.

182dbdf0f4

in configure.ac:858 in 14f11083e4 outdated

850@@ -851,6 +851,21 @@ AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[#include <malloc.h>]],
851  [ AC_MSG_RESULT(no)]
852 )
853 
854+dnl Check for posix_fallocate
855+AC_MSG_CHECKING(for posix_fallocate)
856+AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[
857+                   #ifdef __linux__

laanwj commented at 12:31 pm on April 1, 2020:

Not sure this is correct. Is POSIX_C_SOURCE a Linux or a general GCC thing? I don’t think it hurts to leave out the #ifdef __linux__ here.

vasild commented at 1:36 pm on April 1, 2020:

_POSIX_C_SOURCE is not Linux or GCC specific. However, only the Linux man page for posix_fallocate() mentions it as a requirement. FreeBSD and NetBSD man pages don’t, implying that the function is available without having to define _POSIX_C_SOURCE to a specific value.

In any case, this is what the source code in src/util/system.cpp already does and it’s important to have the check and source code do the same thing.

Empact commented at 9:28 pm on April 11, 2020:

Maybe worth adding comments cross-referencing the identical setups?

vasild commented at 7:51 am on April 14, 2020:

Done

vasild force-pushed on Apr 14, 2020

luke-jr approved

luke-jr commented at 0:00 am on April 23, 2020: member

utACK

laanwj commented at 8:44 am on April 30, 2020: member

I dislike the duplication of #ifdef logic, but okay, as long as it’s commented…

ACK 182dbdf0f4b6e6484b0d4588aaefacc75862a99c

laanwj merged this on Apr 30, 2020

laanwj closed this on Apr 30, 2020

vasild deleted the branch on Apr 30, 2020

vasild commented at 3:34 pm on April 30, 2020: member

I dislike the duplication of #ifdef logic, but okay, as long as it’s commented…

I did consider putting this:

0#ifdef __linux__
1
2#ifdef _POSIX_C_SOURCE
3#undef _POSIX_C_SOURCE
4#endif
5
6#define _POSIX_C_SOURCE 200112L
7
8#endif // __linux__

in a separate header file and then including that header file from configure.ac and from system.cpp, but decided it would be an overkill. Something to keep in mind should we need to do similar thing again.

sidhujag referenced this in commit c1c3e93ee7 on May 2, 2020

Fabcien referenced this in commit a952590df7 on Jan 29, 2021

DrahtBot locked this on Feb 15, 2022

util: Detect posix_fallocate() instead of assuming #18437

Conflicts