[Gluster-devel] Invalid DIR * usage in quota xlator

[Pranith Kumar Karampuri]

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.
> +                /* Set invalid offset for later detection */
> +                this_entry->d_off = INVALID_DIRENT_OFFSET;
> +        }
>   out:
>           return count;
>   }
>
>