Kernel/Devices: Defer creation of SysFS component after the constructor

Instead of doing so in the constructor, let's do immediately after the
constructor, so we can safely pass a reference of a Device, so the
SysFSDeviceComponent constructor can use that object to identify whether
it's a block device or a character device.
This allows to us to not hold a device in SysFSDeviceComponent with a
RefPtr.
Also, we also call the before_removing method in both SlavePTY::unref
and File::unref, so because Device has that method being overrided, it
can ensure the device is removed always cleanly.

Kernel/Devices/Device.cpp

@@ -26,8 +26,9 @@ NonnullRefPtr<SysFSDeviceComponent> SysFSDeviceComponent::must_create(Device con
 }
 SysFSDeviceComponent::SysFSDeviceComponent(Device const& device)
     : SysFSComponent(String::formatted("{}:{}", device.major(), device.minor()))
-    , m_associated_device(device)
+    , m_block_device(device.is_block_device())
 {
+    VERIFY(device.is_block_device() || device.is_character_device());
 }
 
 UNMAP_AFTER_INIT NonnullRefPtr<SysFSDevicesDirectory> SysFSDevicesDirectory::must_create(SysFSRootDirectory const& root_directory)
@@ -58,7 +59,7 @@ KResult SysFSBlockDevicesDirectory::traverse_as_directory(unsigned fsid, Functio
 
     SysFSComponentRegistry::the().devices_list().with_exclusive([&](auto& list) -> void {
         for (auto& exposed_device : list) {
-            if (!exposed_device.device().is_block_device())
+            if (!exposed_device.is_block_device())
                 continue;
             callback({ exposed_device.name(), { fsid, exposed_device.component_index() }, 0 });
         }
@@ -69,7 +70,7 @@ RefPtr<SysFSComponent> SysFSBlockDevicesDirectory::lookup(StringView name)
 {
     return SysFSComponentRegistry::the().devices_list().with_exclusive([&](auto& list) -> RefPtr<SysFSComponent> {
         for (auto& exposed_device : list) {
-            if (!exposed_device.device().is_block_device())
+            if (!exposed_device.is_block_device())
                 continue;
             if (exposed_device.name() == name)
                 return exposed_device;
@@ -94,7 +95,7 @@ KResult SysFSCharacterDevicesDirectory::traverse_as_directory(unsigned fsid, Fun
 
     SysFSComponentRegistry::the().devices_list().with_exclusive([&](auto& list) -> void {
         for (auto& exposed_device : list) {
-            if (!exposed_device.device().is_character_device())
+            if (exposed_device.is_block_device())
                 continue;
             callback({ exposed_device.name(), { fsid, exposed_device.component_index() }, 0 });
         }
@@ -105,7 +106,7 @@ RefPtr<SysFSComponent> SysFSCharacterDevicesDirectory::lookup(StringView name)
 {
     return SysFSComponentRegistry::the().devices_list().with_exclusive([&](auto& list) -> RefPtr<SysFSComponent> {
         for (auto& exposed_device : list) {
-            if (!exposed_device.device().is_character_device())
+            if (exposed_device.is_block_device())
                 continue;
             if (exposed_device.name() == name)
                 return exposed_device;
@@ -145,6 +146,11 @@ Device::Device(unsigned major, unsigned minor)
         VERIFY(!map.contains(device_id));
         map.set(device_id, this);
     });
+}
+
+void Device::after_inserting()
+{
+    VERIFY(!m_sysfs_component);
     auto sys_fs_component = SysFSDeviceComponent::must_create(*this);
     m_sysfs_component = sys_fs_component;
     SysFSComponentRegistry::the().devices_list().with_exclusive([&](auto& list) -> void {
@@ -154,12 +160,11 @@ Device::Device(unsigned major, unsigned minor)
 
 void Device::before_removing()
 {
-    auto sys_fs_component = m_sysfs_component.strong_ref();
-    VERIFY(sys_fs_component);
-    SysFSComponentRegistry::the().devices_list().with_exclusive([&](auto& list) -> void {
-        list.remove(*sys_fs_component);
-    });
     m_state = State::BeingRemoved;
+    VERIFY(m_sysfs_component);
+    SysFSComponentRegistry::the().devices_list().with_exclusive([&](auto& list) -> void {
+        list.remove(*m_sysfs_component);
+    });
 }
 
 Device::~Device()

Kernel/Devices/Device.h

@@ -18,6 +18,7 @@
 #include <AK/Function.h>
 #include <AK/HashMap.h>
 #include <AK/RefPtr.h>
+#include <Kernel/API/KResult.h>
 #include <Kernel/Devices/AsyncDeviceRequest.h>
 #include <Kernel/FileSystem/File.h>
 #include <Kernel/FileSystem/SysFS.h>
@@ -26,6 +27,14 @@
 
 namespace Kernel {
 
+template<typename DeviceType, typename... Args>
+inline KResultOr<NonnullRefPtr<DeviceType>> try_create_device(Args&&... args)
+{
+    auto device = TRY(adopt_nonnull_ref_or_enomem(new DeviceType(forward<Args>(args)...)));
+    device->after_inserting();
+    return device;
+}
+
 class Device : public File {
 protected:
     enum class State {
@@ -46,7 +55,8 @@ public:
     GroupID gid() const { return m_gid; }
 
     virtual bool is_device() const override { return true; }
-    virtual void before_removing();
+    virtual void before_removing() override;
+    virtual void after_inserting();
 
     static void for_each(Function<void(Device&)>);
     static Device* get_device(unsigned major, unsigned minor);
@@ -82,7 +92,7 @@ private:
 
     Spinlock m_requests_lock;
     DoublyLinkedList<RefPtr<AsyncDeviceRequest>> m_requests;
-    WeakPtr<SysFSDeviceComponent> m_sysfs_component;
+    RefPtr<SysFSDeviceComponent> m_sysfs_component;
 };
 
 }

Kernel/Devices/FullDevice.cpp

@@ -13,7 +13,10 @@ namespace Kernel {
 
 UNMAP_AFTER_INIT NonnullRefPtr<FullDevice> FullDevice::must_create()
 {
-    return adopt_ref(*new FullDevice);
+    auto full_device_or_error = try_create_device<FullDevice>();
+    // FIXME: Find a way to propagate errors
+    VERIFY(!full_device_or_error.is_error());
+    return full_device_or_error.release_value();
 }
 
 UNMAP_AFTER_INIT FullDevice::FullDevice()

Kernel/Devices/FullDevice.h

@@ -16,9 +16,10 @@ public:
     static NonnullRefPtr<FullDevice> must_create();
     virtual ~FullDevice() override;
 
-private:
+    // FIXME: We expose this constructor to make try_create_device helper to work
     FullDevice();
 
+private:
     // ^CharacterDevice
     virtual KResultOr<size_t> read(OpenFileDescription&, u64, UserOrKernelBuffer&, size_t) override;
     virtual KResultOr<size_t> write(OpenFileDescription&, u64, const UserOrKernelBuffer&, size_t) override;

Kernel/Devices/HID/HIDManagement.cpp

@@ -112,6 +112,7 @@ UNMAP_AFTER_INIT void HIDManagement::enumerate()
     m_i8042_controller->detect_devices();
     if (m_i8042_controller->mouse())
         m_hid_devices.append(m_i8042_controller->mouse().release_nonnull());
+
     if (m_i8042_controller->keyboard())
         m_hid_devices.append(m_i8042_controller->keyboard().release_nonnull());
 }

Kernel/Devices/HID/PS2KeyboardDevice.cpp

@@ -83,9 +83,11 @@ bool PS2KeyboardDevice::handle_irq(const RegisterState&)
 
 UNMAP_AFTER_INIT RefPtr<PS2KeyboardDevice> PS2KeyboardDevice::try_to_initialize(const I8042Controller& ps2_controller)
 {
-    auto device = adopt_ref(*new PS2KeyboardDevice(ps2_controller));
-    if (device->initialize())
-        return device;
+    auto keyboard_device_or_error = try_create_device<PS2KeyboardDevice>(ps2_controller);
+    // FIXME: Find a way to propagate errors
+    VERIFY(!keyboard_device_or_error.is_error());
+    if (keyboard_device_or_error.value()->initialize())
+        return keyboard_device_or_error.release_value();
     return nullptr;
 }
 

Kernel/Devices/HID/PS2KeyboardDevice.h

@@ -35,9 +35,10 @@ public:
         enable_irq();
     }
 
-private:
+    // FIXME: We expose this constructor to make try_create_device helper to work
     explicit PS2KeyboardDevice(const I8042Controller&);
 
+private:
     // ^IRQHandler
     virtual bool handle_irq(const RegisterState&) override;
 

Kernel/Devices/HID/PS2MouseDevice.cpp

@@ -174,9 +174,11 @@ void PS2MouseDevice::set_sample_rate(u8 rate)
 
 UNMAP_AFTER_INIT RefPtr<PS2MouseDevice> PS2MouseDevice::try_to_initialize(const I8042Controller& ps2_controller)
 {
-    auto device = adopt_ref(*new PS2MouseDevice(ps2_controller));
-    if (device->initialize())
-        return device;
+    auto mouse_device_or_error = try_create_device<PS2MouseDevice>(ps2_controller);
+    // FIXME: Find a way to propagate errors
+    VERIFY(!mouse_device_or_error.is_error());
+    if (mouse_device_or_error.value()->initialize())
+        return mouse_device_or_error.release_value();
     return nullptr;
 }
 

Kernel/Devices/HID/PS2MouseDevice.h

@@ -32,8 +32,10 @@ public:
         enable_irq();
     }
 
-protected:
+    // FIXME: We expose this constructor to make try_create_device helper to work
     explicit PS2MouseDevice(const I8042Controller&);
+
+protected:
     // ^IRQHandler
     virtual bool handle_irq(const RegisterState&) override;
 

Kernel/Devices/HID/VMWareMouseDevice.cpp

@@ -16,9 +16,11 @@ UNMAP_AFTER_INIT RefPtr<VMWareMouseDevice> VMWareMouseDevice::try_to_initialize(
         return {};
     if (!VMWareBackdoor::the()->vmmouse_is_absolute())
         return {};
-    auto device = adopt_ref(*new VMWareMouseDevice(ps2_controller));
-    if (device->initialize())
-        return device;
+    auto mouse_device_or_error = try_create_device<VMWareMouseDevice>(ps2_controller);
+    // FIXME: Find a way to propagate errors
+    VERIFY(!mouse_device_or_error.is_error());
+    if (mouse_device_or_error.value()->initialize())
+        return mouse_device_or_error.release_value();
     return {};
 }
 

Kernel/Devices/HID/VMWareMouseDevice.h

@@ -23,7 +23,7 @@ public:
     // ^I8042Device
     virtual void irq_handle_byte_read(u8 byte) override;
 
-private:
+    // FIXME: We expose this constructor to make try_create_device helper to work
     explicit VMWareMouseDevice(const I8042Controller&);
 };
 

Kernel/Devices/KCOVDevice.cpp

@@ -20,7 +20,10 @@ HashMap<ThreadID, KCOVInstance*>* KCOVDevice::thread_instance;
 
 UNMAP_AFTER_INIT NonnullRefPtr<KCOVDevice> KCOVDevice::must_create()
 {
-    return adopt_ref(*new KCOVDevice);
+    auto kcov_device_or_error = try_create_device<KCOVDevice>();
+    // FIXME: Find a way to propagate errors
+    VERIFY(!kcov_device_or_error.is_error());
+    return kcov_device_or_error.release_value();
 }
 
 UNMAP_AFTER_INIT KCOVDevice::KCOVDevice()

Kernel/Devices/KCOVDevice.h

@@ -25,6 +25,9 @@ public:
     KResultOr<Memory::Region*> mmap(Process&, OpenFileDescription&, Memory::VirtualRange const&, u64 offset, int prot, bool shared) override;
     KResultOr<NonnullRefPtr<OpenFileDescription>> open(int options) override;
 
+    // FIXME: We expose this constructor to make try_create_device helper to work
+    KCOVDevice();
+
 protected:
     virtual StringView class_name() const override { return "KCOVDevice"; }
 
@@ -34,9 +37,6 @@ protected:
     virtual KResultOr<size_t> read(OpenFileDescription&, u64, UserOrKernelBuffer&, size_t) override { return EINVAL; }
     virtual KResultOr<size_t> write(OpenFileDescription&, u64, const UserOrKernelBuffer&, size_t) override { return EINVAL; }
     virtual KResult ioctl(OpenFileDescription&, unsigned request, Userspace<void*> arg) override;
-
-private:
-    KCOVDevice();
 };
 
 }

Kernel/Devices/MemoryDevice.cpp

@@ -15,7 +15,10 @@ namespace Kernel {
 
 UNMAP_AFTER_INIT NonnullRefPtr<MemoryDevice> MemoryDevice::must_create()
 {
-    return adopt_ref(*new MemoryDevice);
+    auto memory_device_or_error = try_create_device<MemoryDevice>();
+    // FIXME: Find a way to propagate errors
+    VERIFY(!memory_device_or_error.is_error());
+    return memory_device_or_error.release_value();
 }
 
 UNMAP_AFTER_INIT MemoryDevice::MemoryDevice()

Kernel/Devices/MemoryDevice.h

@@ -21,8 +21,10 @@ public:
 
     virtual KResultOr<Memory::Region*> mmap(Process&, OpenFileDescription&, Memory::VirtualRange const&, u64 offset, int prot, bool shared) override;
 
-private:
+    // FIXME: We expose this constructor to make try_create_device helper to work
     MemoryDevice();
+
+private:
     virtual StringView class_name() const override { return "MemoryDevice"; }
     virtual bool can_read(const OpenFileDescription&, size_t) const override { return true; }
     virtual bool can_write(const OpenFileDescription&, size_t) const override { return false; }

Kernel/Devices/NullDevice.cpp

@@ -15,6 +15,7 @@ static Singleton<NullDevice> s_the;
 UNMAP_AFTER_INIT void NullDevice::initialize()
 {
     s_the.ensure_instance();
+    s_the->after_inserting();
 }
 
 NullDevice& NullDevice::the()

Kernel/Devices/RandomDevice.cpp

@@ -12,7 +12,10 @@ namespace Kernel {
 
 UNMAP_AFTER_INIT NonnullRefPtr<RandomDevice> RandomDevice::must_create()
 {
-    return adopt_ref(*new RandomDevice);
+    auto random_device_or_error = try_create_device<RandomDevice>();
+    // FIXME: Find a way to propagate errors
+    VERIFY(!random_device_or_error.is_error());
+    return random_device_or_error.release_value();
 }
 
 UNMAP_AFTER_INIT RandomDevice::RandomDevice()

Kernel/Devices/RandomDevice.h

@@ -16,9 +16,10 @@ public:
     static NonnullRefPtr<RandomDevice> must_create();
     virtual ~RandomDevice() override;
 
-private:
+    // FIXME: We expose this constructor to make try_create_device helper to work
     RandomDevice();
 
+private:
     // ^CharacterDevice
     virtual KResultOr<size_t> read(OpenFileDescription&, u64, UserOrKernelBuffer&, size_t) override;
     virtual KResultOr<size_t> write(OpenFileDescription&, u64, const UserOrKernelBuffer&, size_t) override;

Kernel/Devices/SB16.cpp

@@ -89,6 +89,7 @@ UNMAP_AFTER_INIT void SB16::detect()
 UNMAP_AFTER_INIT void SB16::create()
 {
     s_the.ensure_instance();
+    s_the->after_inserting();
 }
 
 SB16& SB16::the()

Kernel/Devices/ZeroDevice.cpp

@@ -12,7 +12,10 @@ namespace Kernel {
 
 UNMAP_AFTER_INIT NonnullRefPtr<ZeroDevice> ZeroDevice::must_create()
 {
-    return adopt_ref(*new ZeroDevice);
+    auto zero_device_or_error = try_create_device<ZeroDevice>();
+    // FIXME: Find a way to propagate errors
+    VERIFY(!zero_device_or_error.is_error());
+    return zero_device_or_error.release_value();
 }
 
 UNMAP_AFTER_INIT ZeroDevice::ZeroDevice()

Kernel/Devices/ZeroDevice.h

@@ -16,8 +16,10 @@ public:
     static NonnullRefPtr<ZeroDevice> must_create();
     virtual ~ZeroDevice() override;
 
-private:
+    // FIXME: We expose this constructor to make try_create_device helper to work
     ZeroDevice();
+
+private:
     // ^CharacterDevice
     virtual KResultOr<size_t> read(OpenFileDescription&, u64, UserOrKernelBuffer&, size_t) override;
     virtual KResultOr<size_t> write(OpenFileDescription&, u64, const UserOrKernelBuffer&, size_t) override;