> On 6 Sep 2022, at 21:44, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Daniel Gustafsson <daniel@yesql.se> writes:
>> Doh, of course. The attached is a quick (lightly make check tested) take on
>> allowing 0, but I'm not sure that's what we want?
>
> Actually, wait a second. At least some of these are not dealing
> with kernel FDs but with our "virtual FD" abstraction. For those,
> zero is indeed an invalid value.
Yes and no, I think; PathNameOpenFile kind of returns a Vfd on success but a
kernel fd on failure which makes this a bit confusing. Abbreviated for space,
the code looks like this:
file = AllocateVfd();
vfdP = &VfdCache[file];
...
vfdP->fd = BasicOpenFilePerm(fileName, fileFlags, fileMode);
if (vfdP->fd < 0)
{
...
return -1;
}
...
return file;
So if the underlying BasicOpenFilePerm fails then open(2) failed and we return
-1, which is what open(2) returned. If it succeeds, then we return the Vfd
returned by AllocateVfd which can never be zero, as thats the VFD_CLOSED
ringbuffer anchor. Since AllocateVfd doesn't return on error, it's easy to
confuse oneself on exactly which error is propagated.
Checking for (fd <= 0) on an fd returned from PathNameOpenFile is thus not
wrong, albeit wearing both belts and suspenders since it can never return 0.
Changing them seem like codechurn for no reason even if the check for 0 is
superfluous. The callsites that only check for (fd < 0) are equally correct,
and need not be changed.
Callers of BasicOpenFile need however allow for zero since they get the kernel
fd back, which AFAICT from scanning that they all do.
So in summary, I can't spot a callsite which isn't safe.
--
Daniel Gustafsson https://vmware.com/
