CESNET
Musl patches for 2.1.0
Hello again:
We have various build targets with different archs, C libraries, etc. Most of them use Musl, but one of the ARM boards this is targeted for actually does use traditional glibc. Everything I add also has to work for our 32- and 64-bit simulation environments as well.
These are the cross-compile targets I'm working with:
- arm-glibc
- x86-musl
- x86_64-musl
There are 2 commits here.
The first commit addresses the usage of pthread NP:
In applying the same kind of Musl patch for 'libnetconf2', https://github.com/CESNET/libnetconf2/commit/153fe40bd60499677e825e66501e8601536e0323 , I noticed that it actually doesn't work. check_function_exists(pthread_rwlockattr_setkind_np HAVE_PTHREAD_RWLOCKATTR_SETKIND_NP) returns false / 'not found' on that arm-glibc target, which definitely does have it in that toolchain's "pthread.h" file.
So I ended up writing a patch for 'IPFIXcol2' that uses the slightly different CMake check_symbol_exists, which is actually recommended as a replacement by CMake. See note at the bottom of their docs here: https://cmake.org/cmake/help/latest/module/CheckFunctionExists.html
You might want to do a similar change in your 'libnetconf2'
arm-glibc target now:
-- The C compiler identification is GNU 5.5.0
-- The CXX compiler identification is GNU 5.5.0
...
-- Found LibFds: /mnt/ipfixcol2.2.1.0-patch/.odedeps/usr/lib/libfds.so (found suitable version "0.2.0", minimum required is "0.2.0")
-- Looking for pthread.h
-- Looking for pthread.h - found
-- Looking for pthread_create
-- Looking for pthread_create - not found
-- Check if compiler accepts -pthread
-- Check if compiler accepts -pthread - yes
-- Found Threads: TRUE
-- Looking for pthread_rwlockattr_setkind_np
-- Looking for pthread_rwlockattr_setkind_np - found
-- Found ZLIB: /mnt/ipfixcol2.2.1.0-patch/.odedeps/usr/lib/libz.so (found version "1.2.11")
...
[ 75%] Building CXX object src/plugins/output/json/CMakeFiles/json-output.dir/src/File.cpp.o
cd /mnt/ipfixcol2.2.1.0-patch/src/plugins/output/json && /usr/bin/ccache /opt/toolchains/crosstools-arm-gcc-5.5-linux-4.1-glibc-2.26-binutils-2.28.1/bin/arm-buildroot-linux-gnueabi-g++ -Djson_output_EXPORTS -I/mnt/ipfixcol2.2.1.0-patch/include -I/mnt/ipfixcol2.2.1.0-patch/src -I/mnt/ipfixcol2.2.1.0-patch/.odedeps/usr/include -Os -pipe -g3 -fno-caller-saves -rdynamic -DNGX -I/mnt/ipfixcol2.2.1.0-patch/.odedeps/usr/include -I/opt/toolchains/crosstools-arm-gcc-5.5-linux-4.1-glibc-2.26-binutils-2.28.1/arm-buildroot-linux-gnueabi/include -I/opt/toolchains/crosstools-arm-gcc-5.5-linux-4.1-glibc-2.26-binutils-2.28.1/arm-buildroot-linux-gnueabi/sysroot/usr/include -fvisibility=hidden -std=gnu++11 -DHAVE_PTHREAD_RWLOCKATTR_SETKIND_NP -O2 -DNDEBUG -fPIC -o CMakeFiles/json-output.dir/src/File.cpp.o -c /mnt/ipfixcol2.2.1.0-patch/src/plugins/output/json/src/File.cpp
Notice it adds -DHAVE_PTHREAD_RWLOCKATTR_SETKIND_NP to CFLAGS. I don't know if this was the best way to do it or not.
x86_64-musl target now:
-- The C compiler identification is GNU 7.3.0
-- The CXX compiler identification is GNU 7.3.0
...
-- Found LibFds: /mnt/ipfixcol2.2.1.0-patch/.odedeps/usr/lib/libfds.so (found suitable version "0.2.0", minimum required is "0.2.0")
-- Looking for pthread.h
-- Looking for pthread.h - found
-- Looking for pthread_create
-- Looking for pthread_create - found
-- Found Threads: TRUE
-- Looking for pthread_rwlockattr_setkind_np
-- Looking for pthread_rwlockattr_setkind_np - not found
-- Found ZLIB: /mnt/ipfixcol2.2.1.0-patch/.odedeps/usr/lib/libz.so (found version "1.2.11")
...
[ 75%] Building CXX object src/plugins/output/json/CMakeFiles/json-output.dir/src/File.cpp.o
cd /mnt/ipfixcol2.2.1.0-patch/src/plugins/output/json && /usr/bin/ccache /opt/toolchains/bin/x86_64-openwrt-linux-musl-g++ -Djson_output_EXPORTS -I/mnt/ipfixcol2.2.1.0-patch/include -I/mnt/ipfixcol2.2.1.0-patch/src -I/mnt/ipfixcol2.2.1.0-patch/.odedeps/usr/include -Os -pipe -fno-caller-saves -g3 -rdynamic -fhonour-copts -fstack-protector -D_FORTIFY_SOURCE=1 -Wl,-z,now -Wl,-z,relro -Wno-error=unused-const-variable -I/opt/toolchains/include -I/mnt/ipfixcol2.2.1.0-patch/.odedeps/usr/include -I/opt/toolchains/x86_64-openwrt-linux-musl/include -fvisibility=hidden -std=gnu++11 -O2 -DNDEBUG -fPIC -o CMakeFiles/json-output.dir/src/File.cpp.o -c /mnt/ipfixcol2.2.1.0-patch/src/plugins/output/json/src/File.cpp
The second commit addresses the usage of strerror_r:
From manpage of 'strerror' group: https://man7.org/linux/man-pages/man3/strerror.3.html There are 2 signatures of strerror_r: XSI and GNU. Musl does not do any GNU extensions, so it only provides the XSI signature returning an int. strerror_r is considered non-portable and deprecated anyways, so replace with strerror_l.
"libnl" did something similar: https://lists.infradead.org/pipermail/libnl/2016-August/002196.html
/mnt/ipfixcol2.2.1.0-patch/src/plugins/output/json/src/File.cpp: In static member function 'static int File::dir_create(ipx_ctx_t*, const string&)':
/mnt/ipfixcol2.2.1.0-patch/src/plugins/output/json/src/File.cpp:346:45: error: invalid conversion from 'int' to 'const char*' [-fpermissive]
const char *err_str = strerror_r(errno, buffer, 128);
~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~
With these 2 commits, I can successfully build for all my targets, and I'll consider that it fixes #34
Thank you for the patch.
I will try to merge it soon but I will have to resolve some collisions with newer changes in devel branch. Since I'm quite busy right now, I will do it early next week.
Using strerror_r and strerror_l is always painful. The first one has two different definitions and the latter cannot use LC_GLOBAL_LOCALE. These functions are just stupidly designed.
However, just after brief look at the code of the patch, I can tell that I'm not fan of code duplication and I'm not sure whether the string returned by strerror_l can be considered as valid after freelocale().
Hey, sounds good. This PR is just an equivalent to the patches that we are now using internally, and I wanted to make sure it got shared upstream :)
That's also why I split this into 2 commits -- the second one with the strerror changes is definitely more hacky. I do agree though, the copy/pasted segments are bad and it would be much better as a function somewhere. I initially did that, however, I didn't really see a good common place in the 'json' plugin to put it. Sorry about that!
For the strerror_l code though, it was largely taken from the popular "libnl" doing the same thing here, so it should be valid: https://github.com/thom311/libnl/blob/master/lib/utils.c#L120
You don't have to actually approve & merge this PR btw. More or less, I'm just pointing out some issues with Musl and how I resolved them. If you just want to git cherry-pick -x <sha> the individual commits into your "devel" branch and clean it up as you see fit, or just ignore this entirely, both are totally OK. I don't mean to create work for you :)
Thanks!