Thread: Return value of PathNameOpenFile()

I've noticed that some callers of PathNameOpenFile()
(e.g. bbsink_server_begin_archive()) consider the call failed even if the
function returned zero, while other ones do check whether the file descriptor
is strictly negative. Since the file descriptor is actually returned by the
open() system call, I assume that zero is a valid result, isn't it?

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

> On 6 Sep 2022, at 09:26, Antonin Houska <ah@cybertec.at> wrote:
>
> I've noticed that some callers of PathNameOpenFile()
> (e.g. bbsink_server_begin_archive()) consider the call failed even if the
> function returned zero, while other ones do check whether the file descriptor
> is strictly negative. Since the file descriptor is actually returned by the
> open() system call, I assume that zero is a valid result, isn't it?

Agreed, zero should be valid as it's a non-negative integer.  However, callers
in fd.c are themselves checking for (fd <= 0) in some cases, and some have done
so since the very early days of the codebase, so I wonder if there historically
used to be a platform which considered 0 an invalid fd?

--
Daniel Gustafsson        https://vmware.com/

Daniel Gustafsson <daniel@yesql.se> writes:
> Agreed, zero should be valid as it's a non-negative integer.  However, callers
> in fd.c are themselves checking for (fd <= 0) in some cases, and some have done
> so since the very early days of the codebase, so I wonder if there historically
> used to be a platform which considered 0 an invalid fd?

I'm betting it's a thinko that never got caught because 0 would
always be taken up by stdin.  Maybe you'd notice if you tried to
close-and-reopen stdin, but that's not something the server ever does.

            regards, tom lane

> On 6 Sep 2022, at 16:12, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Daniel Gustafsson <daniel@yesql.se> writes:
>> Agreed, zero should be valid as it's a non-negative integer.  However, callers
>> in fd.c are themselves checking for (fd <= 0) in some cases, and some have done
>> so since the very early days of the codebase, so I wonder if there historically
>> used to be a platform which considered 0 an invalid fd?
>
> I'm betting it's a thinko that never got caught because 0 would
> always be taken up by stdin.  Maybe you'd notice if you tried to
> close-and-reopen stdin, but that's not something the server ever does.

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?

--
Daniel Gustafsson        https://vmware.com/

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.  There might well be some "<= 0"
versus "< 0" errors in this area, but it requires closer inspection.

            regards, tom lane

> 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/