Add support for removing directories.

It's really only supported in Ext2FS since SynthFS doesn't really want you
mucking around with its files. This is pretty neat though :^)

I ran into some trouble with HashMap while working on this but opted to work
around it and leave that for a separate investigation.

Kernel/Ext2FileSystem.cpp

@@ -313,6 +313,20 @@ void Ext2FS::free_inode(Ext2FSInode& inode)
         set_block_allocation_state(group_index, block_index, false);
 
     set_inode_allocation_state(inode.index(), false);
+
+    if (inode.is_directory()) {
+        auto& bgd = const_cast<ext2_group_desc&>(group_descriptor(group_index_from_inode(inode.index())));
+        --bgd.bg_used_dirs_count;
+        dbgprintf("Ext2FS: decremented bg_used_dirs_count %u -> %u\n", bgd.bg_used_dirs_count - 1, bgd.bg_used_dirs_count);
+        flush_block_group_descriptor_table();
+    }
+}
+
+void Ext2FS::flush_block_group_descriptor_table()
+{
+    unsigned blocks_to_write = ceilDiv(m_blockGroupCount * (unsigned)sizeof(ext2_group_desc), blockSize());
+    unsigned first_block_of_bgdt = blockSize() == 1024 ? 2 : 1;
+    writeBlocks(first_block_of_bgdt, blocks_to_write, m_cached_group_descriptor_table);
 }
 
 Ext2FSInode::Ext2FSInode(Ext2FS& fs, unsigned index, const ext2_inode& raw_inode)
@@ -356,10 +370,16 @@ void Ext2FSInode::flush_metadata()
     dbgprintf("Ext2FSInode: flush_metadata for inode %u\n", index());
     fs().write_ext2_inode(index(), m_raw_inode);
     if (is_directory()) {
-        // FIXME: This invalidation is way too hardcore.
+        // Unless we're about to go away permanently, invalidate the lookup cache.
+        if (m_raw_inode.i_links_count != 0) {
             LOCKER(m_lock);
+            // FIXME: Something isn't working right when we hit this code path.
+            //        I've seen crashes inside HashMap::clear() all the way down in DoublyLinkedList::clear().
+            //        My guess would be a HashTable bug.
+            // FIXME: This invalidation is way too hardcore. It's sad to throw away the whole cache.
             m_lookup_cache.clear();
         }
+    }
     set_metadata_dirty(false);
 }
 
@@ -401,7 +421,7 @@ RetainPtr<Inode> Ext2FS::get_inode(InodeIdentifier inode) const
     return new_inode;
 }
 
