On Wed, 2009-06-03 at 16:13 -0400, Jeff Layton wrote:
> On Wed, 03 Jun 2009 16:03:41 -0400
> Trond Myklebust <trond.myklebust@xxxxxxxxxx> wrote:
>
> > On Wed, 2009-06-03 at 15:44 -0400, Jeff Layton wrote:
> > > The closeall function is broken in such a way that it almost never
> > > closes any file descriptors. It's calling strtol on the text
> > > representation of the file descriptor, and then checking to see if the
> > > value of *endptr is not '\0' before trying to close the file. This check
> > > is wrong.
> > >
> > > When strtol returns an endptr that points to a NULL byte, that indicates
> > > that the conversion was completely successful. I believe this check
> > > should instead be requiring that endptr is pointing to '\0' before closing
> > > the fd.
> > >
> > > Also, fix up the function to check for conversion errors from strtol. If
> > > one occurs, just skip the close on that entry.
> > >
> > > Reported-by: Chuck Lever <chuck.lever@xxxxxxxxxx>
> > > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > > ---
> > > support/nfs/closeall.c | 4 +++-
> > > 1 files changed, 3 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/support/nfs/closeall.c b/support/nfs/closeall.c
> > > index cc7fb3b..5fe07de 100644
> > > --- a/support/nfs/closeall.c
> > > +++ b/support/nfs/closeall.c
> > > @@ -7,6 +7,7 @@
> > > #include <unistd.h>
> > > #include <stdlib.h>
> > > #include <dirent.h>
> > > +#include <errno.h>
> > >
> > > void
> > > closeall(int min)
> > > @@ -18,8 +19,9 @@ closeall(int min)
> > >
> > > while ((d = readdir(dir)) != NULL) {
> > > char *endp;
> > > + errno = 0;
> > > long n = strtol(d->d_name, &endp, 10);
> > > - if (*endp != '\0' && n >= min && n != dfd)
> > > + if (!errno && *endp == '\0' && n >= min && n != dfd)
> > > (void) close(n);
> > > }
> > > closedir(dir);
> >
> > Shouldn't you also check the case endp == d->d_name? I don't think that
> > actually sets errno...
> >
>
> Hmm...for that to be a problem, readdir would have to return a name of
> "" (i.e. d->d_name would have to point to a NULL byte). Is that a
> possibility here? If not, then the *endp == '\0' check should catch
> that case too...
In theory, d->d_name cannot be an empty string.
Then again, if you believe theory, it shouldn't be necessary to check
errno at all since (with exception of '.' and '..', which are caught by
the *endp=='\0' test) the readdir entries in /proc/self/fd should always
convert to positive integer values...
Trond
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
