Re: [HACKERS] pg_check_dir comments and implementation mismatch
On Sun, Feb 22, 2015 at 07:57:41PM -0500, Tom Lane wrote:
> Robert Haas writes:
> > On Fri, Feb 20, 2015 at 12:59 AM, Noah Misch 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
Re: [HACKERS] pg_check_dir comments and implementation mismatch
Robert Haas writes:
> On Fri, Feb 20, 2015 at 12:59 AM, Noah Misch 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
Re: [HACKERS] pg_check_dir comments and implementation mismatch
On Fri, Feb 20, 2015 at 12:59 AM, Noah Misch 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
>> 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
Re: [HACKERS] pg_check_dir comments and implementation mismatch
On Mon, Feb 02, 2015 at 03:48:33PM -0500, Robert Haas wrote:
> On Sat, Jan 31, 2015 at 8:28 AM, Marco Nenciarini
> 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
Re: [HACKERS] pg_check_dir comments and implementation mismatch
On Thu, Feb 12, 2015 at 9:31 AM, Marco Nenciarini wrote: > Il 02/02/15 21:48, Robert Haas ha scritto: >> On Sat, Jan 31, 2015 at 8:28 AM, Marco Nenciarini >> wrote: >>> Il 30/01/15 03:54, Michael Paquier ha scritto: On Fri, Jan 30, 2015 at 2:49 AM, Tom Lane 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
Re: [HACKERS] pg_check_dir comments and implementation mismatch
Il 02/02/15 21:48, Robert Haas ha scritto:
> On Sat, Jan 31, 2015 at 8:28 AM, Marco Nenciarini
> wrote:
>> Il 30/01/15 03:54, Michael Paquier ha scritto:
>>> On Fri, Jan 30, 2015 at 2:49 AM, Tom Lane 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.nenciar...@2ndquadrant.it | www.2ndQuadrant.it
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;
booldot_found = false;
+ boolmount_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;
signature.asc
Description: OpenPGP digital signature
Re: [HACKERS] pg_check_dir comments and implementation mismatch
On Sat, Jan 31, 2015 at 8:28 AM, Marco Nenciarini wrote: > Il 30/01/15 03:54, Michael Paquier ha scritto: >> On Fri, Jan 30, 2015 at 2:49 AM, Tom Lane 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
Re: [HACKERS] pg_check_dir comments and implementation mismatch
Il 30/01/15 03:54, Michael Paquier ha scritto:
> On Fri, Jan 30, 2015 at 2:49 AM, Tom Lane 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.nenciar...@2ndquadrant.it | www.2ndQuadrant.it
From d2bfb6878aed404fdba1447d3f813edb4442ff47 Mon Sep 17 00:00:00 2001
From: Marco Nenciarini
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;
booldot_found = false;
+ boolmount_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
signature.asc
Description: OpenPGP digital signature
Re: [HACKERS] pg_check_dir comments and implementation mismatch
On Fri, Jan 30, 2015 at 2:49 AM, Tom Lane 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
Re: [HACKERS] pg_check_dir comments and implementation mismatch
Il 29/01/15 18:37, Robert Haas ha scritto: > On Thu, Jan 29, 2015 at 11:00 AM, Marco Nenciarini > 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.nenciar...@2ndquadrant.it | www.2ndQuadrant.it signature.asc Description: OpenPGP digital signature
Re: [HACKERS] pg_check_dir comments and implementation mismatch
Robert Haas writes: > On Thu, Jan 29, 2015 at 11:00 AM, Marco Nenciarini > 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
Re: [HACKERS] pg_check_dir comments and implementation mismatch
On Thu, Jan 29, 2015 at 11:00 AM, Marco Nenciarini 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
[HACKERS] 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.nenciar...@2ndquadrant.it | www.2ndQuadrant.it
From 44623f67a124c4c77f7ff8097f13e116d20d83a5 Mon Sep 17 00:00:00 2001
From: Marco Nenciarini
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;
booldot_found = false;
+ boolmount_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
signature.asc
Description: OpenPGP digital signature
