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 <buildbot@rampaginggeek.com>
Reviewed-by: Derrick Brashear <shadow@your-file-system.com>

diff --git a/src/vol/ihandle.c b/src/vol/ihandle.c
index 16d2332..3144b70 100644 (file)
--- 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;