Re: readdir is incorrectly implemented at Windows

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: Konstantin Knizhnik <k(dot)knizhnik(at)postgrespro(dot)ru>
Cc: PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: readdir is incorrectly implemented at Windows
Date: 2019-03-01 06:13:29
Message-ID: 20190301061329.GA31098@paquier.xyz
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Thu, Feb 28, 2019 at 11:15:53AM +0900, Michael Paquier wrote:
> Could you add it to the next commit fest as a bug fix please? I think
> that I will be able to look at that in details soon, but if not it
> would be better to not lose track of your fix.

Okay, I have looked at your patch. And double-checked on Windows. To
put it short, I agree with the approach you are taking. I have been
curious about the mention to MinGW in the existing code as well as in
the patch you are proposing, and I have checked if what your patch and
what the current state are correct, and I think that HEAD is wrong on
one thing.

First mingw64 code can be found here:
https://sourceforge.net/p/mingw-w64/mingw-w64/ci/master/tree/
https://github.com/Alexpux/mingw-w64/

Then, the implementation of readdir/opendir/closedir can be found in
mingw-w64-crt/misc/dirent.c. Looking at their implementation of
readdir, I can see two things:
1) When beginning a search in a directory, _tfindfirst gets used,
which returns ENOENT as error if no matches are found:
https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/findfirst-functions?view=vs-2017
So from this point of view your patch is right: you make readdir()
return errno=0 which matches the normal *nix behaviors. And MinGW
does not do that.
2) On follow-up lookups, MinGW code uses _tfindnext, and actually
*enforces* errno=0 when seeing ERROR_NO_MORE_FILES. So from this
point of view the code's comment in HEAD is incorrect as of today.
The current implementation exists since 399a36a so perhaps MinGW was
not like that when dirent.c has been added in src/port/, but that's
not true today. So let's fix the comment at the same time.

Attached is an updated patch with my suggestions. Does it look fine
to you?

Also, I think that we should credit Yuri Kurenkov for the discovery of
the issue, with yourself, Konstantin, as the author of the patch.
Are there other people involved which should be credited? Like
Grigory?
--
Michael

Attachment Content-Type Size
readdir-win32-fix.patch text/x-diff 733 bytes

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2019-03-01 06:16:12 Re: Prevent extension creation in temporary schemas
Previous Message Etsuro Fujita 2019-03-01 05:48:42 Re: [HACKERS] CLUSTER command progress monitor