[io][conformance] Return NodeInfo::Vmofile for VMO files in rust vfs This fixes a small bug in the Rust VFS's Describe method and OnOpen event for VMO files. It now correctly returns NodeInfo::Vmofile for these, instead of NodeInfo::File, which it was previously doing. Bug: 68968 Change-Id: I5a8a086be0c693a22e6eaccd614ba4ecb5286873 Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/483004 Reviewed-by: Chris Suter <csuter@google.com> Commit-Queue: Cameron Silvestrini <csilvestrini@google.com>
diff --git a/src/lib/storage/vfs/rust/src/directory/mutable/simple/tests.rs b/src/lib/storage/vfs/rust/src/directory/mutable/simple/tests.rs index 924f650..7780bdf 100644 --- a/src/lib/storage/vfs/rust/src/directory/mutable/simple/tests.rs +++ b/src/lib/storage/vfs/rust/src/directory/mutable/simple/tests.rs
@@ -17,6 +17,7 @@ assert_unlink, assert_unlink_err, assert_watch, assert_watcher_one_message_watched_events, assert_write, open_as_directory_assert_err, open_as_file_assert_err, open_get_directory_proxy_assert_ok, open_get_file_proxy_assert_ok, open_get_proxy_assert, + open_get_vmo_file_proxy_assert_ok, }; use crate::{ @@ -543,9 +544,9 @@ // `open_as_file_assert_content!` as the later will immediately close the connection, // and if no connections exist the VMO is recycled and the assertion is invoked to // check if the VMO content matches the final expected state. - let file = open_get_file_proxy_assert_ok!(&proxy, flags, "file"); + let file = open_get_vmo_file_proxy_assert_ok!(&proxy, flags, "file"); - open_as_file_assert_content!(&proxy, flags, "file", "Initial"); + open_as_vmo_file_assert_content!(&proxy, flags, "file", "Initial"); let root_token = assert_get_token!(&proxy); assert_link!(&proxy, "file", root_token, "same-file"); @@ -562,11 +563,11 @@ assert_read_dirents!(proxy, 1000, expected.into_vec()); } - let same_file = open_get_file_proxy_assert_ok!(&proxy, flags, "same-file"); + let same_file = open_get_vmo_file_proxy_assert_ok!(&proxy, flags, "same-file"); assert_write!(file, "------- twice"); assert_write!(same_file, "Updated"); - open_as_file_assert_content!(&proxy, flags, "file", "Updated twice"); + open_as_vmo_file_assert_content!(&proxy, flags, "file", "Updated twice"); drop(watcher_client); assert_close!(file); @@ -612,9 +613,9 @@ // `open_as_file_assert_content!` as the later will immediately close the connection, // and if no connections exist the VMO is recycled and the assertion is invoked to // check if the VMO content matches the final expected state. - let passwd = open_get_file_proxy_assert_ok!(&etc, flags, "passwd"); + let passwd = open_get_vmo_file_proxy_assert_ok!(&etc, flags, "passwd"); - open_as_file_assert_content!(&etc, flags, "passwd", "secret"); + open_as_vmo_file_assert_content!(&etc, flags, "passwd", "secret"); let tmp_token = assert_get_token!(&tmp); assert_link!(&etc, "passwd", tmp_token, "linked-passwd"); @@ -638,11 +639,11 @@ assert_read_dirents!(tmp, 1000, expected.into_vec()); } - let linked_passwd = open_get_file_proxy_assert_ok!(&tmp, flags, "linked-passwd"); + let linked_passwd = open_get_vmo_file_proxy_assert_ok!(&tmp, flags, "linked-passwd"); assert_write!(passwd, "-------- twice"); assert_write!(linked_passwd, "modified"); - open_as_file_assert_content!(&etc, flags, "passwd", "modified twice"); + open_as_vmo_file_assert_content!(&etc, flags, "passwd", "modified twice"); drop(tmp_watcher_client); assert_close!(passwd); @@ -740,9 +741,9 @@ // `open_as_file_assert_content!` as the later will immediately close the connection, // and if no connections exist the VMO is recycled and the assertion is invoked to // check if the VMO content matches the final expected state. - let passwd = open_get_file_proxy_assert_ok!(&etc, flags, "passwd"); + let passwd = open_get_vmo_file_proxy_assert_ok!(&etc, flags, "passwd"); - open_as_file_assert_content!(&etc, flags, "passwd", "secret"); + open_as_vmo_file_assert_content!(&etc, flags, "passwd", "secret"); let tmp_token = assert_get_token!(&tmp); assert_link!(&etc, "passwd", tmp_token, "linked-passwd"); @@ -766,11 +767,11 @@ assert_read_dirents!(tmp, 1000, expected.into_vec()); } - let linked_passwd = open_get_file_proxy_assert_ok!(&tmp, flags, "linked-passwd"); + let linked_passwd = open_get_vmo_file_proxy_assert_ok!(&tmp, flags, "linked-passwd"); assert_write!(passwd, "-------- twice"); assert_write!(linked_passwd, "modified"); - open_as_file_assert_content!(&etc, flags, "passwd", "modified twice"); + open_as_vmo_file_assert_content!(&etc, flags, "passwd", "modified twice"); drop(tmp_watcher_client); assert_close!(passwd);
diff --git a/src/lib/storage/vfs/rust/src/directory/test_utils.rs b/src/lib/storage/vfs/rust/src/directory/test_utils.rs index c2ff7f2..d0aa0fd 100644 --- a/src/lib/storage/vfs/rust/src/directory/test_utils.rs +++ b/src/lib/storage/vfs/rust/src/directory/test_utils.rs
@@ -120,6 +120,17 @@ }}; } +/// Opens the specified path as a VMO file and checks its content. Also see all the `assert_*` +/// macros in `../test_utils.rs`. +#[macro_export] +macro_rules! open_as_vmo_file_assert_content { + ($proxy:expr, $flags:expr, $path:expr, $expected_content:expr) => {{ + let file = open_get_vmo_file_proxy_assert_ok!($proxy, $flags, $path); + assert_read!(file, $expected_content); + assert_close!(file); + }}; +} + #[macro_export] macro_rules! assert_watch { ($proxy:expr, $mask:expr) => {{
diff --git a/src/lib/storage/vfs/rust/src/file/vmo/asynchronous/tests.rs b/src/lib/storage/vfs/rust/src/file/vmo/asynchronous/tests.rs index 4986ff7..791c00e 100644 --- a/src/lib/storage/vfs/rust/src/file/vmo/asynchronous/tests.rs +++ b/src/lib/storage/vfs/rust/src/file/vmo/asynchronous/tests.rs
@@ -13,7 +13,7 @@ assert_read_fidl_err_closed, assert_seek, assert_seek_err, assert_truncate, assert_truncate_err, assert_vmo_content, assert_write, assert_write_at, assert_write_at_err, assert_write_err, assert_write_fidl_err_closed, clone_as_file_assert_err, - clone_get_file_proxy_assert_ok, clone_get_proxy_assert, report_invalid_vmo_content, + clone_get_proxy_assert, clone_get_vmo_file_proxy_assert_ok, report_invalid_vmo_content, }; use crate::{ @@ -33,7 +33,7 @@ use { fidl::endpoints::create_proxy, fidl_fuchsia_io::{ - FileEvent, FileMarker, FileObject, NodeAttributes, NodeInfo, INO_UNKNOWN, MODE_TYPE_FILE, + FileEvent, FileMarker, NodeAttributes, NodeInfo, Vmofile, INO_UNKNOWN, MODE_TYPE_FILE, OPEN_FLAG_DESCRIBE, OPEN_FLAG_NODE_REFERENCE, OPEN_FLAG_POSIX, OPEN_FLAG_TRUNCATE, OPEN_RIGHT_READABLE, OPEN_RIGHT_WRITABLE, VMO_FLAG_EXACT, VMO_FLAG_PRIVATE, VMO_FLAG_READ, VMO_FLAG_WRITE, @@ -111,10 +111,8 @@ assert_event!(proxy, FileEvent::OnOpen_ { s, info }, { assert_eq!(s, ZX_OK); - assert_eq!( - info, - Some(Box::new(NodeInfo::File(FileObject { event: None, stream: None }))) - ); + let info = *info.expect("Empty NodeInfo"); + assert!(matches!(info, NodeInfo::Vmofile(Vmofile { vmo: _, offset: 0, length: 14 }))); }); }); } @@ -256,10 +254,11 @@ assert_event!(proxy, FileEvent::OnOpen_ { s, info }, { assert_eq!(s, ZX_OK); - assert_eq!( + let info = *info.expect("Empty NodeInfo"); + assert!(matches!( info, - Some(Box::new(NodeInfo::File(FileObject { event: None, stream: None }))) - ); + NodeInfo::Vmofile(Vmofile { vmo: _, offset: 0, length: 10 }) + )); }); assert_read!(proxy, "Have value"); @@ -744,7 +743,7 @@ assert_seek!(first_proxy, 0, Start); assert_write!(first_proxy, "As updated"); - let second_proxy = clone_get_file_proxy_assert_ok!( + let second_proxy = clone_get_vmo_file_proxy_assert_ok!( &first_proxy, OPEN_RIGHT_READABLE | OPEN_FLAG_DESCRIBE ); @@ -771,7 +770,7 @@ assert_seek!(first_proxy, 0, Start); assert_write!(first_proxy, "As updated"); - let second_proxy = clone_get_file_proxy_assert_ok!( + let second_proxy = clone_get_vmo_file_proxy_assert_ok!( &first_proxy, CLONE_FLAG_SAME_RIGHTS | OPEN_FLAG_DESCRIBE ); @@ -917,7 +916,8 @@ assert_write_fidl_err_closed!(second_proxy, "Write attempt"); // We now try without OPEN_RIGHT_READABLE, as we might still be able to Seek. - let third_proxy = clone_get_file_proxy_assert_ok!(&first_proxy, OPEN_FLAG_DESCRIBE); + let third_proxy = + clone_get_vmo_file_proxy_assert_ok!(&first_proxy, OPEN_FLAG_DESCRIBE); assert_seek_err!(third_proxy, 0, Current, Status::BAD_HANDLE, 0);
diff --git a/src/lib/storage/vfs/rust/src/file/vmo/connection/io1.rs b/src/lib/storage/vfs/rust/src/file/vmo/connection/io1.rs index b2fefaa..e56c3b4 100644 --- a/src/lib/storage/vfs/rust/src/file/vmo/connection/io1.rs +++ b/src/lib/storage/vfs/rust/src/file/vmo/connection/io1.rs
@@ -21,8 +21,8 @@ anyhow::Error, fidl::endpoints::{RequestStream, ServerEnd}, fidl_fuchsia_io::{ - FileObject, FileRequest, FileRequestStream, NodeAttributes, NodeInfo, NodeMarker, - SeekOrigin, INO_UNKNOWN, MODE_TYPE_FILE, OPEN_FLAG_DESCRIBE, OPEN_FLAG_NODE_REFERENCE, + FileRequest, FileRequestStream, NodeAttributes, NodeInfo, NodeMarker, SeekOrigin, Vmofile, + INO_UNKNOWN, MODE_TYPE_FILE, OPEN_FLAG_DESCRIBE, OPEN_FLAG_NODE_REFERENCE, OPEN_FLAG_TRUNCATE, OPEN_RIGHT_READABLE, OPEN_RIGHT_WRITABLE, VMO_FLAG_EXACT, VMO_FLAG_EXEC, VMO_FLAG_PRIVATE, VMO_FLAG_READ, VMO_FLAG_WRITE, }, @@ -199,15 +199,7 @@ } }; - if flags & OPEN_FLAG_DESCRIBE != 0 { - let mut info = NodeInfo::File(FileObject { event: None, stream: None }); - match control_handle.send_on_open_(Status::OK.into_raw(), Some(&mut info)) { - Ok(()) => (), - Err(_) => return, - } - } - - let handle_requests = FileConnection { + let mut connection = FileConnection { scope: scope.clone(), file, requests, @@ -215,9 +207,26 @@ readable, writable, seek: 0, + }; + + if flags & OPEN_FLAG_DESCRIBE != 0 { + match connection.get_node_info().await { + Ok(mut info) => { + let send_result = + control_handle.send_on_open_(Status::OK.into_raw(), Some(&mut info)); + if send_result.is_err() { + return; + } + } + Err(status) => { + debug_assert!(status != Status::OK); + control_handle.shutdown_with_epitaph(status); + return; + } + } } - .handle_requests(); - handle_requests.await; + + connection.handle_requests().await; } async fn ensure_vmo<'state_guard>( @@ -366,6 +375,19 @@ } } + /// Returns `NodeInfo` for the VMO file. + async fn get_node_info(&mut self) -> Result<NodeInfo, Status> { + let vmofile = update_initialized_state! { + match &*self.file.state().await; + error: "get_node_info" => Err(Status::INTERNAL); + { vmo, size, .. } => { + let vmo = vmo.duplicate_handle(Rights::SAME_RIGHTS).unwrap(); + Ok(Vmofile {vmo, offset: 0, length: *size}) + } + }?; + Ok(NodeInfo::Vmofile(vmofile)) + } + /// Handle a [`FileRequest`]. This function is responsible for handing all the file operations /// that operate on the connection-specific buffer. async fn handle_request(&mut self, req: FileRequest) -> Result<ConnectionState, Error> { @@ -381,10 +403,13 @@ let _ = self.handle_close(|status| responder.send(status.into_raw())).await; return Ok(ConnectionState::Closed); } - FileRequest::Describe { responder } => { - let mut info = NodeInfo::File(FileObject { event: None, stream: None }); - responder.send(&mut info)?; - } + FileRequest::Describe { responder } => match self.get_node_info().await { + Ok(mut info) => responder.send(&mut info)?, + Err(status) => { + debug_assert!(status != Status::OK); + responder.control_handle().shutdown_with_epitaph(status); + } + }, FileRequest::Sync { responder } => { // VMOs are always in sync. responder.send(ZX_OK)?;
diff --git a/src/lib/storage/vfs/rust/src/test_utils/assertions.rs b/src/lib/storage/vfs/rust/src/test_utils/assertions.rs index 4c447cc..d119ac3 100644 --- a/src/lib/storage/vfs/rust/src/test_utils/assertions.rs +++ b/src/lib/storage/vfs/rust/src/test_utils/assertions.rs
@@ -10,10 +10,10 @@ crate::directory::test_utils::DirentsSameInodeBuilder, fidl_fuchsia_io::{ DirectoryEvent, DirectoryMarker, DirectoryObject, FileEvent, FileMarker, FileObject, - NodeInfo, SeekOrigin, Service, DIRENT_TYPE_BLOCK_DEVICE, DIRENT_TYPE_DIRECTORY, - DIRENT_TYPE_FILE, DIRENT_TYPE_SERVICE, DIRENT_TYPE_SOCKET, DIRENT_TYPE_UNKNOWN, - INO_UNKNOWN, OPEN_FLAG_DESCRIBE, OPEN_RIGHT_READABLE, WATCH_EVENT_ADDED, - WATCH_EVENT_EXISTING, WATCH_EVENT_IDLE, WATCH_EVENT_REMOVED, + NodeInfo, SeekOrigin, Service, Vmofile, DIRENT_TYPE_BLOCK_DEVICE, + DIRENT_TYPE_DIRECTORY, DIRENT_TYPE_FILE, DIRENT_TYPE_SERVICE, DIRENT_TYPE_SOCKET, + DIRENT_TYPE_UNKNOWN, INO_UNKNOWN, OPEN_FLAG_DESCRIBE, OPEN_RIGHT_READABLE, + WATCH_EVENT_ADDED, WATCH_EVENT_EXISTING, WATCH_EVENT_IDLE, WATCH_EVENT_REMOVED, }, fuchsia_zircon::{MessageBuf, Status}, futures::stream::StreamExt, @@ -370,6 +370,25 @@ // See comment at the top of the file for why this is a macro. #[macro_export] +macro_rules! open_get_vmo_file_proxy_assert_ok { + ($proxy:expr, $flags:expr, $path:expr) => {{ + use $crate::test_utils::assertions::reexport::{ + FileEvent, FileMarker, NodeInfo, Status, Vmofile, + }; + + open_get_proxy_assert!($proxy, $flags, $path, FileMarker, FileEvent::OnOpen_ { s, info }, { + assert_eq!(Status::from_raw(s), Status::OK); + let info = *info.expect("Empty NodeInfo"); + assert!( + matches!(info, NodeInfo::Vmofile(Vmofile { .. })), + format!("Expected Vmofile but got {:?}", info) + ); + }) + }}; +} + +// See comment at the top of the file for why this is a macro. +#[macro_export] macro_rules! open_as_file_assert_err { ($proxy:expr, $flags:expr, $path:expr, $expected_status:expr) => {{ use $crate::test_utils::assertions::reexport::{FileEvent, FileMarker, Status}; @@ -461,6 +480,25 @@ // See comment at the top of the file for why this is a macro. #[macro_export] +macro_rules! clone_get_vmo_file_proxy_assert_ok { + ($proxy:expr, $flags:expr) => {{ + use $crate::test_utils::assertions::reexport::{ + FileEvent, FileMarker, NodeInfo, Status, Vmofile, + }; + + clone_get_proxy_assert!($proxy, $flags, FileMarker, FileEvent::OnOpen_ { s, info }, { + assert_eq!(Status::from_raw(s), Status::OK); + let info = *info.expect("Empty NodeInfo"); + assert!( + matches!(info, NodeInfo::Vmofile(Vmofile { .. })), + format!("Expected Vmofile but got {:?}", info) + ); + }) + }}; +} + +// See comment at the top of the file for why this is a macro. +#[macro_export] macro_rules! clone_as_file_assert_err { ($proxy:expr, $flags:expr, $expected_status:expr) => {{ use $crate::test_utils::assertions::reexport::{FileEvent, FileMarker, Status};