pg_check_dir comments and implementation mismatch
Hi,
reading pgcheckdir.c code I noticed that the function comment
was outdated. The code now can return values from 0 to 4 while the
comment explains only values 0,1,2.
Patch attached.
Regards,
Marco
--
Marco Nenciarini - 2ndQuadrant Italy
PostgreSQL Training, Services and Support
marco.nenciarini@2ndQuadrant.it | www.2ndQuadrant.it
Attachments:
pg_check_dir.patchtext/plain; charset=UTF-8; name=pg_check_dir.patch; x-mac-creator=0; x-mac-type=0Download
From 44623f67a124c4c77f7ff8097f13e116d20d83a5 Mon Sep 17 00:00:00 2001
From: Marco Nenciarini <marco.nenciarini@2ndQuadrant.it>
Date: Thu, 29 Jan 2015 16:45:27 +0100
Subject: [PATCH] Update pg_check_dir comment
---
src/port/pgcheckdir.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/src/port/pgcheckdir.c b/src/port/pgcheckdir.c
index 7061893..9165ebb 100644
*** a/src/port/pgcheckdir.c
--- b/src/port/pgcheckdir.c
***************
*** 22,28 ****
* Returns:
* 0 if nonexistent
* 1 if exists and empty
! * 2 if exists and not empty
* -1 if trouble accessing directory (errno reflects the error)
*/
int
--- 22,30 ----
* Returns:
* 0 if nonexistent
* 1 if exists and empty
! * 2 if exists and contains _only_ dot files
! * 3 if exists and contains a mount point
! * 4 if exists and not empty
* -1 if trouble accessing directory (errno reflects the error)
*/
int
*************** pg_check_dir(const char *dir)
*** 32,37 ****
--- 34,40 ----
DIR *chkdir;
struct dirent *file;
bool dot_found = false;
+ bool mount_found = false;
chkdir = opendir(dir);
if (chkdir == NULL)
*************** pg_check_dir(const char *dir)
*** 51,60 ****
{
dot_found = true;
}
else if (strcmp("lost+found", file->d_name) == 0)
{
! result = 3; /* not empty, mount point */
! break;
}
#endif
else
--- 54,63 ----
{
dot_found = true;
}
+ /* lost+found directory */
else if (strcmp("lost+found", file->d_name) == 0)
{
! mount_found = true;
}
#endif
else
*************** pg_check_dir(const char *dir)
*** 67,72 ****
--- 70,79 ----
if (errno || closedir(chkdir))
result = -1; /* some kind of I/O error? */
+ /* We report on mount point if we find a lost+found directory */
+ if (result == 1 && mount_found)
+ result = 3;
+
/* We report on dot-files if we _only_ find dot files */
if (result == 1 && dot_found)
result = 2;
--
2.2.2
On Thu, Jan 29, 2015 at 11:00 AM, Marco Nenciarini
<marco.nenciarini@2ndquadrant.it> wrote:
reading pgcheckdir.c code I noticed that the function comment
was outdated. The code now can return values from 0 to 4 while the
comment explains only values 0,1,2.
This is not just a comment fix; you are clearly changing the behavior
of the function in some way.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Robert Haas <robertmhaas@gmail.com> writes:
On Thu, Jan 29, 2015 at 11:00 AM, Marco Nenciarini
<marco.nenciarini@2ndquadrant.it> wrote:reading pgcheckdir.c code I noticed that the function comment
was outdated. The code now can return values from 0 to 4 while the
comment explains only values 0,1,2.
This is not just a comment fix; you are clearly changing the behavior
of the function in some way.
I think he's trying to fix a bug in terms of slipshod definition of the
non-empty-directory subcases, but it would be nice to have some
clarity about that.
There is at least one other bug in that function now that I look at it:
in event of a readdir() failure, it neglects to execute closedir().
Perhaps not too significant since all existing callers will just exit()
anyway after a failure, but still ...
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Il 29/01/15 18:37, Robert Haas ha scritto:
On Thu, Jan 29, 2015 at 11:00 AM, Marco Nenciarini
<marco.nenciarini@2ndquadrant.it> wrote:reading pgcheckdir.c code I noticed that the function comment
was outdated. The code now can return values from 0 to 4 while the
comment explains only values 0,1,2.This is not just a comment fix; you are clearly changing the behavior
of the function in some way.
The previous version was returning 3 (mount point) even if the dir
contains something after the lost+found directory. I think this case
deserves a 4 output. For example:
lost+found
zzz.txt
give the result 3.
If the directory contains
aaa.txt
lost+found
the result is instead 4.
This doesn't make much difference, as 3 and 4 are both error condition
for all the callers, but the current behavior looks odd to me, and
surely is not what one can expect reading the comments.
My version returns 3 only if the lost+found file is alone in the
directory (eventually ignoring dot files along it, as it was doing before)
Regards,
Marco
--
Marco Nenciarini - 2ndQuadrant Italy
PostgreSQL Training, Services and Support
marco.nenciarini@2ndQuadrant.it | www.2ndQuadrant.it
On Fri, Jan 30, 2015 at 2:49 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
There is at least one other bug in that function now that I look at it:
in event of a readdir() failure, it neglects to execute closedir().
Perhaps not too significant since all existing callers will just exit()
anyway after a failure, but still ...
I would imagine that code scanners like coverity or similar would not
be happy about that. ISTM that it is better to closedir()
appropriately in all the code paths.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Il 30/01/15 03:54, Michael Paquier ha scritto:
On Fri, Jan 30, 2015 at 2:49 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
There is at least one other bug in that function now that I look at it:
in event of a readdir() failure, it neglects to execute closedir().
Perhaps not too significant since all existing callers will just exit()
anyway after a failure, but still ...I would imagine that code scanners like coverity or similar would not
be happy about that. ISTM that it is better to closedir()
appropriately in all the code paths.
I've attached a new version of the patch fixing the missing closedir on
readdir error.
Regards,
Marco
--
Marco Nenciarini - 2ndQuadrant Italy
PostgreSQL Training, Services and Support
marco.nenciarini@2ndQuadrant.it | www.2ndQuadrant.it
Attachments:
pg_check_dir-comments-and-code.patchtext/plain; charset=UTF-8; name=pg_check_dir-comments-and-code.patch; x-mac-creator=0; x-mac-type=0Download
From d2bfb6878aed404fdba1447d3f813edb4442ff47 Mon Sep 17 00:00:00 2001
From: Marco Nenciarini <marco.nenciarini@2ndQuadrant.it>
Date: Sat, 31 Jan 2015 14:06:54 +0100
Subject: [PATCH] Improve pg_check_dir comments and code
Update the pg_check_dir comment to explain all the possible return
values (0 to 4).
Fix the beaviour in presence of lost+found directory. The previous
version was returning 3 (mount point) even if the readdir returns
something after the lost+found directory. In this case the output should
be 4 (not empty).
Ensure that closedir are always called even if readdir returns an error.
---
src/port/pgcheckdir.c | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/src/port/pgcheckdir.c b/src/port/pgcheckdir.c
index 7061893..02a048c 100644
*** a/src/port/pgcheckdir.c
--- b/src/port/pgcheckdir.c
***************
*** 22,28 ****
* Returns:
* 0 if nonexistent
* 1 if exists and empty
! * 2 if exists and not empty
* -1 if trouble accessing directory (errno reflects the error)
*/
int
--- 22,30 ----
* Returns:
* 0 if nonexistent
* 1 if exists and empty
! * 2 if exists and contains _only_ dot files
! * 3 if exists and contains a mount point
! * 4 if exists and not empty
* -1 if trouble accessing directory (errno reflects the error)
*/
int
*************** pg_check_dir(const char *dir)
*** 32,37 ****
--- 34,40 ----
DIR *chkdir;
struct dirent *file;
bool dot_found = false;
+ bool mount_found = false;
chkdir = opendir(dir);
if (chkdir == NULL)
*************** pg_check_dir(const char *dir)
*** 51,60 ****
{
dot_found = true;
}
else if (strcmp("lost+found", file->d_name) == 0)
{
! result = 3; /* not empty, mount point */
! break;
}
#endif
else
--- 54,63 ----
{
dot_found = true;
}
+ /* lost+found directory */
else if (strcmp("lost+found", file->d_name) == 0)
{
! mount_found = true;
}
#endif
else
*************** pg_check_dir(const char *dir)
*** 64,72 ****
}
}
! if (errno || closedir(chkdir))
result = -1; /* some kind of I/O error? */
/* We report on dot-files if we _only_ find dot files */
if (result == 1 && dot_found)
result = 2;
--- 67,82 ----
}
}
! if (errno)
result = -1; /* some kind of I/O error? */
+ if (closedir(chkdir))
+ result = -1; /* error executing closedir */
+
+ /* We report on mount point if we find a lost+found directory */
+ if (result == 1 && mount_found)
+ result = 3;
+
/* We report on dot-files if we _only_ find dot files */
if (result == 1 && dot_found)
result = 2;
--
2.2.2
On Sat, Jan 31, 2015 at 8:28 AM, Marco Nenciarini
<marco.nenciarini@2ndquadrant.it> wrote:
Il 30/01/15 03:54, Michael Paquier ha scritto:
On Fri, Jan 30, 2015 at 2:49 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
There is at least one other bug in that function now that I look at it:
in event of a readdir() failure, it neglects to execute closedir().
Perhaps not too significant since all existing callers will just exit()
anyway after a failure, but still ...I would imagine that code scanners like coverity or similar would not
be happy about that. ISTM that it is better to closedir()
appropriately in all the code paths.I've attached a new version of the patch fixing the missing closedir on
readdir error.
If readir() fails and closedir() succeeds, the return will be -1 but
errno will be 0.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Il 02/02/15 21:48, Robert Haas ha scritto:
On Sat, Jan 31, 2015 at 8:28 AM, Marco Nenciarini
<marco.nenciarini@2ndquadrant.it> wrote:Il 30/01/15 03:54, Michael Paquier ha scritto:
On Fri, Jan 30, 2015 at 2:49 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
There is at least one other bug in that function now that I look at it:
in event of a readdir() failure, it neglects to execute closedir().
Perhaps not too significant since all existing callers will just exit()
anyway after a failure, but still ...I would imagine that code scanners like coverity or similar would not
be happy about that. ISTM that it is better to closedir()
appropriately in all the code paths.I've attached a new version of the patch fixing the missing closedir on
readdir error.If readir() fails and closedir() succeeds, the return will be -1 but
errno will be 0.
The attached patch should fix it.
Regards,
Marco
--
Marco Nenciarini - 2ndQuadrant Italy
PostgreSQL Training, Services and Support
marco.nenciarini@2ndQuadrant.it | www.2ndQuadrant.it
Attachments:
pg_check_dir-v2.patchtext/plain; charset=UTF-8; name=pg_check_dir-v2.patch; x-mac-creator=0; x-mac-type=0Download
diff --git a/src/port/pgcheckdir.c b/src/port/pgcheckdir.c
index 7061893..7102f2c 100644
*** a/src/port/pgcheckdir.c
--- b/src/port/pgcheckdir.c
***************
*** 22,28 ****
* Returns:
* 0 if nonexistent
* 1 if exists and empty
! * 2 if exists and not empty
* -1 if trouble accessing directory (errno reflects the error)
*/
int
--- 22,30 ----
* Returns:
* 0 if nonexistent
* 1 if exists and empty
! * 2 if exists and contains _only_ dot files
! * 3 if exists and contains a mount point
! * 4 if exists and not empty
* -1 if trouble accessing directory (errno reflects the error)
*/
int
*************** pg_check_dir(const char *dir)
*** 32,37 ****
--- 34,41 ----
DIR *chkdir;
struct dirent *file;
bool dot_found = false;
+ bool mount_found = false;
+ int readdir_errno;
chkdir = opendir(dir);
if (chkdir == NULL)
*************** pg_check_dir(const char *dir)
*** 51,60 ****
{
dot_found = true;
}
else if (strcmp("lost+found", file->d_name) == 0)
{
! result = 3; /* not empty, mount point */
! break;
}
#endif
else
--- 55,64 ----
{
dot_found = true;
}
+ /* lost+found directory */
else if (strcmp("lost+found", file->d_name) == 0)
{
! mount_found = true;
}
#endif
else
*************** pg_check_dir(const char *dir)
*** 64,72 ****
}
}
! if (errno || closedir(chkdir))
result = -1; /* some kind of I/O error? */
/* We report on dot-files if we _only_ find dot files */
if (result == 1 && dot_found)
result = 2;
--- 68,87 ----
}
}
! if (errno)
result = -1; /* some kind of I/O error? */
+ /* Close chkdir and avoid overwriting the readdir errno on success */
+ readdir_errno = errno;
+ if (closedir(chkdir))
+ result = -1; /* error executing closedir */
+ else
+ errno = readdir_errno;
+
+ /* We report on mount point if we find a lost+found directory */
+ if (result == 1 && mount_found)
+ result = 3;
+
/* We report on dot-files if we _only_ find dot files */
if (result == 1 && dot_found)
result = 2;
On Thu, Feb 12, 2015 at 9:31 AM, Marco Nenciarini
<marco.nenciarini@2ndquadrant.it> wrote:
Il 02/02/15 21:48, Robert Haas ha scritto:
On Sat, Jan 31, 2015 at 8:28 AM, Marco Nenciarini
<marco.nenciarini@2ndquadrant.it> wrote:Il 30/01/15 03:54, Michael Paquier ha scritto:
On Fri, Jan 30, 2015 at 2:49 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
There is at least one other bug in that function now that I look at it:
in event of a readdir() failure, it neglects to execute closedir().
Perhaps not too significant since all existing callers will just exit()
anyway after a failure, but still ...I would imagine that code scanners like coverity or similar would not
be happy about that. ISTM that it is better to closedir()
appropriately in all the code paths.I've attached a new version of the patch fixing the missing closedir on
readdir error.If readir() fails and closedir() succeeds, the return will be -1 but
errno will be 0.The attached patch should fix it.
Looks nice. Committed.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Feb 02, 2015 at 03:48:33PM -0500, Robert Haas wrote:
On Sat, Jan 31, 2015 at 8:28 AM, Marco Nenciarini <marco.nenciarini@2ndquadrant.it> wrote:
I've attached a new version of the patch fixing the missing closedir on
readdir error.If readir() fails and closedir() succeeds, the return will be -1 but
errno will be 0.
Out of curiosity, have you seen a closedir() implementation behave that way?
It would violate C99 ("The value of errno is zero at program startup, but is
never set to zero by any library function.") and POSIX.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Feb 20, 2015 at 12:59 AM, Noah Misch <noah@leadboat.com> wrote:
On Mon, Feb 02, 2015 at 03:48:33PM -0500, Robert Haas wrote:
On Sat, Jan 31, 2015 at 8:28 AM, Marco Nenciarini <marco.nenciarini@2ndquadrant.it> wrote:
I've attached a new version of the patch fixing the missing closedir on
readdir error.If readir() fails and closedir() succeeds, the return will be -1 but
errno will be 0.Out of curiosity, have you seen a closedir() implementation behave that way?
It would violate C99 ("The value of errno is zero at program startup, but is
never set to zero by any library function.") and POSIX.
No. Good point, I didn't think about that. I think this way is safer, though.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Robert Haas <robertmhaas@gmail.com> writes:
On Fri, Feb 20, 2015 at 12:59 AM, Noah Misch <noah@leadboat.com> wrote:
On Mon, Feb 02, 2015 at 03:48:33PM -0500, Robert Haas wrote:
If readir() fails and closedir() succeeds, the return will be -1 but
errno will be 0.
Out of curiosity, have you seen a closedir() implementation behave that way?
It would violate C99 ("The value of errno is zero at program startup, but is
never set to zero by any library function.") and POSIX.
No. Good point, I didn't think about that. I think this way is safer, though.
While the spec forbids library functions from setting errno to zero, there
is no restriction on them changing errno in other ways despite returning
success; their exit-time value of errno is only well-defined if they fail.
So we do need to preserve errno explicitly across closedir(), or we may
report the wrong failure from readdir().
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sun, Feb 22, 2015 at 07:57:41PM -0500, Tom Lane wrote:
Robert Haas <robertmhaas@gmail.com> writes:
On Fri, Feb 20, 2015 at 12:59 AM, Noah Misch <noah@leadboat.com> wrote:
On Mon, Feb 02, 2015 at 03:48:33PM -0500, Robert Haas wrote:
If readir() fails and closedir() succeeds, the return will be -1 but
errno will be 0.Out of curiosity, have you seen a closedir() implementation behave that way?
It would violate C99 ("The value of errno is zero at program startup, but is
never set to zero by any library function.") and POSIX.No. Good point, I didn't think about that. I think this way is safer, though.
While the spec forbids library functions from setting errno to zero, there
is no restriction on them changing errno in other ways despite returning
success; their exit-time value of errno is only well-defined if they fail.
So we do need to preserve errno explicitly across closedir(), or we may
report the wrong failure from readdir().
Yes. I'm happy with the commit itself.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers