Thomas Munro <thomas.munro@gmail.com> writes:
>> On Wed, Sep 2, 2020 at 4:35 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Hm. If we do this, I can see wanting to apply the knowledge in more
>>> places than walkdir().
> Good idea. Here's a new version that defines a new function
> get_dirent_type() in src/common/file_utils_febe.c and uses it for both
> frontend and backend walkdir().
Quick thoughts on this patch:
* The API spec for get_dirent_type() needs to say that errno is
meaningful when the return value is PGFILETYPE_ERROR. That's
something that would not be hard to break, so not documenting
the point at all doesn't seem great. More generally, I don't
especially like having the callers know that the errno is from
stat() rather than something else.
* I don't quite like the calling code you have that covers some
return values and then has a default: case without any comment.
It's not really obvious that the default: case is expected to be
hit in non-error situations, especially when there is a separate
switch path for errors. I can't find fault with the code as such,
but I think it'd be good to have a comment there. Maybe along
the lines of "Ignore file types other than regular files and
directories".
Both of these concerns would abate if we had get_dirent_type()
just throw an error itself when stat() fails, thereby removing the
PGFILETYPE_ERROR result code. I'm not 100% sold either way on
that, but it's something to think about. Is there ever going
to be a reason for the caller to ignore an error?
regards, tom lane