-ssize_t Ext2FSInode::read_bytes(off_t offset, size_t count, byte* buffer, FileDescriptor*)
+ssize_t Ext2FSInode::read_bytes(off_t offset, size_t count, byte* buffer, FileDescriptor*) const
 {
     ASSERT(offset >= 0);
     if (m_raw_inode.i_size == 0)
@@ -550,7 +570,7 @@ ssize_t Ext2FSInode::write_bytes(off_t offset, size_t count, const byte* data, F
     return nwritten;
 }
 
-bool Ext2FSInode::traverse_as_directory(Function<bool(const FS::DirectoryEntry&)> callback)
+bool Ext2FSInode::traverse_as_directory(Function<bool(const FS::DirectoryEntry&)> callback) const
 {
     ASSERT(metadata().isDirectory());
 
@@ -610,6 +630,9 @@ bool Ext2FSInode::add_child(InodeIdentifier child_id, const String& name, byte f
 
 bool Ext2FSInode::remove_child(const String& name, int& error)
 {
+#ifdef EXT2_DEBUG
+    dbgprintf("Ext2FSInode::remove_child(%s) in inode %u\n", name.characters(), index());
+#endif
     ASSERT(is_directory());
 
     unsigned child_inode_index;
@@ -710,7 +733,9 @@ bool Ext2FS::write_directory_inode(unsigned directoryInode, Vector<DirectoryEntr
     kprintf("\n");
 #endif
 
-    return get_inode({ fsid(), directoryInode })->write_bytes(0, directoryData.size(), directoryData.pointer(), nullptr);
+    auto directory_inode = get_inode({ fsid(), directoryInode });
+    ssize_t nwritten = directory_inode->write_bytes(0, directoryData.size(), directoryData.pointer(), nullptr);
+    return nwritten == directoryData.size();
 }
 
 unsigned Ext2FS::inodes_per_block() const
@@ -955,10 +980,7 @@ bool Ext2FS::set_inode_allocation_state(unsigned index, bool newState)
         ++mutableBGD.bg_free_inodes_count;
     dbgprintf("Ext2FS: group free inode count %u -> %u\n", bgd.bg_free_inodes_count, bgd.bg_free_inodes_count - 1);
 
-    unsigned blocksToWrite = ceilDiv(m_blockGroupCount * (unsigned)sizeof(ext2_group_desc), blockSize());
+    flush_block_group_descriptor_table();
-    unsigned firstBlockOfBGDT = blockSize() == 1024 ? 2 : 1;
-    writeBlocks(firstBlockOfBGDT, blocksToWrite, m_cached_group_descriptor_table);
-    
     return true;
 }
 
@@ -1000,10 +1022,7 @@ bool Ext2FS::set_block_allocation_state(GroupIndex group, BlockIndex bi, bool ne
         ++mutableBGD.bg_free_blocks_count;
     dbgprintf("Ext2FS: group free block count %u -> %u\n", bgd.bg_free_blocks_count, bgd.bg_free_blocks_count - 1);
 
-    unsigned blocksToWrite = ceilDiv(m_blockGroupCount * (unsigned)sizeof(ext2_group_desc), blockSize());
+    flush_block_group_descriptor_table();
-    unsigned firstBlockOfBGDT = blockSize() == 1024 ? 2 : 1;
-    writeBlocks(firstBlockOfBGDT, blocksToWrite, m_cached_group_descriptor_table);
-    
     return true;
 }
 
@@ -1040,9 +1059,7 @@ RetainPtr<Inode> Ext2FS::create_directory(InodeIdentifier parent_id, const Strin
     ++bgd.bg_used_dirs_count;
     dbgprintf("Ext2FS: incremented bg_used_dirs_count %u -> %u\n", bgd.bg_used_dirs_count - 1, bgd.bg_used_dirs_count);
 
-    unsigned blocksToWrite = ceilDiv(m_blockGroupCount * (unsigned)sizeof(ext2_group_desc), blockSize());
+    flush_block_group_descriptor_table();
-    unsigned firstBlockOfBGDT = blockSize() == 1024 ? 2 : 1;
-    writeBlocks(firstBlockOfBGDT, blocksToWrite, m_cached_group_descriptor_table);
 
     error = 0;
     return inode;
@@ -1165,7 +1182,7 @@ RetainPtr<Inode> Ext2FSInode::parent() const
     return fs().get_inode(m_parent_id);
 }
 
-void Ext2FSInode::populate_lookup_cache()
+void Ext2FSInode::populate_lookup_cache() const
 {
     {
         LOCKER(m_lock);
@@ -1267,3 +1284,11 @@ void Ext2FS::uncache_inode(InodeIndex index)
     LOCKER(m_inode_cache_lock);
     m_inode_cache.remove(index);
 }
+
+size_t Ext2FSInode::directory_entry_count() const
+{
+    ASSERT(is_directory());
+    populate_lookup_cache();
+    LOCKER(m_lock);
+    return m_lookup_cache.size();
+}

Kernel/Ext2FileSystem.h

@@ -25,9 +25,9 @@ public:
 
 private:
     // ^Inode
-    virtual ssize_t read_bytes(off_t, size_t, byte* buffer, FileDescriptor*) override;
+    virtual ssize_t read_bytes(off_t, size_t, byte* buffer, FileDescriptor*) const override;
     virtual InodeMetadata metadata() const override;
-    virtual bool traverse_as_directory(Function<bool(const FS::DirectoryEntry&)>) override;
+    virtual bool traverse_as_directory(Function<bool(const FS::DirectoryEntry&)>) const override;
     virtual InodeIdentifier lookup(const String& name) override;
     virtual String reverse_lookup(InodeIdentifier) override;
     virtual void flush_metadata() override;
@@ -40,15 +40,16 @@ private:
     virtual int set_mtime(time_t) override;
     virtual int increment_link_count() override;
     virtual int decrement_link_count() override;
+    virtual size_t directory_entry_count() const override;
 
-    void populate_lookup_cache();
+    void populate_lookup_cache() const;
 
     Ext2FS& fs();
     const Ext2FS& fs() const;
     Ext2FSInode(Ext2FS&, unsigned index, const ext2_inode&);
 
-    Vector<unsigned> m_block_list;
+    mutable Vector<unsigned> m_block_list;
-    HashMap<String, unsigned> m_lookup_cache;
+    mutable HashMap<String, unsigned> m_lookup_cache;
     ext2_inode m_raw_inode;
     mutable InodeIdentifier m_parent_id;
 };
@@ -68,6 +69,7 @@ private:
 
     const ext2_super_block& super_block() const;
     const ext2_group_desc& group_descriptor(unsigned groupIndex) const;
+    void flush_block_group_descriptor_table();
     unsigned first_block_of_group(unsigned groupIndex) const;
     unsigned inodes_per_block() const;
     unsigned inodes_per_group() const;

Kernel/FileSystem.cpp

@@ -48,7 +48,7 @@ FS* FS::from_fsid(dword id)
     return nullptr;
 }
 
-ByteBuffer Inode::read_entire(FileDescriptor* descriptor)
+ByteBuffer Inode::read_entire(FileDescriptor* descriptor) const
 {
     size_t initial_size = metadata().size ? metadata().size : 4096;
     auto contents = ByteBuffer::create_uninitialized(initial_size);

Kernel/FileSystem.h

@@ -78,16 +78,17 @@ public:
     InodeIdentifier identifier() const { return { fsid(), index() }; }
     virtual InodeMetadata metadata() const = 0;
 
-    ByteBuffer read_entire(FileDescriptor* = nullptr);
+    ByteBuffer read_entire(FileDescriptor* = nullptr) const;
 
-    virtual ssize_t read_bytes(off_t, size_t, byte* buffer, FileDescriptor*) = 0;
+    virtual ssize_t read_bytes(off_t, size_t, byte* buffer, FileDescriptor*) const = 0;
-    virtual bool traverse_as_directory(Function<bool(const FS::DirectoryEntry&)>) = 0;
+    virtual bool traverse_as_directory(Function<bool(const FS::DirectoryEntry&)>) const = 0;
     virtual InodeIdentifier lookup(const String& name) = 0;
     virtual String reverse_lookup(InodeIdentifier) = 0;
     virtual ssize_t write_bytes(off_t, size_t, const byte* data, FileDescriptor*) = 0;
     virtual bool add_child(InodeIdentifier child_id, const String& name, byte file_type, int& error) = 0;
     virtual bool remove_child(const String& name, int& error) = 0;
     virtual RetainPtr<Inode> parent() const = 0;
+    virtual size_t directory_entry_count() const = 0;
 
     bool is_metadata_dirty() const { return m_metadata_dirty; }
 
@@ -109,7 +110,7 @@ protected:
     Inode(FS& fs, unsigned index);
     void set_metadata_dirty(bool b) { m_metadata_dirty = b; }
 
-    Lock m_lock;
+    mutable Lock m_lock;
 
 private:
     FS& m_fs;

Kernel/Process.cpp

@@ -2101,6 +2101,16 @@ int Process::sys$unlink(const char* pathname)
     return 0;
 }
 
+int Process::sys$rmdir(const char* pathname)
+{
+    if (!validate_read_str(pathname))
+        return -EFAULT;
+    int error;
+    if (!VFS::the().rmdir(String(pathname), *cwd_inode(), error))
+        return error;
+    return 0;
+}
+
 int Process::sys$read_tsc(dword* lsw, dword* msw)
 {
     if (!validate_write_typed(lsw))

Kernel/Process.h

@@ -194,6 +194,7 @@ public:
     clock_t sys$times(tms*);
     int sys$utime(const char* pathname, const struct utimbuf*);
     int sys$unlink(const char* pathname);
+    int sys$rmdir(const char* pathname);
     int sys$read_tsc(dword* lsw, dword* msw);
 
     int gui$create_window(const GUI_WindowParameters*);

Kernel/SyntheticFileSystem.cpp

@@ -200,7 +200,7 @@ InodeMetadata SynthFSInode::metadata() const
     return m_metadata;
 }
 
-ssize_t SynthFSInode::read_bytes(off_t offset, size_t count, byte* buffer, FileDescriptor* descriptor)
+ssize_t SynthFSInode::read_bytes(off_t offset, size_t count, byte* buffer, FileDescriptor* descriptor) const
 {
 #ifdef SYNTHFS_DEBUG
     kprintf("SynthFS: read_bytes %u\n", index());
@@ -211,10 +211,10 @@ ssize_t SynthFSInode::read_bytes(off_t offset, size_t count, byte* buffer, FileD
     ByteBuffer generatedData;
     if (m_generator) {
         if (!descriptor) {
-            generatedData = m_generator(*this);
+            generatedData = m_generator(const_cast<SynthFSInode&>(*this));
         } else {
             if (!descriptor->generator_cache())
-                descriptor->generator_cache() = m_generator(*this);
+                descriptor->generator_cache() = m_generator(const_cast<SynthFSInode&>(*this));
             generatedData = descriptor->generator_cache();
         }
     }
@@ -227,7 +227,7 @@ ssize_t SynthFSInode::read_bytes(off_t offset, size_t count, byte* buffer, FileD
     return nread;
 }
 
-bool SynthFSInode::traverse_as_directory(Function<bool(const FS::DirectoryEntry&)> callback)
+bool SynthFSInode::traverse_as_directory(Function<bool(const FS::DirectoryEntry&)> callback) const
 {
     InterruptDisabler disabler;
 #ifdef SYNTHFS_DEBUG
@@ -305,3 +305,10 @@ bool SynthFSInode::remove_child(const String& name, int& error)
 SynthFSInodeCustomData::~SynthFSInodeCustomData()
 {
 }
+
+size_t SynthFSInode::directory_entry_count() const
+{
+    ASSERT(is_directory());
+    // NOTE: The 2 is for '.' and '..'
+    return m_children.size() + 2;
+}

Kernel/SyntheticFileSystem.h

@@ -54,9 +54,9 @@ public:
 
 private:
     // ^Inode
-    virtual ssize_t read_bytes(off_t, size_t, byte* buffer, FileDescriptor*) override;
+    virtual ssize_t read_bytes(off_t, size_t, byte* buffer, FileDescriptor*) const override;
     virtual InodeMetadata metadata() const override;
-    virtual bool traverse_as_directory(Function<bool(const FS::DirectoryEntry&)>) override;
+    virtual bool traverse_as_directory(Function<bool(const FS::DirectoryEntry&)>) const override;
     virtual InodeIdentifier lookup(const String& name) override;
     virtual String reverse_lookup(InodeIdentifier) override;
     virtual void flush_metadata() override;
@@ -64,6 +64,7 @@ private:
     virtual bool add_child(InodeIdentifier child_id, const String& name, byte file_type, int& error) override;
     virtual bool remove_child(const String& name, int& error) override;
     virtual RetainPtr<Inode> parent() const override;
+    virtual size_t directory_entry_count() const override;
 
     SynthFS& fs();
     const SynthFS& fs() const;

Kernel/Syscall.cpp

@@ -219,6 +219,8 @@ static dword handle(RegisterDump& regs, dword function, dword arg1, dword arg2,
         return current->gui$notify_paint_finished((int)arg1, (const GUI_Rect*)arg2);
     case Syscall::SC_gui_set_global_cursor_tracking_enabled:
         return current->gui$set_global_cursor_tracking_enabled((int)arg1, (bool)arg2);
+    case Syscall::SC_rmdir:
+        return current->sys$rmdir((const char*)arg1);
     default:
         kprintf("<%u> int0x80: Unknown function %u requested {%x, %x, %x}\n", current->pid(), function, arg1, arg2, arg3);
         break;

Kernel/Syscall.h

@@ -82,6 +82,7 @@
     __ENUMERATE_SYSCALL(gui_set_window_rect) \
     __ENUMERATE_SYSCALL(gui_notify_paint_finished) \
     __ENUMERATE_SYSCALL(gui_set_global_cursor_tracking_enabled) \
+    __ENUMERATE_SYSCALL(rmdir) \
 
 
 #ifdef SERENITY

Kernel/VirtualFileSystem.cpp

@@ -260,6 +260,65 @@ bool VFS::unlink(const String& path, Inode& base, int& error)
     return true;
 }
 
+bool VFS::rmdir(const String& path, Inode& base, int& error)
+{
+    error = -EWHYTHO;
+    // FIXME: This won't work nicely across mount boundaries.
+    FileSystemPath p(path);
+    if (!p.is_valid()) {
+        error = -EINVAL;
+        return false;
+    }
+
+    InodeIdentifier parent_dir;
+    auto inode_id = resolve_path(path, base.identifier(), error, 0, &parent_dir);
+    if (!inode_id.is_valid()) {
+        error = -ENOENT;
+        return false;
+    }
+
+    if (inode_id.fs()->is_readonly()) {
+        error = -EROFS;
+        return false;
+    }
+
+    // FIXME: We should return EINVAL if the last component of the path is "."
+    // FIXME: We should return ENOTEMPTY if the last component of the path is ".."
+
+    auto inode = get_inode(inode_id);
+    if (!inode->is_directory()) {
+        error = -ENOTDIR;
+        return false;
+    }
+
+    if (inode->directory_entry_count() != 2) {
+        error = -ENOTEMPTY;
+        return false;
+    }
+
+    auto parent_inode = get_inode(parent_dir);
+    ASSERT(parent_inode);
+
+    dbgprintf("VFS::rmdir: Removing inode %u:%u from parent %u:%u\n", inode_id.fsid(), inode_id.index(), parent_dir.fsid(), parent_dir.index());
+
+    // To do:
+    // - Remove '.' in target (--child.link_count)
+    // - Remove '..' in target (--parent.link_count)
+    // - Remove target from its parent (--parent.link_count)
+    if (!inode->remove_child(".", error))
+        return false;
+
+    if (!inode->remove_child("..", error))
+        return false;
+
+    // FIXME: The reverse_lookup here can definitely be avoided.
+    if (!parent_inode->remove_child(parent_inode->reverse_lookup(inode_id), error))
+        return false;
+
+    error = 0;
+    return true;
+}
+
 InodeIdentifier VFS::resolve_symbolic_link(InodeIdentifier base, Inode& symlink_inode, int& error)
 {
     auto symlink_contents = symlink_inode.read_entire();
@@ -315,7 +374,7 @@ String VFS::absolute_path(Inode& core_inode)
     return builder.build();
 }
 
-InodeIdentifier VFS::resolve_path(const String& path, InodeIdentifier base, int& error, int options, InodeIdentifier* deepest_dir)
+InodeIdentifier VFS::resolve_path(const String& path, InodeIdentifier base, int& error, int options, InodeIdentifier* parent_id)
 {
     if (path.is_empty()) {
         error = -EINVAL;
@@ -330,8 +389,8 @@ InodeIdentifier VFS::resolve_path(const String& path, InodeIdentifier base, int&
     else
         crumb_id = base.is_valid() ? base : root_inode_id();
 
-    if (deepest_dir)
+    if (parent_id)
-        *deepest_dir = crumb_id;
+        *parent_id = crumb_id;
 
     for (unsigned i = 0; i < parts.size(); ++i) {
         bool inode_was_root_at_head_of_loop = crumb_id.is_root_inode();
@@ -383,8 +442,10 @@ InodeIdentifier VFS::resolve_path(const String& path, InodeIdentifier base, int&
         crumb_inode = get_inode(crumb_id);
         metadata = crumb_inode->metadata();
         if (metadata.isDirectory()) {
-            if (deepest_dir)
+            if (i != parts.size() - 1) {
-                *deepest_dir = crumb_id;
+                if (parent_id)
+                    *parent_id = crumb_id;
+            }
         }
         if (metadata.isSymbolicLink()) {
             if (i == parts.size() - 1) {

Kernel/VirtualFileSystem.h

@@ -68,6 +68,7 @@ public:
     RetainPtr<FileDescriptor> create(const String& path, int& error, int options, mode_t mode, InodeIdentifier base);
     bool mkdir(const String& path, mode_t mode, InodeIdentifier base, int& error);
     bool unlink(const String& path, Inode& base, int& error);
+    bool rmdir(const String& path, Inode& base, int& error);
 
     void register_character_device(CharacterDevice&);
 
@@ -90,7 +91,7 @@ private:
     bool is_vfs_root(InodeIdentifier) const;
 
     void traverse_directory_inode(Inode&, Function<bool(const FS::DirectoryEntry&)>);
-    InodeIdentifier resolve_path(const String& path, InodeIdentifier base, int& error, int options = 0, InodeIdentifier* deepest_dir = nullptr);
+    InodeIdentifier resolve_path(const String& path, InodeIdentifier base, int& error, int options = 0, InodeIdentifier* parent_id = nullptr);
     InodeIdentifier resolve_symbolic_link(InodeIdentifier base, Inode& symlink_inode, int& error);
 
     Mount* find_mount_for_host(InodeIdentifier);

Kernel/sync.sh

@@ -43,6 +43,7 @@ cp -v ../Userland/touch mnt/bin/touch
 cp -v ../Userland/sync mnt/bin/sync
 cp -v ../Userland/more mnt/bin/more
 cp -v ../Userland/rm mnt/bin/rm
+cp -v ../Userland/rmdir mnt/bin/rmdir
 cp -v ../Userland/cp mnt/bin/cp
 cp -v ../Userland/guitest mnt/bin/guitest
 cp -v ../Userland/guitest2 mnt/bin/guitest2

LibC/errno_numbers.h

@@ -44,6 +44,7 @@
     __ERROR(EWHYTHO,        "Failed without setting an error code (Bug!)") \
     __ERROR(EBADWINDOW,     "Bad window ID") \
     __ERROR(EBADBACKING,    "Bad backing store ID") \
+    __ERROR(ENOTEMPTY,      "Directory not empty") \
 
 
 enum __errno_values {

LibC/unistd.cpp

@@ -210,6 +210,12 @@ int unlink(const char* pathname)
     __RETURN_WITH_ERRNO(rc, rc, -1);
 }
 
+int rmdir(const char* pathname)
+{
+    int rc = syscall(SC_rmdir, pathname);
+    __RETURN_WITH_ERRNO(rc, rc, -1);
+}
+
 int isatty(int fd)
 {
     int rc = syscall(SC_isatty, fd);

LibC/unistd.h

@@ -48,6 +48,7 @@ int ttyname_r(int fd, char* buffer, size_t);
 off_t lseek(int fd, off_t, int whence);
 int link(const char* oldpath, const char* newpath);
 int unlink(const char* pathname);
+int rmdir(const char* pathname);
 int getdtablesize();
 int dup(int old_fd);
 int dup2(int old_fd, int new_fd);

Userland/.gitignore

@@ -26,3 +26,4 @@ guitest2
 sysctl
 rm
 cp
+rmdir

Userland/Makefile

@@ -23,6 +23,7 @@ OBJS = \
        guitest2.o \
        sysctl.o \
        cp.o \
+       rmdir.o \
        rm.o
 
 APPS = \
@@ -51,6 +52,7 @@ APPS = \
        guitest2 \
        sysctl \
        cp \
+       rmdir \
        rm
 
 ARCH_FLAGS =
@@ -149,6 +151,9 @@ cp: cp.o
 rm: rm.o
 	$(LD) -o $@ $(LDFLAGS) $< ../LibC/LibC.a
 
+rmdir: rmdir.o
+	$(LD) -o $@ $(LDFLAGS) $< ../LibC/LibC.a
+
 .cpp.o:
 	@echo "CXX $<"; $(CXX) $(CXXFLAGS) -o $@ -c $<
 

Userland/rmdir.cpp

@@ -0,0 +1,18 @@
+#include <stdio.h>
+#include <errno.h>
+#include <unistd.h>
+
+int main(int argc, char** argv)
+{
+    if (argc != 2) {
+        fprintf(stderr, "usage: rmdir <path>\n");
+        return 1;
+    }
+    int rc = rmdir(argv[1]);
+    if (rc < 0) {
+        perror("rmdir");
+        return 1;
+    }
+    return 0;
+}
+