End a directory's unfinished enumerations when the directory is closed

src/fs.cpp

@@ -254,11 +254,49 @@ kal_uintptr put_name(const char* src, kal_uintptr n,
 // calling.
 struct listing {
     int      fd;
+    int      dir;      // the descriptor of the directory it was begun upon
+    listing* next;
     okl_uptr used;
     okl_uptr pos;
     char     buf[8192];
 };
 
+// THE ENUMERATIONS NOT YET READ TO THE END, BECAUSE CLOSING THE DIRECTORY MUST
+// END THEM. fs.h releases an iterator "by reading past the end or by closing
+// the directory that produced it", and the descriptor a listing holds is its
+// own, so closing the directory's descriptor does not reach it. Measured before
+// this list existed: a C library above that opened a directory larger than its
+// buffer, read one entry and closed it, 64 times, was left holding 64 more
+// descriptors.
+//
+// The lock spins rather than suspends, because suspending would make openkal.fs
+// depend upon openkal.task, as src/memory.cpp says of its own.
+listing*     g_listings = nullptr;
+volatile int g_listings_held = 0;
+
+void listings_lock() {
+    while (__atomic_exchange_n(&g_listings_held, 1, __ATOMIC_ACQUIRE)) okl::sys(okl::nr_sched_yield);
+}
+void listings_unlock() { __atomic_store_n(&g_listings_held, 0, __ATOMIC_RELEASE); }
+
+// Ends every listing begun upon `dir', or `one' alone where `dir' is negative.
+// What is ended is unlinked under the lock and released after it.
+void listings_end(int dir, listing* one) {
+    listing* done = nullptr;
+    listings_lock();
+    for (listing** p = &g_listings; *p != nullptr;) {
+        listing* s = *p;
+        if (dir >= 0 ? s->dir == dir : s == one) { *p = s->next; s->next = done; done = s; }
+        else p = &s->next;
+    }
+    listings_unlock();
+    while (done != nullptr) {
+        listing* s = done; done = s->next;
+        okl::sys(okl::nr_close, s->fd);
+        kal_free(s, sizeof(listing), alignof(listing));
+    }
+}
+
 }  // namespace
 
 extern "C" {
@@ -312,7 +350,11 @@ int kal_fs_open(kal_dir base, const char* name, kal_uintptr len,
 
 void kal_fs_close_dir(kal_dir d) {
     const int fd = okl::unpack(d.h);
-    if (fd >= 0) { okl::retire(d.h); okl::sys(okl::nr_close, fd); }
+    if (fd < 0) return;
+    // Before the close, because after it the number may already name another
+    // directory whose enumerations are not this one's.
+    listings_end(fd, nullptr);
+    okl::retire(d.h); okl::sys(okl::nr_close, fd);
 }
 
 void kal_fs_close_file(kal_file f) {
@@ -515,7 +557,8 @@ int kal_fs_list_begin(kal_dir d, kal_uintptr* iter) {
     if (okl::failed(own)) return okl::translate(own);
     auto* s = static_cast<listing*>(kal_alloc(sizeof(listing), alignof(listing)));
     if (s == nullptr) { okl::sys(okl::nr_close, own); return kal_err_no_memory; }
-    s->fd = static_cast<int>(own); s->used = 0; s->pos = 0;
+    s->fd = static_cast<int>(own); s->dir = fd; s->used = 0; s->pos = 0;
+    listings_lock(); s->next = g_listings; g_listings = s; listings_unlock();
     *iter = reinterpret_cast<kal_uintptr>(s);
     return kal_ok;
 }
@@ -532,8 +575,7 @@ int kal_fs_list_next(kal_dir, kal_uintptr* iter,
                                         static_cast<okl_long>(sizeof s->buf));
             if (okl::interrupted(r)) continue;
             if (okl::failed(r) || r == 0) {
-                okl::sys(okl::nr_close, s->fd);
-                kal_free(s, sizeof(listing), alignof(listing));
+                listings_end(-1, s);
                 *iter = 0;
                 if (name_len) *name_len = 0;
                 return okl::failed(r) ? okl::translate(r) : kal_ok;

tests/conformance_fs.cpp

@@ -95,6 +95,48 @@ int main() {
     check(kal_fs_open(d, "inner", 5, kal::fs::open::read.bits, &after) != kal_ok,
           "a released handle is not treated as valid");
 
+    // An enumeration left part-way is ended by closing the directory that
+    // produced it, which fs.h states. Counted in the kernel's own table, through
+    // the whole file system this implementation supplies as a directory.
+    {
+        kal_dir slash{}; bool have_root = false;
+        for (kal_uintptr i = 0; i < kal::fs::preopen_count(); ++i) {
+            char nm[512]; kal_uintptr nl = 0; kal_dir p{};
+            if (kal_fs_preopen(i, &p, nm, sizeof nm, &nl) == kal_ok && nl == 1 && nm[0] == '/') {
+                slash = p; have_root = true;
+            }
+        }
+        auto open_now = [&]() -> int {
+            kal_dir fds{}; kal_uintptr it = 0; int n = 0;
+            if (kal_fs_open_dir(slash, "proc/self/fd", 12, &fds) != kal_ok) return -1;
+            if (kal_fs_list_begin(fds, &it) == kal_ok) {
+                for (;;) {
+                    char name[512]; kal_uintptr len = 0; int kind = 0;
+                    if (kal_fs_list_next(fds, &it, name, sizeof name, &len, &kind) != kal_ok || it == 0) break;
+                    ++n;
+                }
+            }
+            kal_fs_close_dir(fds);
+            return n;
+        };
+        if (have_root) {
+            const int before = open_now();
+            check(before >= 0, "the open descriptors are counted through proc/self/fd");
+            if (before >= 0) {
+                for (int i = 0; i < 64; ++i) {
+                    kal_dir again{}; kal_uintptr it = 0;
+                    if (kal_fs_open_dir(root, "okl_dir", 7, &again) != kal_ok) break;
+                    if (kal_fs_list_begin(again, &it) == kal_ok) {
+                        char name[512]; kal_uintptr len = 0; int kind = 0;
+                        kal_fs_list_next(again, &it, name, sizeof name, &len, &kind);
+                    }
+                    kal_fs_close_dir(again);
+                }
+                check(open_now() == before, "closing a directory ends an enumeration left part-way");
+            }
+        }
+    }
+
     check(kal_fs_remove(root, "okl_dir/inner", 13) == kal_ok, "the inner file is removed");
     check(kal_fs_remove(root, "okl_dir", 7) == kal_ok, "the directory is removed");
     check(kal_fs_remove(root, "okl_probe.txt", 13) == kal_ok, "the file is removed");