Replace readdir_r

[kripton]

This PR replaces the deprecated readdir_r with the non-deprecated readdir.
A unit test is added to make sure we don't break anything. Tested on the old readdir_r and the new code.

Open for discussions :)

[kripton]

Oh, forgot: Fixes #1055

[kripton]

Forgot another thing: I also wanted to revert #1168. That might lead to more warning being shown, so not tested yet. Will do so later. So please don't merge yet.

[kripton]

Okay, Travis tests are failing due to out-of-tree builds. I'll check how other tests are handling test data

[kripton]

Okay, I'd say this is ready for review & comments. New unit test is passing on Travis, existing tests are flaky and currently sometimes fail non macOS. Doesn't seem to be related to these changes.

[peternewman]

A few comments

[kripton]

@peternewman : All comments addressed (code changed or reasoning added ;)). Ready for a new round of review

[kripton]

Oh, right, linting pointed out some stuff I saw just now. Will also take care of that

[kripton]

All lint fixes done and rebased onto latest 0.10 branch

[peternewman]

A few comments and improvements if you wouldn't mind.

[peternewman]

Two more comments

[peternewman]

Apologies for the flurry of commits @kripton but I've now got Codespell working again on Travis which would need to be sorted to merge this anyway, hope you don't mind.

[kripton]

No need to excuse for the Travis-fixing commits.
I've taken care of your comments.
About your comment on:

I'm unclear what a successful opendir call and an errorno value would mean, although it seems to say it shouldn't.

As far as I understand: If opendir fails, it'll return NULL and set errno so one can see why it failed. If it succeeds, it returns a value and probably leaves errno untouched. However, it could also set a value. But since there is no errno-value for "success", it probably doesn't. So if a value is returned, just ignore errno. I wouldn't know which value to ASSERT here. But before calling readdir, I set it to 0 to make sure it has a defined value.
I agree that a successful opendir WITH an value in errno would mean. But ASSERTING it to be 0 when the manuals says it could be changed seems a bit risky to me.

[peternewman]

I agree that a successful opendir WITH an value in errno would mean. But ASSERTING it to be 0 when the manuals says it could be changed seems a bit risky to me.

Ah yeah true. I see your point.

If we can have a comment briefly explaining why it's being set to 0 (due to the NULL thing), and then one other style comment and we're good to go thanks.

Feel free to take some credit for this and other bits you've done in AUTHORS if you'd like, and add a line to NEWS (or I can do the latter).

[kripton]

Hi @peternewman , I've added comments regarding errno = 0, changed the ifs to else ifs and modified NEWS and AUTHORS. Feel free to modify the two latter files if you prefer a different wording :)

[peternewman]

Sorry a handful of minor style bits I'd somehow missed before.

Thanks for fixing Codespell too!

[kripton]

One thing I noticed: Travis build on macOS fails due to newer libmicrohttpd. I'm experiencing the same on my Gentoo Linux machine. v0.9.70 is fine, v0.9.71 fails.

[peternewman]

One thing I noticed: Travis build on macOS fails due to newer libmicrohttpd. I'm experiencing the same on my Gentoo Linux machine. v0.9.70 is fine, v0.9.71 fails.

Oops, that's embarrassing, I'd just assumed it was the tests that are always flaky on Mac and hadn't looked yet.

Looks like it's someone else who doesn't understand semver! See for example https://bugs.archlinux.org/task/67208 which links to https://git.gnunet.org/libmicrohttpd.git/commit/?id=6347f514aa2388e774d5bf356df8046864e5f73c and their fix seems to be here: systemd/systemd@d17eabb#diff-2d7e61049e387b011a9df20244a7922d

Do you want to chuck all the necessary #ifdef cotton wool in, either in this PR or another one? We'd normally deal with it like this:

ola/libs/usb/LibUsbAdaptor.cpp

Lines 175 to 183 in a4f2c0c

	string LibUsbAdaptor::ErrorCodeToString(const int error_code) {
	#ifdef HAVE_LIBUSB_ERROR_NAME
	return libusb_error_name(error_code);
	#else
	ostringstream str;
	str << "Error code " << error_code;
	return str.str();
	#endif // HAVE_LIBUSB_ERROR_NAME
	}

See also: xbmc/xbmc#18131 and https://git.gnunet.org/gnunet.git/tree/src/include/gnunet_mhd_compat.h

[kripton]

Yep, that libmicrohttpd-changes are a pity. I'll create a separate PR for that, feel free to create an issue to track the build breakage.

[kripton]

@peternewman I think I've done all changes + pushed now :) Let's see what Travis says, despite the expected-for-now failures on macOS

[kripton]

#1650 created to track the libmicrohttpd build failures. However, now Travis fails on macOS due to some python file conflicts.....

[peternewman]

However, now Travis fails on macOS due to some python file conflicts.....

From what I can see, it's trying to upgrade the Homebrew packages, so postgis pulls in numpy via Homebrew but because numpy is already installed via pip it fails. Hopefully Travis will get their act together soon, or I can force merge it, or manually delete files/uninstall from pip or something...

[peternewman]

LGTM, thanks for this one too!