Re: Invalid DIR * usage in quota xlator

[Pranith Kumar Karampuri <pkarampu@xxxxxxxxxx>]

On 10/13/2014 02:37 PM, Pranith Kumar Karampuri wrote:
> On 10/13/2014 02:27 PM, Emmanuel Dreyfus wrote:
> > On Mon, Oct 13, 2014 at 01:42:38PM +0530, Pranith Kumar Karampuri wrote:
> > > > No bug here, just suboptimal behavior, both in glusterfs and NetBSD 
> > > > FUSE.
> > > oh!, but shouldn't it get op_ret = 0 instead of op_ret -1, op_errno 
> > > EINVAL?
> > It happens because of thei change I introduced:
> >                  seekdir (dir, (long)off);
> > #ifndef GF_LINUX_HOST_OS
> >                  if (telldir(dir) != off) {
> >                          gf_log (THIS->name, GF_LOG_ERROR,
> >                                  "seekdir(%ld) failed on dir=%p: "
> >                                  "Invalid argument (offset reused from "
> >                                  "another DIR * structure?)", 
> > (long)off, dir);
> >                          errno = EINVAL;
> >                          count = -1;
> >                          goto out;
> >                  }
> > #endif /* GF_LINUX_HOST_OS */
> >
> > The idea is to catch wrong opendir/closedir usage, and the errno = 
> > EINVAL
> > is critical in such a situation to avoid an infinite loop. On the other
> > hand it also fires for last entry. I did not notice it before because
> > this EINVAL will not reach calling process, but it may cause trouble
> > for internal glusterfs usage.
> >
> > I see an easy way yo fix it (patch below)
> > - when posix xlator detects EOF, return an offset of -1 in last entry.
> > - When called with an offset of -1, immediatly return op_ret = 0 with 
> > no entry
> >
> > What do you think?
> I am not aware of backend filesystems that much, may be someone with 
> that knowledge can comment here, what happens when new entries are 
> created in the directory after this readdir is responded with '-1'?
>
> > diff --git a/xlators/storage/posix/src/posix.c 
> > b/xlators/storage/posix/src/posix
> > index 6b12d39..3e539bb 100644
> > --- a/xlators/storage/posix/src/posix.c
> > +++ b/xlators/storage/posix/src/posix.c
> > @@ -79,6 +79,10 @@ extern char *marker_xattrs[];
> >   #define SET_TO_OLD_FS_ID()
> >     #endif
> > +
> > +/* offset < sizeof(struct dirent) cannot be valid */
> > +#define INVALID_DIRENT_OFFSET sizeof (void *)
> > +
> >   int
> >   posix_forget (xlator_t *this, inode_t *inode)
> >   {
> > @@ -4851,6 +4855,10 @@ posix_fill_readdir (fd_t *fd, DIR *dir, off_t 
> > off, size_t
> >           int                   len = 0;
> >           int                   ret = 0;
> >   +        /* EOF was previously detected */
> > +        if (off == INVALID_DIRENT_OFFSET)
> > +               goto out;
> > +
> >           if (skip_dirs) {
> >                   len = posix_handle_path (this, fd->inode->gfid, 
> > NULL, NULL, 0);
> >                   hpath = alloca (len + 256); /* NAME_MAX */
> > @@ -4969,9 +4977,12 @@ posix_fill_readdir (fd_t *fd, DIR *dir, off_t 
> > off, size_t
> >                   count ++;
> >           }
> >   -        if ((!readdir (dir) && (errno == 0)))
> > +        if ((!readdir (dir) && (errno == 0))) {
> >                   /* Indicate EOF */
> >                   errno = ENOENT;
> Now I understand why you kept saying ENOENT ;-). This may not be a 
> good idea IMO. Code all through gluster doesn't care what errno it 
> unwinds upwards.
In case op_ret is zero I mean :-)

Pranith
> > +                /* Set invalid offset for later detection */
> > +                this_entry->d_off = INVALID_DIRENT_OFFSET;
> > +        }
> >   out:
> >           return count;
> >   }
>
> _______________________________________________
> Gluster-devel mailing list
> Gluster-devel@xxxxxxxxxxx
> http://supercolony.gluster.org/mailman/listinfo/gluster-devel

_______________________________________________
Gluster-devel mailing list
Gluster-devel@xxxxxxxxxxx
http://supercolony.gluster.org/mailman/listinfo/gluster-devel