summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorShehjar Tikoo <shehjart@zresearch.com>2009-05-11 18:22:29 +0530
committerAnand V. Avati <avati@amp.gluster.com>2009-05-18 19:12:37 +0530
commitaeda0a31977e8da295b913e6a306ff01ccf0ce0a (patch)
tree465b49ea3305583e06bf3ad24d49f338f7970118
parent4f1a87a245b960f1cd1fb4fe40b4254ac3793213 (diff)
libglusterfsclient: Revert and re-do readdir conformance
This commit basically reverts the previous readdir conformance patch I sent a few days back. That commit had a completely retarded and broken way of maintaining per-directory dirent. It was broken for two reasons: 1. Creating a wrapper structure around the directory's fd_t only for storing a struct dirent is not clean enough. This commit takes a better approach by storing the dirent in fd_t context. This dirent is valid only if the fd_t refers to a directory. 2. That commit was made and tested under the assumption (..stupidity is a better word..) that only opendir call is used for opening a directory. That is not correct. Directories are also opened using the open syscall. The point is, glusterfs_open returns an fd_t and so did glusterfs_opendir. The previous patch actually changed opendir to return a new wrapper structure. That is fine, if we go by the POSIX definition of open and opendir because, they're both supposed to return different types, an int and a DIR*. However, in libglusterfsclient, all other code assumes that directory handles corresponding to DIR* and file descriptors corresponding to int types are the same type, resulting in use of the same locking and fd context addition/extraction code. So a directory opened using opendir returned a wrapper structure which went down into the libglusterfsclient stack where some function called a lock on the handle assuming it was an fd_t, since it is not and dereferencing of the supposed fd->inode->lock results in a seg fault. Obviously, this didnt show up till unfs3 used open() to open a directory and not opendir. Signed-off-by: Anand V. Avati <avati@amp.gluster.com>
-rwxr-xr-xlibglusterfsclient/src/libglusterfsclient-internals.h17
-rwxr-xr-xlibglusterfsclient/src/libglusterfsclient.c73
2 files changed, 29 insertions, 61 deletions
diff --git a/libglusterfsclient/src/libglusterfsclient-internals.h b/libglusterfsclient/src/libglusterfsclient-internals.h
index 4cdde26e7..40ad6f692 100755
--- a/libglusterfsclient/src/libglusterfsclient-internals.h
+++ b/libglusterfsclient/src/libglusterfsclient-internals.h
@@ -83,6 +83,11 @@ typedef struct {
pthread_mutex_t lock;
off_t offset;
libglusterfs_client_ctx_t *ctx;
+ /* `man readdir` says readdir is non-re-entrant
+ * only if two readdirs are racing on the same
+ * handle.
+ */
+ struct dirent dirp;
} libglusterfs_client_fd_ctx_t;
typedef struct libglusterfs_client_async_local {
@@ -231,16 +236,4 @@ struct vmp_entry {
};
-/* Internal directory handle inited in opendir.
- * This is needed in order to store a per-handle
- * dirent structure.
- */
-struct libgf_dir_handle {
- fd_t *dirfd;
- struct dirent dirp;
-};
-
-
-
-
#endif
diff --git a/libglusterfsclient/src/libglusterfsclient.c b/libglusterfsclient/src/libglusterfsclient.c
index f16dcb4d1..0cfc23e9e 100755
--- a/libglusterfsclient/src/libglusterfsclient.c
+++ b/libglusterfsclient/src/libglusterfsclient.c
@@ -2120,7 +2120,6 @@ glusterfs_glh_open (glusterfs_handle_t handle, const char *path, int flags,...)
mode_t mode = 0;
va_list ap;
char *pathres = NULL;
- glusterfs_file_t *fh = NULL;
GF_VALIDATE_OR_GOTO (LIBGF_XL_NAME, ctx, out);
GF_VALIDATE_ABSOLUTE_PATH_OR_GOTO (LIBGF_XL_NAME, path, out);
@@ -2191,22 +2190,13 @@ glusterfs_glh_open (glusterfs_handle_t handle, const char *path, int flags,...)
va_end (ap);
op_ret = libgf_client_creat (ctx, &loc, fd, flags, mode);
} else {
- if (S_ISDIR (loc.inode->st_mode)) {
- /* This could be optimized a bit by finding
- * the common functionality between
- * glusterfs_glh_open and
- * glusterfs_glh_opendir.
- */
- fh = glusterfs_glh_opendir (ctx, path);
- goto out;
- } else
+ if (S_ISDIR (loc.inode->st_mode))
+ op_ret = libgf_client_opendir (ctx, &loc, fd);
+ else
op_ret = libgf_client_open (ctx, &loc, fd, flags);
}
op_over:
- /* For a directory all this is done inside
- * glusterfs_glh_opendir.
- */
if (op_ret == -1) {
fd_unref (fd);
fd = NULL;
@@ -2229,12 +2219,6 @@ op_over:
}
}
- /* This assignment will happen if the pathname to be opened is
- * a file, otherwise, if it is a directory, we'll jump to out:
- * right after the call to glusterfs_glh_opendir above.
- */
- fh = (glusterfs_file_t)fd;
-
out:
libgf_client_loc_wipe (&loc);
@@ -2245,7 +2229,7 @@ out:
if (pathres)
FREE (pathres);
- return fh;
+ return fd;
}
glusterfs_file_t
@@ -3375,9 +3359,8 @@ glusterfs_readdir (glusterfs_dir_t dirfd)
off_t offset = 0;
libglusterfs_client_fd_ctx_t *fd_ctx = NULL;
struct dirent *dirp = NULL;
- struct libgf_dir_handle *dir = (struct libgf_dir_handle *)dirfd;
- fd_ctx = libgf_get_fd_ctx (dir->dirfd);
+ fd_ctx = libgf_get_fd_ctx (dirfd);
if (!fd_ctx) {
errno = EBADF;
goto out;
@@ -3387,12 +3370,12 @@ glusterfs_readdir (glusterfs_dir_t dirfd)
{
ctx = fd_ctx->ctx;
offset = fd_ctx->offset;
+ dirp = &fd_ctx->dirp;
}
pthread_mutex_unlock (&fd_ctx->lock);
- dirp = &dir->dirp;
memset (dirp, 0, sizeof (struct dirent));
- op_ret = libgf_client_readdir (ctx, dir->dirfd, dirp,
+ op_ret = libgf_client_readdir (ctx, (fd_t *)dirfd, dirp,
LIBGF_READDIR_BLOCK, &offset, 1);
if (op_ret <= 0) {
@@ -4611,7 +4594,6 @@ glusterfs_glh_opendir (glusterfs_handle_t handle, const char *path)
loc_t loc = {0, };
fd_t *dirfd = NULL;
char *name = NULL;
- struct libgf_dir_handle *dir = NULL;
GF_VALIDATE_OR_GOTO (LIBGF_XL_NAME, ctx, out);
GF_VALIDATE_ABSOLUTE_PATH_OR_GOTO (LIBGF_XL_NAME, path, out);
@@ -4638,40 +4620,34 @@ glusterfs_glh_opendir (glusterfs_handle_t handle, const char *path)
goto out;
}
- dir = CALLOC (1, sizeof (struct libgf_dir_handle));
- if (!dir) {
- op_ret = -1;
- goto out;
- }
-
- dir->dirfd = fd_create (loc.inode, 0);
- op_ret = libgf_client_opendir (ctx, &loc, dir->dirfd);
-
- if (op_ret == -1)
- goto out;
+ dirfd = fd_create (loc.inode, 0);
+ op_ret = libgf_client_opendir (ctx, &loc, dirfd);
- if (libgf_get_fd_ctx (dir->dirfd))
+ if (op_ret == -1) {
+ fd_unref (dirfd);
+ dirfd = NULL;
goto out;
+ }
- if (!(libgf_alloc_fd_ctx (ctx, dir->dirfd))) {
- op_ret = -1;
- errno = EINVAL;
- goto out;
+ if (!libgf_get_fd_ctx (dirfd)) {
+ if (!(libgf_alloc_fd_ctx (ctx, dirfd))) {
+ op_ret = -1;
+ errno = EINVAL;
+ goto out;
+ }
}
- op_ret = 0;
out:
if (name)
FREE (name);
if (op_ret == -1) {
- FREE (dir);
fd_unref (dirfd);
- dir = NULL;
+ dirfd = NULL;
}
libgf_client_loc_wipe (&loc);
- return (glusterfs_dir_t)dir;
+ return dirfd;
}
glusterfs_dir_t
@@ -4704,7 +4680,7 @@ glusterfs_closedir (glusterfs_dir_t dirfd)
libglusterfs_client_fd_ctx_t *fdctx = NULL;
GF_VALIDATE_OR_GOTO (LIBGF_XL_NAME, dirfd, out);
- fdctx = libgf_get_fd_ctx (((struct libgf_dir_handle *)dirfd)->dirfd);
+ fdctx = libgf_get_fd_ctx (dirfd);
if (fdctx == NULL) {
errno = EBADF;
@@ -4712,9 +4688,8 @@ glusterfs_closedir (glusterfs_dir_t dirfd)
goto out;
}
- op_ret = libgf_client_flush (fdctx->ctx,
- ((struct libgf_dir_handle *)dirfd)->dirfd);
- fd_unref (((struct libgf_dir_handle *)dirfd)->dirfd);
+ op_ret = libgf_client_flush (fdctx->ctx, (fd_t *)dirfd);
+ fd_unref ((fd_t *)dirfd);
out:
return op_ret;