Skip to content

Commit

Permalink
LibCore+Everywhere: Make Core::Stream::read() return Bytes
Browse files Browse the repository at this point in the history
A mistake I've repeatedly made is along these lines:
```c++
auto nread = TRY(source_file->read(buffer));
TRY(destination_file->write(buffer));
```

It's a little clunky to have to create a Bytes or StringView from the
buffer's data pointer and the nread, and easy to forget and just use
the buffer. So, this patch changes the read() function to return a
Bytes of the data that were just read.

The other read_foo() methods will be modified in the same way in
subsequent commits.

Fixes #13687
  • Loading branch information
AtkinsSJ authored and trflynn89 committed Apr 16, 2022
1 parent 6654efc commit 3b1e063
Show file tree
Hide file tree
Showing 22 changed files with 103 additions and 103 deletions.
4 changes: 2 additions & 2 deletions Meta/Lagom/Tools/CodeGenerators/LibUnicode/GeneratorUtil.h
Original file line number Diff line number Diff line change
Expand Up @@ -259,8 +259,8 @@ inline ErrorOr<JsonValue> read_json_file(StringView path)

// FIXME: When Core::Stream supports reading an entire file, use that.
while (TRY(file->can_read_line())) {
auto nread = TRY(file->read(buffer));
TRY(builder.try_append(reinterpret_cast<char const*>(buffer.data()), nread));
auto bytes_read = TRY(file->read(buffer));
TRY(builder.try_append(StringView { bytes_read }));
}

return JsonValue::from_string(builder.build());
Expand Down
44 changes: 22 additions & 22 deletions Tests/LibCore/TestLibCoreStream.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ TEST_CASE(file_read_bytes)

auto result = file->read(buffer);
EXPECT(!result.is_error());
EXPECT_EQ(result.value(), 131ul);
EXPECT_EQ(result.value().size(), 131ul);

StringView buffer_contents { buffer.bytes() };
EXPECT_EQ(buffer_contents, expected_buffer_contents);
Expand Down Expand Up @@ -193,11 +193,11 @@ TEST_CASE(tcp_socket_read)
auto maybe_receive_buffer = ByteBuffer::create_uninitialized(64);
EXPECT(!maybe_receive_buffer.is_error());
auto receive_buffer = maybe_receive_buffer.release_value();
auto maybe_nread = client_socket->read(receive_buffer);
EXPECT(!maybe_nread.is_error());
auto nread = maybe_nread.release_value();
auto maybe_read_bytes = client_socket->read(receive_buffer);
EXPECT(!maybe_read_bytes.is_error());
auto read_bytes = maybe_read_bytes.release_value();

StringView received_data { receive_buffer.data(), nread };
StringView received_data { read_bytes };
EXPECT_EQ(sent_data, received_data);
}

Expand Down Expand Up @@ -226,11 +226,11 @@ TEST_CASE(tcp_socket_write)
auto maybe_receive_buffer = ByteBuffer::create_uninitialized(64);
EXPECT(!maybe_receive_buffer.is_error());
auto receive_buffer = maybe_receive_buffer.release_value();
auto maybe_nread = server_socket->read(receive_buffer);
EXPECT(!maybe_nread.is_error());
auto nread = maybe_nread.release_value();
auto maybe_read_bytes = server_socket->read(receive_buffer);
EXPECT(!maybe_read_bytes.is_error());
auto read_bytes = maybe_read_bytes.release_value();

StringView received_data { receive_buffer.data(), nread };
StringView received_data { read_bytes };
EXPECT_EQ(sent_data, received_data);
}

Expand Down Expand Up @@ -262,7 +262,7 @@ TEST_CASE(tcp_socket_eof)
auto maybe_receive_buffer = ByteBuffer::create_uninitialized(1);
EXPECT(!maybe_receive_buffer.is_error());
auto receive_buffer = maybe_receive_buffer.release_value();
EXPECT_EQ(client_socket->read(receive_buffer).release_value(), 0ul);
EXPECT(client_socket->read(receive_buffer).release_value().is_empty());
EXPECT(client_socket->is_eof());
}

Expand Down Expand Up @@ -309,11 +309,11 @@ TEST_CASE(udp_socket_read_write)
auto maybe_client_receive_buffer = ByteBuffer::create_uninitialized(64);
EXPECT(!maybe_client_receive_buffer.is_error());
auto client_receive_buffer = maybe_client_receive_buffer.release_value();
auto maybe_nread = client_socket->read(client_receive_buffer);
EXPECT(!maybe_nread.is_error());
auto nread = maybe_nread.release_value();
auto maybe_read_bytes = client_socket->read(client_receive_buffer);
EXPECT(!maybe_read_bytes.is_error());
auto read_bytes = maybe_read_bytes.release_value();

