On Mon, Oct 13, 2014 at 08:57:11AM +0000, 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
You're assuming no filesystem uses -1 as a cookie?
Might be true, but it's taking a small chance.
--b.
>
> What do you think?
> 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;
> + /* Set invalid offset for later detection */
> + this_entry->d_off = INVALID_DIRENT_OFFSET;
> + }
> out:
> return count;
> }
>
>
> --
> Emmanuel Dreyfus
> manu@xxxxxxxxxx
> _______________________________________________
> 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
