Re: Invalid DIR * usage in quota xlator

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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




[Index of Archives]     [Gluster Users]     [Ceph Users]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux