util: Detect posix_fallocate() instead of assuming #18437

pull vasild wants to merge 1 commits into bitcoin:master from vasild:posix_fallocate changing 2 files +18 −2
  1. 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.

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

    To verify that this works as intended:

    1. Check if you have posix_fallocate()

    2. ./configure ... |grep posix_fallocate - the result should coincide with 1.

    3. 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.

  3. fanquake added the label Utils/log/libs on Mar 26, 2020
  4. fanquake commented at 10:43 am on March 26, 2020: member
    Concept ACK
  5. 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?

  6. 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  ^~~~~
  7. 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!

  8. 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.

  9. 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!

  10. 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?

  11. vasild force-pushed on Mar 27, 2020
  12. 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

  14. 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
  15. 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
  16. vasild force-pushed on Apr 14, 2020
  17. luke-jr approved
  18. luke-jr commented at 0:00 am on April 23, 2020: member
    utACK
  19. 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

  20. laanwj merged this on Apr 30, 2020
  21. laanwj closed this on Apr 30, 2020
  22. vasild deleted the branch on Apr 30, 2020
  23. 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.

  24. sidhujag referenced this in commit c1c3e93ee7 on May 2, 2020
  25. Fabcien referenced this in commit a952590df7 on Jan 29, 2021
  26. DrahtBot locked this on Feb 15, 2022


vasild fanquake laanwj jonatack DrahtBot practicalswift Empact luke-jr

Labels
Utils/log/libs

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-23 09:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me