StringView client_received_data { client_receive_buffer.data(), nread };
StringView client_received_data { read_bytes };
EXPECT_EQ(udp_reply_data, client_received_data);
}

Expand Down Expand Up @@ -353,11 +353,11 @@ TEST_CASE(local_socket_read)
auto maybe_receive_buffer = ByteBuffer::create_uninitialized(64);
EXPECT(!maybe_receive_buffer.is_error());
auto receive_buffer = maybe_receive_buffer.release_value();
auto maybe_nread = client_socket->read(receive_buffer);
EXPECT(!maybe_nread.is_error());
auto nread = maybe_nread.release_value();
auto maybe_read_bytes = client_socket->read(receive_buffer);
EXPECT(!maybe_read_bytes.is_error());
auto read_bytes = maybe_read_bytes.release_value();

StringView received_data { receive_buffer.data(), nread };
StringView received_data { read_bytes };
EXPECT_EQ(sent_data, received_data);

return 0;
Expand All @@ -384,11 +384,11 @@ TEST_CASE(local_socket_write)
auto maybe_receive_buffer = ByteBuffer::create_uninitialized(pending_bytes);
EXPECT(!maybe_receive_buffer.is_error());
auto receive_buffer = maybe_receive_buffer.release_value();
auto maybe_nread = server_socket->read(receive_buffer);
EXPECT(!maybe_nread.is_error());
EXPECT_EQ(maybe_nread.value(), sent_data.length());
auto maybe_read_bytes = server_socket->read(receive_buffer);
EXPECT(!maybe_read_bytes.is_error());
EXPECT_EQ(maybe_read_bytes.value().size(), sent_data.length());

StringView received_data { receive_buffer.data(), maybe_nread.value() };
StringView received_data { maybe_read_bytes.value() };
EXPECT_EQ(sent_data, received_data);

