Обсуждение: 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
Вложения
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
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
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
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
Вложения
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
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
Вложения
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
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.
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
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
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.