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

[Pranith Kumar Karampuri]

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 at gluster.org
> http://supercolony.gluster.org/mailman/listinfo/gluster-devel