Skip to content

CDRIVER-4679 Prefer XSI-compliant strerror_r when available #1350

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.

Sign up for GitHub

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

Merged
eramongodb merged 3 commits into from
Jul 24, 2023
Merged

CDRIVER-4679 Prefer XSI-compliant strerror_r when available #1350

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b221b80
CDRIVER-4679 Prefer XSI-compliant strerror_r when available
eramongodb Jul 18, 2023
caf670c
Use CheckCSourceCompiles to test for XSI-compliant strerror_r
eramongodb Jul 21, 2023
37abf49
Slight wording improvements
eramongodb Jul 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -394,7 +394,7 @@ endif ()
# Enable POSIX features up to POSIX.1-2008 plus the XSI extension and BSD-derived definitions.
# Both _BSD_SOURCE and _DEFAULT_SOURCE are defined for backwards-compatibility with glibc 2.19 and earlier.
# _BSD_SOURCE and _DEFAULT_SOURCE are required by `getpagesize`, `h_errno`, etc.
# _XOPEN_SOURCE=700 is required by `strnlen`, etc.
# _XOPEN_SOURCE=700 is required by `strnlen`, `strerror_r`, etc.
add_definitions (-D_XOPEN_SOURCE=700 -D_BSD_SOURCE -D_DEFAULT_SOURCE)
list (APPEND CMAKE_REQUIRED_DEFINITIONS -D_XOPEN_SOURCE=700 -D_BSD_SOURCE -D_DEFAULT_SOURCE)

Expand Down
11 changes: 8 additions & 3 deletions src/libbson/src/bson/bson-error.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,12 +109,17 @@ bson_strerror_r (int err_code, /* IN */
if (strerror_s (buf, buflen, err_code) != 0) {
ret = buf;
}
#elif defined(__GNUC__) && defined(_GNU_SOURCE)
ret = strerror_r (err_code, buf, buflen);
#else /* XSI strerror_r */
#elif defined(_XOPEN_SOURCE) && _XOPEN_SOURCE >= 600 && !defined(_GNU_SOURCE)
Copy link
Member

@jmikola jmikola Jul 20, 2023

Choose a reason for hiding this comment

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

https://stackoverflow.com/a/65745713/162228 was a helpful reminder that undefined macros evaluate to zero.

What is the argument for not using the logic suggested in strerror_r(3)?

(_POSIX_C_SOURCE >= 200112L || _XOPEN_SOURCE >= 600) && ! _GNU_SOURCE

I'm now familiar with what platforms define _POSIX_C_SOURCE and/or _XOPEN_SOURCE, but it would seem more flexible to me to allow either. Likewise, ! _GNU_SOURCE would also handle the case where _GNU_SOURCE is defined but zero (not sure how relevant that is).

Also, I'm not sure if this line from the man page is relevant:

If no feature test macros are explicitly defined, then (since glibc 2.4) _POSIX_SOURCE is defined by default with the value 200112L, so that the XSI-compliant version of strerror_r() is provided by default.

I don't see _POSIX_SOURCE referred to anywhere within libmongoc. It's mentioned once in the bundled zlib sources.

The versions of glibc mentioned in the man page predate 2011, so perhaps none of this is relevant to any supported platform.

Copy link
Contributor Author

@eramongodb eramongodb Jul 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

What is the argument for not using the logic suggested in strerror_r(3)?

_XOPEN_SOURCE implies _POSIX_C_SOURCE per https://pubs.opengroup.org/onlinepubs/7908799/xsh/compilation.html: "Since this specification is aligned with the ISO C standard, and since all functionality enabled by _POSIX_C_SOURCE set greater than zero and less than or equal to 199506L should be enabled by _XOPEN_SOURCE set equal to 500, there should be no need to define either _POSIX_SOURCE or _POSIX_C_SOURCE if _XOPEN_SOURCE is so defined."

Likewise, !_GNU_SOURCE would also handle the case where _GNU_SOURCE is defined but zero (not sure how relevant that is).

Only the presence of _GNU_SOURCE matters, not its value, per https://man7.org/linux/man-pages/man7/feature_test_macros.7.html: "Defining this macro (with any value) implicitly defines [...]. In addition, various GNU-specific extensions are also exposed."

I don't see _POSIX_SOURCE referred to anywhere within libmongoc.

It is unconditionally defined via the definition of _XOPEN_SOURCE here:

mongo-c-driver/CMakeLists.txt

Line 398 in 6d2be25

add_definitions (-D_XOPEN_SOURCE=700 -D_BSD_SOURCE -D_DEFAULT_SOURCE)

All our requirements concerning POSIX and extensions are being handled by the (unconditional) definition of the macros listed in the link above. The conflict between the XSI-compliant version and GNU extension version of strerror_r is AFAIK a unique case (among the features being used by the C Driver).

Copy link
Member

@jmikola jmikola Jul 21, 2023

Choose a reason for hiding this comment

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

Thanks for the rigorous response! LGTM.

// The presence of _GNU_SOURCE determines whether the POSIX
// XSI-conforming version or the GNU extension is available.
if (strerror_r (err_code, buf, buflen) == 0) {
ret = buf;
}
#elif defined(_GNU_SOURCE)
// Fallback to GNU extension.
ret = strerror_r (err_code, buf, buflen);
#else
#error "Unable to find a supported strerror_r candidate"
#endif

if (!ret) {
Expand Down