naemon / naemon-livestatus

Naemon - Livestatus Eventbroker Module
GNU General Public License v2.0
26 stars 30 forks source link

fix since glibc 2.24 deprecated readdir_r #75

Closed pbiering closed 4 years ago

pbiering commented 4 years ago

split-out of https://github.com/naemon/naemon-livestatus/pull/71

"readdir_r" is deprecated since glibc 2.24, "readdir" should be used (again)

pbiering commented 4 years ago

no clue why Travis CI build fails...code compiles clean, error message later is imho unrelated to code change...but who knows...

jacobbaungard commented 4 years ago

Do we actually need the if statements here? I think readdir has been available for a long time, so should work fine even on glibc versions prior to 2.24.

jacobbaungard commented 4 years ago

I re-triggered the Travis build and worked fine, so I assume it was not related to this change.

pbiering commented 4 years ago

Do we actually need the if statements here? I think readdir has been available for a long time, so should work fine even on glibc versions prior to 2.24.

the question is more since which glibc version readdir is reentrant-save. If already in glibc-version(EL6) then it can be potentially unconditionally replaced, otherwise not.

jacobbaungard commented 4 years ago

I think readdir has been safe in glibc for a long time, so if it works OK as in this PR past glibc 2.24 I think we are good without the if statements. @sni any thoughts?

It is recommended that applications use readdir(3) instead of readdir_r(). Furthermore, since version 2.24, glibc deprecates readdir_r(). The reasons are as follows:

  • On systems where NAME_MAX is undefined, calling readdir_r() may be unsafe because the interface does not allow the caller to specify the length of the buffer used for the returned directory entry.

  • On some systems, readdir_r() can't read directory entries with very long names. When the glibc implementa‐ tion encounters such a name, readdir_r() fails with the error ENAMETOOLONG after the final directory entry has been read. On some other systems, readdir_r() may return a success status, but the returned d_name field may not be null terminated or may be truncated.

  • In the current POSIX.1 specification (POSIX.1-2008), readdir(3) is not required to be thread-safe. However, in modern implementations (including the glibc implementation), concurrent calls to readdir(3) that specify different directory streams are thread-safe. Therefore, the use of readdir_r() is generally unnecessary in multithreaded programs. In cases where multiple threads must read from the same directory stream, using readdir(3) with external synchronization is still preferable to the use of readdir_r(), for the reasons given in the points above.

  • It is expected that a future version of POSIX.1 will make readdir_r() obsolete, and require that readdir(3) be thread-safe when concurrently employed on different directory streams.

jacobbaungard commented 4 years ago

The commit that seems to introduce locking into the readdir in gblic i 20+ years old.

Of course it might be different on other platforms.

sni commented 4 years ago

I'd try to keep the code clean and do not add unnecessary ifdefs and go with readdir. If it is recommended to use readdir, then we shoud do that.

jacobbaungard commented 4 years ago

Applied without the ifdefs here: https://github.com/naemon/naemon-livestatus/commit/0abe207aa4957086efda2b899ff2f0aa341cf7e2

Thanks!