Re: [HACKERS] pg_check_dir comments and implementation mismatch

2015-02-23 Thread Noah Misch 
On Sun, Feb 22, 2015 at 07:57:41PM -0500, Tom Lane wrote:
 Robert Haas robertmh...@gmail.com writes:
  On Fri, Feb 20, 2015 at 12:59 AM, Noah Misch n...@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

Re: [HACKERS] pg_check_dir comments and implementation mismatch

2015-02-22 Thread Robert Haas 
On Fri, Feb 20, 2015 at 12:59 AM, Noah Misch n...@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.nenciar...@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

Re: [HACKERS] pg_check_dir comments and implementation mismatch

2015-02-22 Thread Tom Lane 
Robert Haas robertmh...@gmail.com writes:
 On Fri, Feb 20, 2015 at 12:59 AM, Noah Misch n...@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

Re: [HACKERS] pg_check_dir comments and implementation mismatch

2015-02-19 Thread Noah Misch 
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.nenciar...@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

Re: [HACKERS] pg_check_dir comments and implementation mismatch

2015-02-17 Thread Robert Haas 
On Thu, Feb 12, 2015 at 9:31 AM, Marco Nenciarini
marco.nenciar...@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.nenciar...@2ndquadrant.it wrote:
 Il 30/01/15 03:54, Michael Paquier ha scritto:
 On Fri, Jan 30, 2015 at 2:49 AM, Tom Lane t...@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

Re: [HACKERS] pg_check_dir comments and implementation mismatch

2015-02-12 Thread Marco Nenciarini 
Il 02/02/15 21:48, Robert Haas ha scritto:
 On Sat, Jan 31, 2015 at 8:28 AM, Marco Nenciarini
 marco.nenciar...@2ndquadrant.it wrote:
 Il 30/01/15 03:54, Michael Paquier ha scritto:
 On Fri, Jan 30, 2015 at 2:49 AM, Tom Lane t...@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.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

2015-02-02 Thread Robert Haas 
On Sat, Jan 31, 2015 at 8:28 AM, Marco Nenciarini
marco.nenciar...@2ndquadrant.it wrote:
 Il 30/01/15 03:54, Michael Paquier ha scritto:
 On Fri, Jan 30, 2015 at 2:49 AM, Tom Lane t...@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

Re: [HACKERS] pg_check_dir comments and implementation mismatch

2015-01-31 Thread Marco Nenciarini 
Il 30/01/15 03:54, Michael Paquier ha scritto:
 On Fri, Jan 30, 2015 at 2:49 AM, Tom Lane t...@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.nenciar...@2ndquadrant.it | www.2ndQuadrant.it
From d2bfb6878aed404fdba1447d3f813edb4442ff47 Mon Sep 17 00:00:00 2001
From: Marco Nenciarini marco.nenciar...@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;
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

2015-01-29 Thread Marco Nenciarini 
Il 29/01/15 18:37, Robert Haas ha scritto:
 On Thu, Jan 29, 2015 at 11:00 AM, Marco Nenciarini
 marco.nenciar...@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.nenciar...@2ndquadrant.it | www.2ndQuadrant.it



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] pg_check_dir comments and implementation mismatch

2015-01-29 Thread Robert Haas 
On Thu, Jan 29, 2015 at 11:00 AM, Marco Nenciarini
marco.nenciar...@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

Re: [HACKERS] pg_check_dir comments and implementation mismatch

2015-01-29 Thread Tom Lane 
Robert Haas robertmh...@gmail.com writes:
 On Thu, Jan 29, 2015 at 11:00 AM, Marco Nenciarini
 marco.nenciar...@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

[HACKERS] pg_check_dir comments and implementation mismatch

2015-01-29 Thread Marco Nenciarini 
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 marco.nenciar...@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;
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

Re: [HACKERS] pg_check_dir comments and implementation mismatch

2015-01-29 Thread Michael Paquier 
On Fri, Jan 30, 2015 at 2:49 AM, Tom Lane t...@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