event_loop.quit(0);
Expand Down
4 changes: 2 additions & 2 deletions Tests/LibTLS/TestTLSHandshake.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,8 @@ TEST_CASE(test_TLS_hello_handshake)
auto tls = MUST(TLS::TLSv12::connect(DEFAULT_SERVER, port, move(options)));
ByteBuffer contents;
tls->on_ready_to_read = [&] {
auto nread = MUST(tls->read(contents.must_get_bytes_for_writing(4 * KiB)));
if (nread == 0) {
auto read_bytes = MUST(tls->read(contents.must_get_bytes_for_writing(4 * KiB)));
if (read_bytes.is_empty()) {
FAIL("No data received");
loop.quit(1);
}
Expand Down
2 changes: 1 addition & 1 deletion Userland/Libraries/LibAudio/FlacLoader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ MaybeLoaderError FlacLoaderPlugin::parse_header()
[[maybe_unused]] u128 md5_checksum;
VERIFY(streaminfo_data->is_aligned_to_byte_boundary());
auto md5_bytes_read = LOADER_TRY(streaminfo_data->read(md5_checksum.bytes()));
FLAC_VERIFY(md5_bytes_read == md5_checksum.my_size(), LoaderError::Category::IO, "MD5 Checksum size");
FLAC_VERIFY(md5_bytes_read.size() == md5_checksum.my_size(), LoaderError::Category::IO, "MD5 Checksum size");
md5_checksum.bytes().copy_to({ m_md5_checksum, sizeof(m_md5_checksum) });

// Parse other blocks
Expand Down
20 changes: 10 additions & 10 deletions Userland/Libraries/LibCore/EventLoop.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -162,31 +162,31 @@ class InspectorServerConnection : public Object {
#ifdef __serenity__
m_socket->on_ready_to_read = [this] {
u32 length;
auto maybe_nread = m_socket->read({ (u8*)&length, sizeof(length) });
if (maybe_nread.is_error()) {
dbgln("InspectorServerConnection: Failed to read message length from inspector server connection: {}", maybe_nread.error());
auto maybe_bytes_read = m_socket->read({ (u8*)&length, sizeof(length) });
if (maybe_bytes_read.is_error()) {
dbgln("InspectorServerConnection: Failed to read message length from inspector server connection: {}", maybe_bytes_read.error());
shutdown();
return;
}

auto nread = maybe_nread.release_value();
if (nread == 0) {
auto bytes_read = maybe_bytes_read.release_value();
if (bytes_read.is_empty()) {
dbgln_if(EVENTLOOP_DEBUG, "RPC client disconnected");
shutdown();
return;
}

VERIFY(nread == sizeof(length));
VERIFY(bytes_read.size() == sizeof(length));

auto request_buffer = ByteBuffer::create_uninitialized(length).release_value();
maybe_nread = m_socket->read(request_buffer.bytes());
if (maybe_nread.is_error()) {
dbgln("InspectorServerConnection: Failed to read message content from inspector server connection: {}", maybe_nread.error());
maybe_bytes_read = m_socket->read(request_buffer.bytes());
if (maybe_bytes_read.is_error()) {
dbgln("InspectorServerConnection: Failed to read message content from inspector server connection: {}", maybe_bytes_read.error());
shutdown();
return;
}

nread = maybe_nread.release_value();
bytes_read = maybe_bytes_read.release_value();

auto request_json = JsonValue::from_string(request_buffer);
if (request_json.is_error() || !request_json.value().is_object()) {
Expand Down
2 changes: 1 addition & 1 deletion Userland/Libraries/LibCore/InputBitStream.h
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ class BigEndianInputBitStream : public Stream {

// ^Stream
virtual bool is_readable() const override { return m_stream.is_readable(); }
virtual ErrorOr<size_t> read(Bytes bytes) override
virtual ErrorOr<Bytes> read(Bytes bytes) override
{
if (m_current_byte.has_value() && is_aligned_to_byte_boundary()) {
bytes[0] = m_current_byte.release_value();
Expand Down
6 changes: 3 additions & 3 deletions Userland/Libraries/LibCore/MemoryStream.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,15 +29,15 @@ class MemoryStream final : public SeekableStream {
// FIXME: It doesn't make sense to truncate a memory stream. Therefore, we don't do anything here. Is that fine?
virtual ErrorOr<void> truncate(off_t) override { return Error::from_errno(ENOTSUP); }

virtual ErrorOr<size_t> read(Bytes bytes) override
virtual ErrorOr<Bytes> read(Bytes bytes) override
{
auto to_read = min(remaining(), bytes.size());
if (to_read == 0)
return 0;
return Bytes {};

m_bytes.slice(m_offset, to_read).copy_to(bytes);
m_offset += to_read;
return bytes.size();
return bytes.trim(to_read);
}

virtual ErrorOr<off_t> seek(i64 offset, SeekMode seek_mode = SeekMode::SetPosition) override
Expand Down
16 changes: 8 additions & 8 deletions Userland/Libraries/LibCore/SOCKSProxyClient.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ ErrorOr<void> send_version_identifier_and_method_selection_message(Core::Stream:
return Error::from_string_literal("SOCKS negotiation failed: Failed to send version identifier and method selection message");

Socks5InitialResponse response;
size = TRY(socket.read({ &response, sizeof(response) }));
size = TRY(socket.read({ &response, sizeof(response) })).size();
if (size != sizeof(response))
return Error::from_string_literal("SOCKS negotiation failed: Failed to receive initial response");

Expand Down Expand Up @@ -169,34 +169,34 @@ ErrorOr<Reply> send_connect_request_message(Core::Stream::Socket& socket, Core::
return Error::from_string_literal("SOCKS negotiation failed: Failed to send connect request");

Socks5ConnectResponseHeader response_header;
size = TRY(socket.read({ &response_header, sizeof(response_header) }));
size = TRY(socket.read({ &response_header, sizeof(response_header) })).size();
if (size != sizeof(response_header))
return Error::from_string_literal("SOCKS negotiation failed: Failed to receive connect response header");

if (response_header.version_identifier != to_underlying(version))
return Error::from_string_literal("SOCKS negotiation failed: Invalid version identifier");

u8 response_address_type;
size = TRY(socket.read({ &response_address_type, sizeof(response_address_type) }));
size = TRY(socket.read({ &response_address_type, sizeof(response_address_type) })).size();
if (size != sizeof(response_address_type))
return Error::from_string_literal("SOCKS negotiation failed: Failed to receive connect response address type");

switch (AddressType(response_address_type)) {
case AddressType::IPV4: {
u8 response_address_data[4];
size = TRY(socket.read({ response_address_data, sizeof(response_address_data) }));
size = TRY(socket.read({ response_address_data, sizeof(response_address_data) })).size();
if (size != sizeof(response_address_data))
return Error::from_string_literal("SOCKS negotiation failed: Failed to receive connect response address data");
break;
}
case AddressType::DomainName: {
u8 response_address_length;
size = TRY(socket.read({ &response_address_length, sizeof(response_address_length) }));
size = TRY(socket.read({ &response_address_length, sizeof(response_address_length) })).size();
if (size != sizeof(response_address_length))
return Error::from_string_literal("SOCKS negotiation failed: Failed to receive connect response address length");
ByteBuffer buffer;
buffer.resize(response_address_length);
size = TRY(socket.read(buffer));
size = TRY(socket.read(buffer)).size();
if (size != response_address_length)
return Error::from_string_literal("SOCKS negotiation failed: Failed to receive connect response address data");
break;
Expand All @@ -207,7 +207,7 @@ ErrorOr<Reply> send_connect_request_message(Core::Stream::Socket& socket, Core::
}

u16 bound_port;
size = TRY(socket.read({ &bound_port, sizeof(bound_port) }));
size = TRY(socket.read({ &bound_port, sizeof(bound_port) })).size();
if (size != sizeof(bound_port))
return Error::from_string_literal("SOCKS negotiation failed: Failed to receive connect response bound port");

Expand Down Expand Up @@ -247,7 +247,7 @@ ErrorOr<u8> send_username_password_authentication_message(Core::Stream::Socket&
return Error::from_string_literal("SOCKS negotiation failed: Failed to send username/password authentication message");

Socks5UsernamePasswordResponse response;
size = TRY(socket.read({ &response, sizeof(response) }));
size = TRY(socket.read({ &response, sizeof(response) })).size();
if (size != sizeof(response))
return Error::from_string_literal("SOCKS negotiation failed: Failed to receive username/password authentication response");

Expand Down
2 changes: 1 addition & 1 deletion Userland/Libraries/LibCore/SOCKSProxyClient.h
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ class SOCKSProxyClient final : public Stream::Socket {
virtual ~SOCKSProxyClient() override;

// ^Stream::Stream
virtual ErrorOr<size_t> read(Bytes bytes) override { return m_socket.read(bytes); }
virtual ErrorOr<Bytes> read(Bytes bytes) override { return m_socket.read(bytes); }
virtual ErrorOr<size_t> write(ReadonlyBytes bytes) override { return m_socket.write(bytes); }
virtual bool is_eof() const override { return m_socket.is_eof(); }
virtual bool is_open() const override { return m_socket.is_open(); }
Expand Down
12 changes: 6 additions & 6 deletions Userland/Libraries/LibCore/Stream.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ bool Stream::read_or_error(Bytes buffer)
return false;
}

nread += result.value();
nread += result.value().size();
} while (nread < buffer.size());

return true;
Expand Down Expand Up @@ -156,7 +156,7 @@ ErrorOr<void> File::open_path(StringView filename, mode_t permissions)
bool File::is_readable() const { return has_flag(m_mode, OpenMode::Read); }
bool File::is_writable() const { return has_flag(m_mode, OpenMode::Write); }

ErrorOr<size_t> File::read(Bytes buffer)
ErrorOr<Bytes> File::read(Bytes buffer)
{
if (!has_flag(m_mode, OpenMode::Read)) {
// NOTE: POSIX says that if the fd is not open for reading, the call
Expand All @@ -167,7 +167,7 @@ ErrorOr<size_t> File::read(Bytes buffer)

ssize_t nread = TRY(System::read(m_fd, buffer));
m_last_read_was_eof = nread == 0;
return nread;
return buffer.trim(nread);
}

ErrorOr<size_t> File::write(ReadonlyBytes buffer)
Expand Down Expand Up @@ -322,7 +322,7 @@ ErrorOr<void> Socket::connect_inet(int fd, SocketAddress const& address)
return System::connect(fd, bit_cast<struct sockaddr*>(&addr), sizeof(addr));
}

ErrorOr<size_t> PosixSocketHelper::read(Bytes buffer, int flags)
ErrorOr<Bytes> PosixSocketHelper::read(Bytes buffer, int flags)
{
if (!is_open()) {
return Error::from_errno(ENOTCONN);
Expand All @@ -337,7 +337,7 @@ ErrorOr<size_t> PosixSocketHelper::read(Bytes buffer, int flags)
if (m_last_read_was_eof && m_notifier)
m_notifier->set_enabled(false);

return nread;
return buffer.trim(nread);
}

ErrorOr<size_t> PosixSocketHelper::write(ReadonlyBytes buffer)
Expand Down Expand Up @@ -554,7 +554,7 @@ ErrorOr<pid_t> LocalSocket::peer_pid() const

ErrorOr<size_t> LocalSocket::read_without_waiting(Bytes buffer)
{
return m_helper.read(buffer, MSG_DONTWAIT);
return TRY(m_helper.read(buffer, MSG_DONTWAIT)).size();
}

ErrorOr<int> LocalSocket::release_fd()
Expand Down
Loading

0 comments on commit 3b1e063

Please sign in to comment.