From 1cf6678fdaae82871a9aeb4addfed3a2db1954e8 Mon Sep 17 00:00:00 2001 From: Andrew Deason Date: Thu, 12 Sep 2013 15:58:34 -0500 Subject: [PATCH] ihandle: Make sure we don't ih_attachfd invalid FD Right now, if you give ih_attachfd_r an invalid fd, and fdLruHead is NULL, we'll return an FdHandle_t* for an invalid fd. Nowhere in the code is this possible right now, but the implementation of ih_attachfd_r and ih_attachfd doesn't make this very clear. Ideally the "close some fds and retry" behavior in ih_attachfd_r will be split out, so this code could be easier to follow, and we could implement open() EMFILE retrying for icreate operations. But for now, just make the current behavior clearer, so future modifications do not introduce such mistakes. Change-Id: Ibc80b32bc6f50480d12e3241fe198bc0587a962c Reviewed-on: http://gerrit.openafs.org/10249 Tested-by: BuildBot Reviewed-by: Derrick Brashear --- src/vol/ihandle.c | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/src/vol/ihandle.c b/src/vol/ihandle.c index 16d233231..3144b702f 100644 --- a/src/vol/ihandle.c +++ b/src/vol/ihandle.c @@ -320,10 +320,11 @@ streamHandleAllocateChunk(void) /* * Get a file descriptor handle given an Inode handle * Takes the given file descriptor, and creates a new FdHandle_t for it, - * attached to the given IHandle_t. fd can be INVALID_FD, indicating that the - * caller failed to open the relevant file because we had too many FDs open; - * ih_attachfd_r will then just evict/close an existing fd in the cache, and - * return NULL. + * attached to the given IHandle_t. If fdLruHead is not NULL, fd can be + * INVALID_FD, indicating that the caller failed to open the relevant file + * because we had too many FDs open; ih_attachfd_r will then just evict/close + * an existing fd in the cache, and return NULL. You must not call this + * function with an invalid fd while fdLruHead is NULL; instead, error out. */ static FdHandle_t * ih_attachfd_r(IHandle_t *ihP, FD_t fd) @@ -331,6 +332,11 @@ ih_attachfd_r(IHandle_t *ihP, FD_t fd) FD_t closeFd; FdHandle_t *fdP; + /* If the given fd is invalid, we must have an available fd to close. + * Otherwise, the caller must have realized this before calling + * ih_attachfd_r and yielded an error before getting here. */ + opr_Assert(fd != INVALID_FD || fdLruHead != NULL); + /* fdCacheSize limits the size of the descriptor cache, but * we permit the number of open files to exceed fdCacheSize. * We only recycle open file descriptors when the number @@ -390,14 +396,16 @@ ih_attachfd(IHandle_t *ihP, FD_t fd) { FdHandle_t *fdP; + if (fd == INVALID_FD) { + return NULL; + } + IH_LOCK; fdInUseCount += 1; fdP = ih_attachfd_r(ihP, fd); - if (!fdP) { - fdInUseCount -= 1; - } + opr_Assert(fdP); IH_UNLOCK; -- 2.39.5