Skip to content

Commit

Permalink
Kernel: Make Memory::Region allocation functions return KResultOr
Browse files Browse the repository at this point in the history
This makes for some nicer handling of errors compared to checking an
OwnPtr for null state.
  • Loading branch information
sin-ack authored and awesomekling committed Aug 15, 2021
1 parent 4bfd6e4 commit 0a18425
Show file tree
Hide file tree
Showing 5 changed files with 45 additions and 38 deletions.
26 changes: 14 additions & 12 deletions Kernel/Memory/AddressSpace.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -149,11 +149,11 @@ Optional<VirtualRange> AddressSpace::allocate_range(VirtualAddress vaddr, size_t

KResultOr<Region*> AddressSpace::try_allocate_split_region(Region const& source_region, VirtualRange const& range, size_t offset_in_vmobject)
{
auto new_region = Region::try_create_user_accessible(
auto maybe_new_region = Region::try_create_user_accessible(
range, source_region.vmobject(), offset_in_vmobject, KString::try_create(source_region.name()), source_region.access(), source_region.is_cacheable() ? Region::Cacheable::Yes : Region::Cacheable::No, source_region.is_shared());
if (!new_region)
return ENOMEM;
auto* region = add_region(new_region.release_nonnull());
if (maybe_new_region.is_error())
return maybe_new_region.error();
auto* region = add_region(maybe_new_region.release_value());
if (!region)
return ENOMEM;
region->set_syscall_region(source_region.is_syscall_region());
Expand All @@ -173,12 +173,14 @@ KResultOr<Region*> AddressSpace::allocate_region(VirtualRange const& range, Stri
auto maybe_vmobject = AnonymousVMObject::try_create_with_size(range.size(), strategy);
if (maybe_vmobject.is_error())
return maybe_vmobject.error();
auto region = Region::try_create_user_accessible(range, maybe_vmobject.release_value(), 0, KString::try_create(name), prot_to_region_access_flags(prot), Region::Cacheable::Yes, false);
if (!region)
return ENOMEM;
auto maybe_region = Region::try_create_user_accessible(range, maybe_vmobject.release_value(), 0, KString::try_create(name), prot_to_region_access_flags(prot), Region::Cacheable::Yes, false);
if (maybe_region.is_error())
return maybe_region.error();

auto region = maybe_region.release_value();
if (!region->map(page_directory()))
return ENOMEM;
auto* added_region = add_region(region.release_nonnull());
auto* added_region = add_region(move(region));
if (!added_region)
return ENOMEM;
return added_region;
Expand All @@ -201,12 +203,12 @@ KResultOr<Region*> AddressSpace::allocate_region_with_vmobject(VirtualRange cons
return EINVAL;
}
offset_in_vmobject &= PAGE_MASK;
auto region = Region::try_create_user_accessible(range, move(vmobject), offset_in_vmobject, KString::try_create(name), prot_to_region_access_flags(prot), Region::Cacheable::Yes, shared);
if (!region) {
auto maybe_region = Region::try_create_user_accessible(range, move(vmobject), offset_in_vmobject, KString::try_create(name), prot_to_region_access_flags(prot), Region::Cacheable::Yes, shared);
if (maybe_region.is_error()) {
dbgln("allocate_region_with_vmobject: Unable to allocate Region");
return ENOMEM;
return maybe_region.error();
}
auto* added_region = add_region(region.release_nonnull());
auto* added_region = add_region(maybe_region.release_value());
if (!added_region)
return ENOMEM;
if (!added_region->map(page_directory()))
Expand Down
9 changes: 6 additions & 3 deletions Kernel/Memory/MemoryManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -743,9 +743,12 @@ OwnPtr<Region> MemoryManager::allocate_kernel_region(PhysicalAddress paddr, size

OwnPtr<Region> MemoryManager::allocate_kernel_region_with_vmobject(VirtualRange const& range, VMObject& vmobject, StringView name, Region::Access access, Region::Cacheable cacheable)
{
auto region = Region::try_create_kernel_only(range, vmobject, 0, KString::try_create(name), access, cacheable);
if (region)
region->map(kernel_page_directory());
auto maybe_region = Region::try_create_kernel_only(range, vmobject, 0, KString::try_create(name), access, cacheable);
if (maybe_region.is_error())
return {};

auto region = maybe_region.release_value();
region->map(kernel_page_directory());
return region;
}

Expand Down
34 changes: 18 additions & 16 deletions Kernel/Memory/Region.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ Region::~Region()
}
}

OwnPtr<Region> Region::clone()
KResultOr<NonnullOwnPtr<Region>> Region::try_clone()
{
VERIFY(Process::current());

Expand All @@ -60,12 +60,14 @@ OwnPtr<Region> Region::clone()
VERIFY(vmobject().is_shared_inode());

// Create a new region backed by the same VMObject.
auto region = Region::try_create_user_accessible(
auto maybe_region = Region::try_create_user_accessible(
m_range, m_vmobject, m_offset_in_vmobject, m_name ? m_name->try_clone() : OwnPtr<KString> {}, access(), m_cacheable ? Cacheable::Yes : Cacheable::No, m_shared);
if (!region) {
if (maybe_region.is_error()) {
dbgln("Region::clone: Unable to allocate new Region");
return nullptr;
return maybe_region.error();
}

auto region = maybe_region.release_value();
region->set_mmap(m_mmap);
region->set_shared(m_shared);
region->set_syscall_region(is_syscall_region());
Expand All @@ -77,16 +79,19 @@ OwnPtr<Region> Region::clone()

auto maybe_vmobject_clone = vmobject().try_clone();
if (maybe_vmobject_clone.is_error())
return {};
return maybe_vmobject_clone.error();
auto vmobject_clone = maybe_vmobject_clone.release_value();

// Set up a COW region. The parent (this) region becomes COW as well!
remap();
auto clone_region = Region::try_create_user_accessible(
m_range, maybe_vmobject_clone.release_value(), m_offset_in_vmobject, m_name ? m_name->try_clone() : OwnPtr<KString> {}, access(), m_cacheable ? Cacheable::Yes : Cacheable::No, m_shared);
if (!clone_region) {
auto maybe_clone_region = Region::try_create_user_accessible(
m_range, vmobject_clone, m_offset_in_vmobject, m_name ? m_name->try_clone() : OwnPtr<KString> {}, access(), m_cacheable ? Cacheable::Yes : Cacheable::No, m_shared);
if (maybe_clone_region.is_error()) {
dbgln("Region::clone: Unable to allocate new Region for COW");
return nullptr;
return maybe_clone_region.error();
}
auto clone_region = maybe_clone_region.release_value();

if (m_stack) {
VERIFY(is_readable());
VERIFY(is_writable());
Expand Down Expand Up @@ -143,17 +148,14 @@ size_t Region::amount_shared() const
return bytes;
}

OwnPtr<Region> Region::try_create_user_accessible(VirtualRange const& range, NonnullRefPtr<VMObject> vmobject, size_t offset_in_vmobject, OwnPtr<KString> name, Region::Access access, Cacheable cacheable, bool shared)
KResultOr<NonnullOwnPtr<Region>> Region::try_create_user_accessible(VirtualRange const& range, NonnullRefPtr<VMObject> vmobject, size_t offset_in_vmobject, OwnPtr<KString> name, Region::Access access, Cacheable cacheable, bool shared)
{
auto region = adopt_own_if_nonnull(new (nothrow) Region(range, move(vmobject), offset_in_vmobject, move(name), access, cacheable, shared));
if (!region)
return nullptr;
return region;
return adopt_nonnull_own_or_enomem(new (nothrow) Region(range, move(vmobject), offset_in_vmobject, move(name), access, cacheable, shared));
}

OwnPtr<Region> Region::try_create_kernel_only(VirtualRange const& range, NonnullRefPtr<VMObject> vmobject, size_t offset_in_vmobject, OwnPtr<KString> name, Region::Access access, Cacheable cacheable)
KResultOr<NonnullOwnPtr<Region>> Region::try_create_kernel_only(VirtualRange const& range, NonnullRefPtr<VMObject> vmobject, size_t offset_in_vmobject, OwnPtr<KString> name, Region::Access access, Cacheable cacheable)
{
return adopt_own_if_nonnull(new (nothrow) Region(range, move(vmobject), offset_in_vmobject, move(name), access, cacheable, false));
return adopt_nonnull_own_or_enomem(new (nothrow) Region(range, move(vmobject), offset_in_vmobject, move(name), access, cacheable, false));
}

bool Region::should_cow(size_t page_index) const
Expand Down
6 changes: 3 additions & 3 deletions Kernel/Memory/Region.h
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,8 @@ class Region final
Yes,
};

static OwnPtr<Region> try_create_user_accessible(VirtualRange const&, NonnullRefPtr<VMObject>, size_t offset_in_vmobject, OwnPtr<KString> name, Region::Access access, Cacheable, bool shared);
static OwnPtr<Region> try_create_kernel_only(VirtualRange const&, NonnullRefPtr<VMObject>, size_t offset_in_vmobject, OwnPtr<KString> name, Region::Access access, Cacheable = Cacheable::Yes);
static KResultOr<NonnullOwnPtr<Region>> try_create_user_accessible(VirtualRange const&, NonnullRefPtr<VMObject>, size_t offset_in_vmobject, OwnPtr<KString> name, Region::Access access, Cacheable, bool shared);
static KResultOr<NonnullOwnPtr<Region>> try_create_kernel_only(VirtualRange const&, NonnullRefPtr<VMObject>, size_t offset_in_vmobject, OwnPtr<KString> name, Region::Access access, Cacheable = Cacheable::Yes);

~Region();

Expand Down Expand Up @@ -90,7 +90,7 @@ class Region final

PageFaultResponse handle_fault(PageFault const&);

OwnPtr<Region> clone();
KResultOr<NonnullOwnPtr<Region>> try_clone();

bool contains(VirtualAddress vaddr) const
{
Expand Down
8 changes: 4 additions & 4 deletions Kernel/Syscalls/fork.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -96,14 +96,14 @@ KResultOr<FlatPtr> Process::sys$fork(RegisterState& regs)
ScopedSpinLock lock(address_space().get_lock());
for (auto& region : address_space().regions()) {
dbgln_if(FORK_DEBUG, "fork: cloning Region({}) '{}' @ {}", region, region->name(), region->vaddr());
auto region_clone = region->clone();
if (!region_clone) {
auto maybe_region_clone = region->try_clone();
if (maybe_region_clone.is_error()) {
dbgln("fork: Cannot clone region, insufficient memory");
// TODO: tear down new process?
return ENOMEM;
return maybe_region_clone.error();
}

auto* child_region = child->address_space().add_region(region_clone.release_nonnull());
auto* child_region = child->address_space().add_region(maybe_region_clone.release_value());
if (!child_region) {
dbgln("fork: Cannot add region, insufficient memory");
// TODO: tear down new process?
Expand Down

0 comments on commit 0a18425

Please sign in to comment.