merge file_exists_in_directory and _fileExistsInDirectory functions and move into common file dumputils.c

On Thu, Apr 10, 2025 at 10:41:33PM +0530, Mahendra Singh Thalor wrote:

We have file_exists_in_directory function in pg_restore.c and same
code we are using in _fileExistsInDirectory function in pg_backup_archiver.c
also.
Here, I am attaching a patch to move these duplicate functions into
dumputils.c file

Indeed. I don't quite see a reason not to remove this duplication,
and both routines in pg_restore.c and the pg_dump code are the same.

dumputils.h is only used by pg_dump and pg_dumpall, and its top
comment documents exactly that, so using this location for a routine
that would be used by a pg_restore path is a bit strange to me for
something that is qualified as a "dump" routine in your patch.

Perhaps we should just use a more centralized place, like file_utils.c
so as all frontends could benefit of it?

Please make sure to add it to the next commit fest that will begin in
July, this refactoring proposal is too late to be considered for v18.
--
Michael

On 2025-Apr-11, Michael Paquier wrote:

Perhaps we should just use a more centralized place, like file_utils.c
so as all frontends could benefit of it?

I'm not sure about that. This code looks to be making too many
assumptions that aren't acceptable for a general routine, such as
complaining only that the directory name is long without the possibility
that the culprit is the file name. It's more or less okay in current
uses because they're all using harcoded short names, but that would not
hold in general. At the same time, isn't every call of this routine a
potential TOCTTOU bug? Again it's probably fine for the current code,
but I wouldn't be too sure about making this generally available as-is.

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Oh, great altar of passive entertainment, bestow upon me thy discordant images
at such speed as to render linear thought impossible" (Calvin a la TV)

On Fri, 11 Apr 2025 at 10:21, Michael Paquier <michael@paquier.xyz> wrote:

On Thu, Apr 10, 2025 at 10:41:33PM +0530, Mahendra Singh Thalor wrote:

We have file_exists_in_directory function in pg_restore.c and same
code we are using in _fileExistsInDirectory function in

pg_backup_archiver.c

also.
Here, I am attaching a patch to move these duplicate functions into
dumputils.c file

Indeed. I don't quite see a reason not to remove this duplication,
and both routines in pg_restore.c and the pg_dump code are the same.

Thanks Michael for the feedback.

dumputils.h is only used by pg_dump and pg_dumpall, and its top
comment documents exactly that, so using this location for a routine
that would be used by a pg_restore path is a bit strange to me for
something that is qualified as a "dump" routine in your patch.

Perhaps we should just use a more centralized place, like file_utils.c
so as all frontends could benefit of it?

I tried to add it into file_utils.c but I was getting many "symbols not
found errors" so I moved this common function into dumputils.h as we have
another common function in that file. (Ex; create_or_open_dir)

If we want to move this function into file_utils.c, then I can try to
rewrite the patch.

Please make sure to add it to the next commit fest that will begin in
July, this refactoring proposal is too late to be considered for v18.
--
Michael

Okay. Thank you. I will add it.

On Fri, 11 Apr 2025 at 15:08, Álvaro Herrera <alvherre@kurilemu.de> wrote:

On 2025-Apr-11, Michael Paquier wrote:

Perhaps we should just use a more centralized place, like file_utils.c
so as all frontends could benefit of it?

I'm not sure about that. This code looks to be making too many
assumptions that aren't acceptable for a general routine, such as
complaining only that the directory name is long without the possibility
that the culprit is the file name. It's more or less okay in current
uses because they're all using harcoded short names, but that would not
hold in general. At the same time, isn't every call of this routine a
potential TOCTTOU bug? Again it's probably fine for the current code,
but I wouldn't be too sure about making this generally available as-is.

--
Álvaro Herrera 48°01'N 7°57'E —

https://www.EnterpriseDB.com/

"Oh, great altar of passive entertainment, bestow upon me thy discordant

images

at such speed as to render linear thought impossible" (Calvin a la TV)

Thanks Álvaro for the feedback.

/*

* file_exists_in_directory
*
* Returns true if the file exists in the given directory.
*/
static bool
file_exists_in_directory(const char *dir, const char *filename)
{
struct stat st;
char buf[MAXPGPATH];

if (strlen(dir) >= MAXPGPATH)
pg_fatal("directory name too long: \"%s\"", dir);

if (strlen(filename) >= MAXPGPATH)
pg_fatal("file name too long: \"%s\"", filename);

/* Now check path length of dir/filename */
if (snprintf(buf, MAXPGPATH, "%s/%s", dir, filename) >= MAXPGPATH)
pg_fatal("combined name of directory:\"%s\" and file:\"%s\" is too
long", filename, dir);

return (stat(buf, &st) == 0 && S_ISREG(st.st_mode));
}

I did changes as per above code and added some checks in code to give an
error for "too long" name. I started a new thread[1]/messages/by-id/CAKYtNApPmWmU9rdf__D=cA7ivL6H_UrPc=w0CHW74P2acxJJ-g@mail.gmail.com for "too long names"
check and later I will post an updated patch here to move
duplicate functions into one common file.

[1]: /messages/by-id/CAKYtNApPmWmU9rdf__D=cA7ivL6H_UrPc=w0CHW74P2acxJJ-g@mail.gmail.com
/messages/by-id/CAKYtNApPmWmU9rdf__D=cA7ivL6H_UrPc=w0CHW74P2acxJJ-g@mail.gmail.com

--
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com