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/
Attachments:
pathnameopenfile_fd0.diffapplication/octet-stream; name=pathnameopenfile_fd0.diff; x-unix-mode=0644Download
diff --git a/src/backend/backup/basebackup_server.c b/src/backend/backup/basebackup_server.c
index d020a92bfa..2e29f6a092 100644
--- a/src/backend/backup/basebackup_server.c
+++ b/src/backend/backup/basebackup_server.c
@@ -143,7 +143,7 @@ bbsink_server_begin_archive(bbsink *sink, const char *archive_name)
mysink->file = PathNameOpenFile(filename,
O_CREAT | O_EXCL | O_WRONLY | PG_BINARY);
- if (mysink->file <= 0)
+ if (mysink->file < 0)
ereport(ERROR,
(errcode_for_file_access(),
errmsg("could not create file \"%s\": %m", filename)));
@@ -236,7 +236,7 @@ bbsink_server_begin_manifest(bbsink *sink)
mysink->file = PathNameOpenFile(tmp_filename,
O_CREAT | O_EXCL | O_WRONLY | PG_BINARY);
- if (mysink->file <= 0)
+ if (mysink->file < 0)
ereport(ERROR,
(errcode_for_file_access(),
errmsg("could not create file \"%s\": %m", tmp_filename)));
diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
index 20c3741aa1..bdd82679fd 100644
--- a/src/backend/storage/file/fd.c
+++ b/src/backend/storage/file/fd.c
@@ -1629,7 +1629,7 @@ PathNameDeleteTemporaryDir(const char *dirname)
File
OpenTemporaryFile(bool interXact)
{
- File file = 0;
+ File file = -1;
Assert(temporary_files_allowed); /* check temp file access is up */
@@ -1662,7 +1662,7 @@ OpenTemporaryFile(bool interXact)
* tablespace. MyDatabaseTableSpace should normally be set before we get
* here, but just in case it isn't, fall back to pg_default tablespace.
*/
- if (file <= 0)
+ if (file < 0)
file = OpenTemporaryFileInTablespace(MyDatabaseTableSpace ?
MyDatabaseTableSpace :
DEFAULTTABLESPACE_OID,
@@ -1728,7 +1728,7 @@ OpenTemporaryFileInTablespace(Oid tblspcOid, bool rejectError)
*/
file = PathNameOpenFile(tempfilepath,
O_RDWR | O_CREAT | O_TRUNC | PG_BINARY);
- if (file <= 0)
+ if (file < 0)
{
/*
* We might need to create the tablespace's tempfile directory, if no
@@ -1742,7 +1742,7 @@ OpenTemporaryFileInTablespace(Oid tblspcOid, bool rejectError)
file = PathNameOpenFile(tempfilepath,
O_RDWR | O_CREAT | O_TRUNC | PG_BINARY);
- if (file <= 0 && rejectError)
+ if (file < 0 && rejectError)
elog(ERROR, "could not create temporary file \"%s\": %m",
tempfilepath);
}
@@ -1777,7 +1777,7 @@ PathNameCreateTemporaryFile(const char *path, bool error_on_failure)
* temp file that can be reused.
*/
file = PathNameOpenFile(path, O_RDWR | O_CREAT | O_TRUNC | PG_BINARY);
- if (file <= 0)
+ if (file < 0)
{
if (error_on_failure)
ereport(ERROR,
@@ -1815,7 +1815,7 @@ PathNameOpenTemporaryFile(const char *path, int mode)
file = PathNameOpenFile(path, mode | PG_BINARY);
/* If no such file, then we don't raise an error. */
- if (file <= 0 && errno != ENOENT)
+ if (file < 0 && errno != ENOENT)
ereport(ERROR,
(errcode_for_file_access(),
errmsg("could not open temporary file \"%s\": %m",
diff --git a/src/backend/storage/file/fileset.c b/src/backend/storage/file/fileset.c
index 9c63f2b267..367d975fff 100644
--- a/src/backend/storage/file/fileset.c
+++ b/src/backend/storage/file/fileset.c
@@ -100,7 +100,7 @@ FileSetCreate(FileSet *fileset, const char *name)
file = PathNameCreateTemporaryFile(path, false);
/* If we failed, see if we need to create the directory on demand. */
- if (file <= 0)
+ if (file < 0)
{
char tempdirpath[MAXPGPATH];
char filesetpath[MAXPGPATH];
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/