From accc7e752165b49a642be5008662da63efb488aa Mon Sep 17 00:00:00 2001 From: Alexander Boettcher Date: Tue, 13 Feb 2018 15:13:06 +0100 Subject: [PATCH] fs servers: handle result propagation better This patch removes the notion of partial writes from the file-system servers. Since write operations are asynchronously submitted, they are expected to succeed completely, except for I/O errors. I/O errors are propagated with the write acknowledgement but those are usually handled out of band at the client side. Partial writes must never occur because they would go undetected by clients, which usually don't wait for the completion of each single write operation. Until now, most file-system servers returned the number of written bytes in the acknowledgement packet. If a server managed to write a part of the request only, it issued the acknowledgement immediately where it should have cared about writing the remaining part first. The patch detects such misbehaving server-side code. If partial writes unexpectedly occur, it prints a message and leaves the corresponding request unacknowdleged. Issue #2672 --- repos/dde_rump/src/server/rump_fs/directory.h | 7 +- repos/dde_rump/src/server/rump_fs/main.cc | 29 ++++++-- repos/dde_rump/src/server/rump_fs/symlink.h | 4 +- repos/libports/run/libc_vfs_fs_test.inc | 4 +- repos/libports/src/server/fatfs_fs/main.cc | 73 ++++++++++++------- .../src/server/fuse_fs/fuse_fs_main.cc | 25 ++++++- repos/os/src/server/lx_fs/main.cc | 30 ++++++-- repos/os/src/server/ram_fs/main.cc | 20 ++++- repos/os/src/server/ram_fs/symlink.h | 5 +- repos/os/src/server/trace_fs/main.cc | 15 +++- repos/os/src/server/vfs/main.cc | 23 +++++- repos/os/src/server/vfs/node.h | 4 +- 12 files changed, 182 insertions(+), 57 deletions(-) diff --git a/repos/dde_rump/src/server/rump_fs/directory.h b/repos/dde_rump/src/server/rump_fs/directory.h index 68ee57307..cfd69ea6a 100644 --- a/repos/dde_rump/src/server/rump_fs/directory.h +++ b/repos/dde_rump/src/server/rump_fs/directory.h @@ -251,7 +251,12 @@ class Rump_fs::Directory : public Node void *current, *end; for (current = buf, end = &buf[bytes]; current < end; - current = _DIRENT_NEXT((dirent *)current), count++) { } + current = _DIRENT_NEXT((dirent *)current)) + { + struct ::dirent *d = (dirent*)current; + if (strcmp(".", d->d_name) && strcmp("..", d->d_name)) + count++; + } } while(bytes); return count; diff --git a/repos/dde_rump/src/server/rump_fs/main.cc b/repos/dde_rump/src/server/rump_fs/main.cc index b0e3d3545..d0d70698a 100644 --- a/repos/dde_rump/src/server/rump_fs/main.cc +++ b/repos/dde_rump/src/server/rump_fs/main.cc @@ -70,17 +70,34 @@ class Rump_fs::Session_component : public Session_rpc_object /* resulting length */ size_t res_length = 0; + bool succeeded = false; switch (packet.operation()) { case Packet_descriptor::READ: - if (content && (packet.length() <= packet.size())) - res_length = open_node.node().read((char *)content, length, packet.position()); + if (content && (packet.length() <= packet.size())) { + res_length = open_node.node().read((char *)content, length, + packet.position()); + + /* read data or EOF is a success */ + succeeded = res_length || (packet.position() >= open_node.node().status().size); + } break; case Packet_descriptor::WRITE: - if (content && (packet.length() <= packet.size())) - res_length = open_node.node().write((char const *)content, length, packet.position()); + if (content && (packet.length() <= packet.size())) { + res_length = open_node.node().write((char const *)content, + length, + packet.position()); + /* File system session can't handle partial writes */ + if (res_length != length) { + Genode::error("partial write detected ", + res_length, " vs ", length); + /* don't acknowledge */ + return; + } + succeeded = true; + } break; case Packet_descriptor::CONTENT_CHANGED: @@ -92,15 +109,17 @@ class Rump_fs::Session_component : public Session_rpc_object case Packet_descriptor::READ_READY: /* not supported */ + succeeded = true; break; case Packet_descriptor::SYNC: rump_sys_sync(); + succeeded = true; break; } packet.length(res_length); - packet.succeeded(res_length > 0); + packet.succeeded(succeeded); tx_sink()->acknowledge_packet(packet); } diff --git a/repos/dde_rump/src/server/rump_fs/symlink.h b/repos/dde_rump/src/server/rump_fs/symlink.h index 49b12001a..f9fc7b58e 100644 --- a/repos/dde_rump/src/server/rump_fs/symlink.h +++ b/repos/dde_rump/src/server/rump_fs/symlink.h @@ -47,7 +47,7 @@ class Rump_fs::Symlink : public Node Node::name(basename(path)); } - size_t write(char const *src, size_t len, seek_off_t seek_offset) override + size_t write(char const *src, size_t const len, seek_off_t seek_offset) override { /* Ideal symlink operations are atomic. */ if (!_create || seek_offset) @@ -57,7 +57,7 @@ class Rump_fs::Symlink : public Node Genode::String target(Genode::Cstring(src, len)); int ret = rump_sys_symlink(target.string(), _path.base()); - return ret == -1 ? 0 : ret; + return ret == -1 ? 0 : len; } size_t read(char *dst, size_t len, seek_off_t seek_offset) override diff --git a/repos/libports/run/libc_vfs_fs_test.inc b/repos/libports/run/libc_vfs_fs_test.inc index 36c00f7a2..8d89deaf6 100644 --- a/repos/libports/run/libc_vfs_fs_test.inc +++ b/repos/libports/run/libc_vfs_fs_test.inc @@ -94,7 +94,7 @@ append_if $use_ahci_drv config { - + } @@ -161,7 +161,7 @@ append qemu_args " -nographic " append_if $use_ahci_drv qemu_args " -drive id=disk,file=$disk_image,format=raw,if=none -device ahci,id=ahci -device ide-drive,drive=disk,bus=ahci.0 -boot d" append_if $use_sd_card_drv qemu_args " -drive file=$disk_image,format=raw,if=sd,cache=writeback " -run_genode_until {.*child "test-libc_vfs" exited with exit value 0.*} 60 +run_genode_until {.*child "test-libc_vfs" exited with exit value 0.*} 120 exec rm -f $disk_image diff --git a/repos/libports/src/server/fatfs_fs/main.cc b/repos/libports/src/server/fatfs_fs/main.cc index d47c921e7..fa404d345 100644 --- a/repos/libports/src/server/fatfs_fs/main.cc +++ b/repos/libports/src/server/fatfs_fs/main.cc @@ -82,40 +82,63 @@ class Fatfs_fs::Session_component : public Session_rpc_object size_t const length = packet.length(); seek_off_t const offset = packet.position(); - if (!content || (packet.length() > packet.size())) { - packet.succeeded(false); - return; - } - /* resulting length */ size_t res_length = 0; + bool succeeded = false; switch (packet.operation()) { - case Packet_descriptor::READ: - res_length = open_node.node().read((char *)content, length, offset); - break; + case Packet_descriptor::READ: + if (content && (packet.length() <= packet.size())) { + res_length = open_node.node().read((char *)content, length, + offset); - case Packet_descriptor::WRITE: - res_length = open_node.node().write((char const *)content, length, offset); - break; + /* read data or EOF is a success */ + succeeded = res_length > 0; + if (!succeeded) { + File * file = dynamic_cast(&open_node.node()); + if (file) + succeeded = f_eof((file->fatfs_fil())); + } + } + break; - case Packet_descriptor::CONTENT_CHANGED: - open_node.register_notify(*tx_sink()); - open_node.node().notify_listeners(); - return; + case Packet_descriptor::WRITE: + if (content && (packet.length() <= packet.size())) { + res_length = open_node.node().write((char const *)content, + length, offset); - case Packet_descriptor::READ_READY: - /* not supported */ - break; + /* File system session can't handle partial writes */ + if (res_length != length) { + Genode::error("partial write detected ", + res_length, " vs ", length); + /* don't acknowledge */ + return; + } + succeeded = true; + } + break; - case Packet_descriptor::SYNC: - /* not supported */ - break; + case Packet_descriptor::CONTENT_CHANGED: + open_node.register_notify(*tx_sink()); + open_node.node().notify_listeners(); + return; + + case Packet_descriptor::READ_READY: + succeeded = true; + /* not supported */ + break; + + case Packet_descriptor::SYNC: + succeeded = true; + /* not supported */ + break; } packet.length(res_length); - packet.succeeded(res_length > 0); + packet.succeeded(succeeded); + + tx_sink()->acknowledge_packet(packet); } void _process_packet() @@ -134,12 +157,6 @@ class Fatfs_fs::Session_component : public Session_rpc_object } catch (Id_space::Unknown_id const &) { Genode::error("Invalid_handle"); } - - /* - * The 'acknowledge_packet' function cannot block because we - * checked for 'ready_to_ack' in '_process_packets'. - */ - tx_sink()->acknowledge_packet(packet); } /** diff --git a/repos/libports/src/server/fuse_fs/fuse_fs_main.cc b/repos/libports/src/server/fuse_fs/fuse_fs_main.cc index 36d893025..bc3889ce4 100644 --- a/repos/libports/src/server/fuse_fs/fuse_fs_main.cc +++ b/repos/libports/src/server/fuse_fs/fuse_fs_main.cc @@ -72,17 +72,32 @@ class Fuse_fs::Session_component : public Session_rpc_object /* resulting length */ size_t res_length = 0; + bool succeeded = false; switch (packet.operation()) { case Packet_descriptor::READ: - if (content && (packet.length() <= packet.size())) - res_length = open_node.node().read((char *)content, length, packet.position()); + if (content && (packet.length() <= packet.size())) { + res_length = open_node.node().read((char *)content, length, + packet.position()); + + succeeded = res_length > 0; + } break; case Packet_descriptor::WRITE: - if (content && (packet.length() <= packet.size())) + if (content && (packet.length() <= packet.size())) { res_length = open_node.node().write((char const *)content, length, packet.position()); + + /* File system session can't handle partial writes */ + if (res_length != length) { + Genode::error("partial write detected ", + res_length, " vs ", length); + /* don't acknowledge */ + return; + } + succeeded = true; + } break; case Packet_descriptor::CONTENT_CHANGED: @@ -93,16 +108,18 @@ class Fuse_fs::Session_component : public Session_rpc_object return; case Packet_descriptor::READ_READY: + succeeded = true; /* not supported */ break; case Packet_descriptor::SYNC: Fuse::sync_fs(); + succeeded = true; break; } packet.length(res_length); - packet.succeeded(res_length > 0); + packet.succeeded(succeeded); tx_sink()->acknowledge_packet(packet); } diff --git a/repos/os/src/server/lx_fs/main.cc b/repos/os/src/server/lx_fs/main.cc index a464fde22..3ace25621 100644 --- a/repos/os/src/server/lx_fs/main.cc +++ b/repos/os/src/server/lx_fs/main.cc @@ -67,17 +67,35 @@ class Lx_fs::Session_component : public Session_rpc_object /* resulting length */ size_t res_length = 0; + bool succeeded = false; switch (packet.operation()) { case Packet_descriptor::READ: - if (content && (packet.length() <= packet.size())) - res_length = open_node.node().read((char *)content, length, packet.position()); + if (content && (packet.length() <= packet.size())) { + res_length = open_node.node().read((char *)content, length, + packet.position()); + + /* read data or EOF is a success */ + succeeded = res_length || (packet.position() >= open_node.node().status().size); + } break; case Packet_descriptor::WRITE: - if (content && (packet.length() <= packet.size())) - res_length = open_node.node().write((char const *)content, length, packet.position()); + if (content && (packet.length() <= packet.size())) { + res_length = open_node.node().write((char const *)content, + length, + packet.position()); + + /* File system session can't handle partial writes */ + if (res_length != length) { + Genode::error("partial write detected ", + res_length, " vs ", length); + /* don't acknowledge */ + return; + } + succeeded = true; + } break; case Packet_descriptor::CONTENT_CHANGED: @@ -88,6 +106,7 @@ class Lx_fs::Session_component : public Session_rpc_object return; case Packet_descriptor::READ_READY: + succeeded = true; /* not supported */ break; @@ -99,11 +118,12 @@ class Lx_fs::Session_component : public Session_rpc_object * use-case. */ Genode::warning("SYNC not implemented!"); + succeeded = true; break; } packet.length(res_length); - packet.succeeded(res_length > 0); + packet.succeeded(succeeded); tx_sink()->acknowledge_packet(packet); } diff --git a/repos/os/src/server/ram_fs/main.cc b/repos/os/src/server/ram_fs/main.cc index 54cf54bf0..d940c9ed7 100644 --- a/repos/os/src/server/ram_fs/main.cc +++ b/repos/os/src/server/ram_fs/main.cc @@ -72,6 +72,7 @@ class Ram_fs::Session_component : public File_system::Session_rpc_object /* resulting length */ size_t res_length = 0; + bool succeeded = false; switch (packet.operation()) { @@ -82,6 +83,9 @@ class Ram_fs::Session_component : public File_system::Session_rpc_object break; res_length = node->read((char *)content, length, packet.position()); + + /* read data or EOF is a success */ + succeeded = res_length || (packet.position() >= node->status().size); } break; @@ -92,17 +96,26 @@ class Ram_fs::Session_component : public File_system::Session_rpc_object break; res_length = node->write((char const *)content, length, packet.position()); + + /* File system session can't handle partial writes */ + if (res_length != length) { + Genode::error("partial write detected ", + res_length, " vs ", length); + /* don't acknowledge */ + return; + } + succeeded = true; } open_node.mark_as_written(); break; - case Packet_descriptor::CONTENT_CHANGED: { + case Packet_descriptor::CONTENT_CHANGED: Genode::error("CONTENT_CHANGED packets from clients have no effect"); return; - } case Packet_descriptor::READ_READY: /* not supported */ + succeeded = true; break; case Packet_descriptor::SYNC: { @@ -110,12 +123,13 @@ class Ram_fs::Session_component : public File_system::Session_rpc_object if (!node.valid()) break; node->notify_listeners(); + succeeded = true; break; } } packet.length(res_length); - packet.succeeded(res_length > 0); + packet.succeeded(succeeded); tx_sink()->acknowledge_packet(packet); } diff --git a/repos/os/src/server/ram_fs/symlink.h b/repos/os/src/server/ram_fs/symlink.h index 562686425..ef3376238 100644 --- a/repos/os/src/server/ram_fs/symlink.h +++ b/repos/os/src/server/ram_fs/symlink.h @@ -40,6 +40,8 @@ class Ram_fs::Symlink : public Node size_t write(char const *src, size_t len, seek_off_t seek_offset) override { + size_t const consumed_len = len; + /* Ideal symlink operations are atomic. */ if (seek_offset) return 0; @@ -59,7 +61,8 @@ class Ram_fs::Symlink : public Node Genode::memcpy(_link_to, src, len); _len = len; - return len; + + return consumed_len; } Status status() override diff --git a/repos/os/src/server/trace_fs/main.cc b/repos/os/src/server/trace_fs/main.cc index ae41c3582..36208de2b 100644 --- a/repos/os/src/server/trace_fs/main.cc +++ b/repos/os/src/server/trace_fs/main.cc @@ -654,6 +654,7 @@ class Trace_fs::Session_component : public Session_rpc_object /* resulting length */ size_t res_length = 0; + bool succeeded = false; switch (packet.operation()) { @@ -663,6 +664,7 @@ class Trace_fs::Session_component : public Session_rpc_object if (!node.valid()) break; res_length = node->read((char *)content, length, packet.position()); + succeeded = res_length > 0; } break; @@ -672,6 +674,15 @@ class Trace_fs::Session_component : public Session_rpc_object if (!node.valid()) break; res_length = node->write((char const *)content, length, packet.position()); + + /* File system session can't handle partial writes */ + if (res_length != length) { + Genode::error("partial write detected ", + res_length, " vs ", length); + /* don't acknowledge */ + return; + } + succeeded = true; } break; @@ -689,16 +700,18 @@ class Trace_fs::Session_component : public Session_rpc_object } case Packet_descriptor::READ_READY: + succeeded = true; /* not supported */ break; case Packet_descriptor::SYNC: + succeeded = true; /* not supported */ break; } packet.length(res_length); - packet.succeeded(res_length > 0); + packet.succeeded(succeeded); tx_sink()->acknowledge_packet(packet); } diff --git a/repos/os/src/server/vfs/main.cc b/repos/os/src/server/vfs/main.cc index d1f80d3a3..5ad026f50 100644 --- a/repos/os/src/server/vfs/main.cc +++ b/repos/os/src/server/vfs/main.cc @@ -148,6 +148,7 @@ class Vfs_server::Session_component : public File_system::Session_rpc_object, /* resulting length */ size_t res_length = 0; + bool succeeded = false; switch (packet.operation()) { @@ -160,8 +161,12 @@ class Vfs_server::Session_component : public File_system::Session_rpc_object, throw Not_ready(); } - if (node.mode() & READ_ONLY) + if (node.mode() & READ_ONLY) { res_length = node.read((char *)content, length, seek); + /* no way to distinguish EOF from unsuccessful + reads, both have res_length == 0 */ + succeeded = true; + } }); } catch (Not_ready) { throw; } @@ -174,8 +179,18 @@ class Vfs_server::Session_component : public File_system::Session_rpc_object, try { _apply(packet.handle(), [&] (Node &node) { - if (node.mode() & WRITE_ONLY) + if (node.mode() & WRITE_ONLY) { res_length = node.write((char const *)content, length, seek); + + /* File system session can't handle partial writes */ + if (res_length != length) { + Genode::error("partial write detected ", + res_length, " vs ", length); + /* don't acknowledge */ + throw Dont_ack(); + } + succeeded = true; + } }); } catch (Operation_incomplete) { throw Not_ready(); @@ -191,6 +206,7 @@ class Vfs_server::Session_component : public File_system::Session_rpc_object, throw Dont_ack(); } }); + succeeded = true; } catch (Dont_ack) { throw; } catch (...) { } @@ -208,6 +224,7 @@ class Vfs_server::Session_component : public File_system::Session_rpc_object, try { _apply(packet.handle(), [&] (Node &node) { node.sync(); + succeeded = true; }); } catch (Operation_incomplete) { throw Not_ready(); @@ -216,7 +233,7 @@ class Vfs_server::Session_component : public File_system::Session_rpc_object, } packet.length(res_length); - packet.succeeded(!!res_length); + packet.succeeded(succeeded); } bool _try_process_packet_op(Packet_descriptor &packet) diff --git a/repos/os/src/server/vfs/node.h b/repos/os/src/server/vfs/node.h index 1c54c5195..0b25ffbec 100644 --- a/repos/os/src/server/vfs/node.h +++ b/repos/os/src/server/vfs/node.h @@ -287,7 +287,7 @@ struct Vfs_server::Symlink : Node return _read(dst, len, 0); } - size_t write(char const *src, size_t len, seek_off_t) + size_t write(char const *src, size_t const len, seek_off_t) { /* * if the symlink target is too long return a short result @@ -311,7 +311,7 @@ struct Vfs_server::Symlink : Node mark_as_updated(); notify_listeners(); - return out_count; + return len; } };