Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 6 additions & 26 deletions runtime-light/stdlib/file/file-system-functions.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,7 @@
#include <unistd.h>
#include <utility>

#include "runtime-common/core/allocator/script-allocator.h"
#include "runtime-common/core/runtime-core.h"
#include "runtime-common/core/std/containers.h"
#include "runtime-common/stdlib/array/array-functions.h"
#include "runtime-common/stdlib/string/string-functions.h"
#include "runtime-light/coroutine/task.h"
Expand Down Expand Up @@ -204,40 +202,22 @@ inline Optional<string> f$file_get_contents(const string& stream) noexcept {
}

inline Optional<array<string>> f$file(const string& name) noexcept {
struct stat stat_buf {};

auto expected_file{kphp::fs::file::open(name.c_str(), "r")};
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: let's add explicit std::string_view creation here so we can pass name.size() to avoid call to std::strlen

if (!expected_file.has_value()) {
return false;
}
if (!k2::stat({name.c_str(), name.size()}, std::addressof(stat_buf)).has_value()) {
return false;
}
if (!S_ISREG(stat_buf.st_mode)) {
kphp::log::warning("regular file expected as first argument in function file, \"{}\" is given", name.c_str());
return false;
}

const size_t size{static_cast<size_t>(stat_buf.st_size)};
if (size > string::max_size()) {
kphp::log::warning("file \"{}\" is too large", name.c_str());
auto expected_file_content{std::move(*expected_file).get_contents()};
if (!expected_file_content.has_value()) {
return false;
}

kphp::stl::vector<std::byte, kphp::memory::script_allocator> file_content;
file_content.resize(size);
{
auto file{std::move(*expected_file)};
if (auto expected_read_result{file.read(file_content)}; !expected_read_result.has_value() || *expected_read_result < size) {
return false;
}
}
auto file_content{std::move(*expected_file_content)};

array<string> result;
int32_t prev{-1};
for (size_t i{0}; i < size; i++) {
if (static_cast<char>(file_content[i]) == '\n' || i + 1 == size) {
result.push_back(string{reinterpret_cast<char*>(file_content.data()) + prev + 1, static_cast<string::size_type>(i - prev)});
for (size_t i{0}; i < file_content.size(); i++) {
if (static_cast<char>(file_content[i]) == '\n' || i + 1 == file_content.size()) {
result.push_back(string{file_content.buffer() + prev + 1, static_cast<string::size_type>(i - prev)});
prev = i;
}
}
Expand Down
27 changes: 26 additions & 1 deletion runtime-light/stdlib/file/resource.h
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
#include <type_traits>
#include <utility>

#include <sys/stat.h>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

System headers should be placed in a single block


#include "runtime-common/core/allocator/script-allocator.h"
#include "runtime-common/core/class-instance/refcountable-php-classes.h"
#include "runtime-common/core/runtime-core.h"
Expand All @@ -25,6 +27,7 @@
#include "runtime-light/coroutine/task.h"
#include "runtime-light/k2-platform/k2-api.h"
#include "runtime-light/server/http/http-server-state.h"
#include "runtime-light/stdlib/diagnostics/logs.h"
#include "runtime-light/stdlib/output/output-state.h"
#include "runtime-light/streams/stream.h"

Expand Down Expand Up @@ -146,7 +149,29 @@ inline auto file::pread(std::span<std::byte> buf, uint64_t offset) noexcept -> s
}

inline auto file::get_contents() noexcept -> std::expected<string, int32_t> {
return std::unexpected{m_descriptor != k2::INVALID_PLATFORM_DESCRIPTOR ? k2::errno_efault : k2::errno_enodev};
struct stat stat_buf {};

return k2::fstat(m_descriptor, std::addressof(stat_buf)).and_then([&stat_buf, this] noexcept -> std::expected<string, int32_t> {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the point of wrapping everything inside lambdas? I doubt it makes this code more readable

if (!S_ISREG(stat_buf.st_mode)) {
kphp::log::warning("regular file expected");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return type of your lambda already contains error type: int32_t. I think it's better to remove these warnings and return proper errno instead

return std::unexpected{k2::errno_efault};
}

const size_t size{static_cast<size_t>(stat_buf.st_size)};
if (size > string::max_size()) {
kphp::log::warning("file is too large");
return std::unexpected{k2::errno_efault};
}

string file_content{static_cast<string::size_type>(size), false};
return read({reinterpret_cast<std::byte*>(file_content.buffer()), file_content.size()})
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. nit: std::as_writable_bytes can be cleaner here
  2. Isn't mmap a better choice here?

.and_then([size, &file_content](size_t read_result) noexcept -> std::expected<string, int32_t> {
if (read_result < size) {
return std::unexpected{k2::errno_efault};
}
return file_content;
});
});
}

inline auto file::flush() noexcept -> std::expected<void, int32_t> {
Expand Down
Loading