From 72e82ffcb11b1aaf1f1cc8db4189ced5ec0aa42e Mon Sep 17 00:00:00 2001 From: Florian Klink Date: Tue, 18 Jul 2023 19:37:25 +0300 Subject: [PATCH] refactor(tvix/store): use bytes for node names and symlink targets Some paths might use names that are not valid UTF-8. We should be able to represent them. We don't actually need to touch the PathInfo structures, as they need to represent StorePaths, which come with their own harder restrictions, which can't encode non-UTF8 data. While this doesn't change any of the wire format of the gRPC messages, it does however change the interface of tvix_eval::EvalIO - its read_dir() method does now return a list of Vec, rather than SmolStr. Maybe this should be OsString instead? Change-Id: I821016d9a58ec441ee081b0b9f01c9240723af0b Reviewed-on: https://cl.tvl.fyi/c/depot/+/8974 Autosubmit: flokli Reviewed-by: raitobezarius Tested-by: BuildkiteCI --- tvix/cli/src/nix_compat.rs | 3 +- tvix/cli/src/tvix_io.rs | 3 +- tvix/eval/src/builtins/impure.rs | 3 +- tvix/eval/src/io.rs | 13 +-- tvix/eval/src/vm/generators.rs | 5 +- tvix/nix-compat/src/nar/writer/mod.rs | 27 +++--- tvix/store/protos/castore.pb.go | 32 +++---- tvix/store/protos/castore.proto | 8 +- tvix/store/src/bin/tvix-store.rs | 8 +- tvix/store/src/directoryservice/traverse.rs | 10 +-- tvix/store/src/fuse/inode_tracker.rs | 13 ++- tvix/store/src/fuse/inodes.rs | 2 +- tvix/store/src/fuse/mod.rs | 11 ++- tvix/store/src/fuse/tests.rs | 20 ++--- tvix/store/src/import.rs | 34 ++------ tvix/store/src/nar/mod.rs | 12 +-- .../proto/grpc_directoryservice_wrapper.rs | 4 +- tvix/store/src/proto/mod.rs | 83 ++++++++++--------- tvix/store/src/proto/tests/directory.rs | 60 +++++++------- .../proto/tests/directory_nodes_iterator.rs | 34 ++++---- .../src/proto/tests/grpc_directoryservice.rs | 6 +- .../src/proto/tests/grpc_pathinfoservice.rs | 4 +- tvix/store/src/proto/tests/pathinfo.rs | 24 +++--- tvix/store/src/store_io.rs | 30 ++++--- tvix/store/src/tests/fixtures.rs | 16 ++-- tvix/store/src/tests/import.rs | 17 ++-- tvix/store/src/tests/nar_renderer.rs | 16 ++-- 27 files changed, 245 insertions(+), 253 deletions(-) diff --git a/tvix/cli/src/nix_compat.rs b/tvix/cli/src/nix_compat.rs index bda238983..dbb67a9a1 100644 --- a/tvix/cli/src/nix_compat.rs +++ b/tvix/cli/src/nix_compat.rs @@ -11,7 +11,6 @@ use std::process::Command; use std::sync::RwLock; use std::{io, path::PathBuf}; -use smol_str::SmolStr; use tvix_eval::{EvalIO, FileType, StdIO}; /// Compatibility implementation of [`EvalIO`] that uses C++ Nix to @@ -78,7 +77,7 @@ impl EvalIO for NixCompatIO { self.underlying.read_to_string(path) } - fn read_dir(&self, path: &Path) -> Result, io::Error> { + fn read_dir(&self, path: &Path) -> Result, FileType)>, io::Error> { self.underlying.read_dir(path) } } diff --git a/tvix/cli/src/tvix_io.rs b/tvix/cli/src/tvix_io.rs index 8ca660f87..387a77b91 100644 --- a/tvix/cli/src/tvix_io.rs +++ b/tvix/cli/src/tvix_io.rs @@ -9,7 +9,6 @@ //! calculation will not work. use crate::KnownPaths; -use smol_str::SmolStr; use std::cell::RefCell; use std::io; use std::path::{Path, PathBuf}; @@ -73,7 +72,7 @@ impl EvalIO for TvixIO { self.actual.read_to_string(path) } - fn read_dir(&self, path: &Path) -> Result, io::Error> { + fn read_dir(&self, path: &Path) -> Result, FileType)>, io::Error> { self.actual.read_dir(path) } } diff --git a/tvix/eval/src/builtins/impure.rs b/tvix/eval/src/builtins/impure.rs index cf3186ce5..e01c642e0 100644 --- a/tvix/eval/src/builtins/impure.rs +++ b/tvix/eval/src/builtins/impure.rs @@ -38,7 +38,8 @@ mod impure_builtins { let dir = generators::request_read_dir(&co, path).await; let res = dir.into_iter().map(|(name, ftype)| { ( - NixString::from(name.as_str()), + // TODO: propagate Vec into NixString. + NixString::from(String::from_utf8(name).expect("parsing file name as string")), Value::String( match ftype { FileType::Directory => "directory", diff --git a/tvix/eval/src/io.rs b/tvix/eval/src/io.rs index 708c36153..de3da9ebe 100644 --- a/tvix/eval/src/io.rs +++ b/tvix/eval/src/io.rs @@ -15,12 +15,14 @@ //! In the context of Nix builds, callers also use this interface to determine //! how store paths are opened and so on. -use smol_str::SmolStr; use std::{ io, path::{Path, PathBuf}, }; +#[cfg(target_family = "unix")] +use std::os::unix::ffi::OsStringExt; + /// Types of files as represented by `builtins.readDir` in Nix. #[derive(Debug)] pub enum FileType { @@ -40,7 +42,7 @@ pub trait EvalIO { /// Read the directory at the specified path and return the names /// of its entries associated with their [`FileType`]. - fn read_dir(&self, path: &Path) -> Result, io::Error>; + fn read_dir(&self, path: &Path) -> Result, FileType)>, io::Error>; /// Import the given path. What this means depends on the /// implementation, for example for a `std::io`-based @@ -63,6 +65,7 @@ pub trait EvalIO { #[cfg(feature = "impure")] pub struct StdIO; +// TODO: we might want to make this whole impl to be target_family = "unix". #[cfg(feature = "impure")] impl EvalIO for StdIO { fn path_exists(&self, path: &Path) -> Result { @@ -73,7 +76,7 @@ impl EvalIO for StdIO { std::fs::read_to_string(&path) } - fn read_dir(&self, path: &Path) -> Result, io::Error> { + fn read_dir(&self, path: &Path) -> Result, FileType)>, io::Error> { let mut result = vec![]; for entry in path.read_dir()? { @@ -90,7 +93,7 @@ impl EvalIO for StdIO { FileType::Unknown }; - result.push((SmolStr::new(entry.file_name().to_string_lossy()), val)); + result.push((entry.file_name().into_vec(), val)) } Ok(result) @@ -122,7 +125,7 @@ impl EvalIO for DummyIO { )) } - fn read_dir(&self, _: &Path) -> Result, io::Error> { + fn read_dir(&self, _: &Path) -> Result, FileType)>, io::Error> { Err(io::Error::new( io::ErrorKind::Unsupported, "I/O methods are not implemented in DummyIO", diff --git a/tvix/eval/src/vm/generators.rs b/tvix/eval/src/vm/generators.rs index fb60a45f2..3437136bd 100644 --- a/tvix/eval/src/vm/generators.rs +++ b/tvix/eval/src/vm/generators.rs @@ -10,7 +10,6 @@ use core::pin::Pin; use genawaiter::rc::Co; pub use genawaiter::rc::Gen; -use smol_str::SmolStr; use std::fmt::Display; use std::future::Future; @@ -187,7 +186,7 @@ pub enum VMResponse { Path(PathBuf), /// VM response with the contents of a directory. - Directory(Vec<(SmolStr, FileType)>), + Directory(Vec<(Vec, FileType)>), /// VM response with a span to use at the current point. Span(LightSpan), @@ -736,7 +735,7 @@ pub(crate) async fn request_path_exists(co: &GenCo, path: PathBuf) -> Value { } } -pub(crate) async fn request_read_dir(co: &GenCo, path: PathBuf) -> Vec<(SmolStr, FileType)> { +pub(crate) async fn request_read_dir(co: &GenCo, path: PathBuf) -> Vec<(Vec, FileType)> { match co.yield_(VMRequest::ReadDir(path)).await { VMResponse::Directory(dir) => dir, msg => panic!( diff --git a/tvix/nix-compat/src/nar/writer/mod.rs b/tvix/nix-compat/src/nar/writer/mod.rs index f24b69883..aafb26c86 100644 --- a/tvix/nix-compat/src/nar/writer/mod.rs +++ b/tvix/nix-compat/src/nar/writer/mod.rs @@ -67,21 +67,21 @@ impl<'a, 'w> Node<'a, 'w> { } /// Make this node a symlink. - pub fn symlink(mut self, target: &str) -> io::Result<()> { + pub fn symlink(mut self, target: &[u8]) -> io::Result<()> { debug_assert!( target.len() <= wire::MAX_TARGET_LEN, "target.len() > {}", wire::MAX_TARGET_LEN ); debug_assert!( - !target.contains('\0'), + !target.contains(&b'\0'), "invalid target characters: {target:?}" ); debug_assert!(!target.is_empty(), "empty target"); self.write(&wire::TOK_SYM)?; self.write(&target.len().to_le_bytes())?; - self.write(target.as_bytes())?; + self.write(target)?; self.pad(target.len() as u64)?; self.write(&wire::TOK_PAR)?; Ok(()) @@ -136,11 +136,11 @@ impl<'a, 'w> Node<'a, 'w> { } #[cfg(debug_assertions)] -type Name = String; +type Name = Vec; #[cfg(not(debug_assertions))] type Name = (); -fn into_name(_name: &str) -> Name { +fn into_name(_name: &[u8]) -> Name { #[cfg(debug_assertions)] _name.to_owned() } @@ -163,17 +163,18 @@ impl<'a, 'w> Directory<'a, 'w> { /// /// The entry is simply another [`Node`], which can then be filled like the /// root of a NAR (including, of course, by nesting directories). - pub fn entry(&mut self, name: &str) -> io::Result> { + pub fn entry(&mut self, name: &[u8]) -> io::Result> { debug_assert!( name.len() <= wire::MAX_NAME_LEN, "name.len() > {}", wire::MAX_NAME_LEN ); - debug_assert!(!["", ".", ".."].contains(&name), "invalid name: {name:?}"); - debug_assert!( - !name.contains(['/', '\0']), - "invalid name characters: {name:?}" - ); + debug_assert!(name != b"", "name may not be empty"); + debug_assert!(name != b".", "invalid name: {name:?}"); + debug_assert!(name != b"..", "invalid name: {name:?}"); + + debug_assert!(!name.contains(&b'/'), "invalid name characters: {name:?}"); + debug_assert!(!name.contains(&b'\0'), "invalid name characters: {name:?}"); match self.prev_name { None => { @@ -187,7 +188,7 @@ impl<'a, 'w> Directory<'a, 'w> { "misordered names: {_prev_name:?} >= {name:?}" ); _prev_name.clear(); - _prev_name.push_str(name); + _prev_name.append(&mut name.to_vec()); } self.node.write(&wire::TOK_PAR)?; } @@ -195,7 +196,7 @@ impl<'a, 'w> Directory<'a, 'w> { self.node.write(&wire::TOK_ENT)?; self.node.write(&name.len().to_le_bytes())?; - self.node.write(name.as_bytes())?; + self.node.write(name)?; self.node.pad(name.len() as u64)?; self.node.write(&wire::TOK_NOD)?; diff --git a/tvix/store/protos/castore.pb.go b/tvix/store/protos/castore.pb.go index e96f115d5..074b39d54 100644 --- a/tvix/store/protos/castore.pb.go +++ b/tvix/store/protos/castore.pb.go @@ -103,7 +103,7 @@ type DirectoryNode struct { unknownFields protoimpl.UnknownFields // The (base)name of the directory - Name string `protobuf:"bytes,1,opt,name=name,proto3" json:"name,omitempty"` + Name []byte `protobuf:"bytes,1,opt,name=name,proto3" json:"name,omitempty"` // The blake3 hash of a Directory message, serialized in protobuf canonical form. Digest []byte `protobuf:"bytes,2,opt,name=digest,proto3" json:"digest,omitempty"` // Number of child elements in the Directory referred to by `digest`. @@ -151,11 +151,11 @@ func (*DirectoryNode) Descriptor() ([]byte, []int) { return file_tvix_store_protos_castore_proto_rawDescGZIP(), []int{1} } -func (x *DirectoryNode) GetName() string { +func (x *DirectoryNode) GetName() []byte { if x != nil { return x.Name } - return "" + return nil } func (x *DirectoryNode) GetDigest() []byte { @@ -179,7 +179,7 @@ type FileNode struct { unknownFields protoimpl.UnknownFields // The (base)name of the file - Name string `protobuf:"bytes,1,opt,name=name,proto3" json:"name,omitempty"` + Name []byte `protobuf:"bytes,1,opt,name=name,proto3" json:"name,omitempty"` // The blake3 digest of the file contents Digest []byte `protobuf:"bytes,2,opt,name=digest,proto3" json:"digest,omitempty"` // The file content size @@ -220,11 +220,11 @@ func (*FileNode) Descriptor() ([]byte, []int) { return file_tvix_store_protos_castore_proto_rawDescGZIP(), []int{2} } -func (x *FileNode) GetName() string { +func (x *FileNode) GetName() []byte { if x != nil { return x.Name } - return "" + return nil } func (x *FileNode) GetDigest() []byte { @@ -255,9 +255,9 @@ type SymlinkNode struct { unknownFields protoimpl.UnknownFields // The (base)name of the symlink - Name string `protobuf:"bytes,1,opt,name=name,proto3" json:"name,omitempty"` + Name []byte `protobuf:"bytes,1,opt,name=name,proto3" json:"name,omitempty"` // The target of the symlink. - Target string `protobuf:"bytes,2,opt,name=target,proto3" json:"target,omitempty"` + Target []byte `protobuf:"bytes,2,opt,name=target,proto3" json:"target,omitempty"` } func (x *SymlinkNode) Reset() { @@ -292,18 +292,18 @@ func (*SymlinkNode) Descriptor() ([]byte, []int) { return file_tvix_store_protos_castore_proto_rawDescGZIP(), []int{3} } -func (x *SymlinkNode) GetName() string { +func (x *SymlinkNode) GetName() []byte { if x != nil { return x.Name } - return "" + return nil } -func (x *SymlinkNode) GetTarget() string { +func (x *SymlinkNode) GetTarget() []byte { if x != nil { return x.Target } - return "" + return nil } var File_tvix_store_protos_castore_proto protoreflect.FileDescriptor @@ -325,20 +325,20 @@ var file_tvix_store_protos_castore_proto_rawDesc = []byte{ 0x53, 0x79, 0x6d, 0x6c, 0x69, 0x6e, 0x6b, 0x4e, 0x6f, 0x64, 0x65, 0x52, 0x08, 0x73, 0x79, 0x6d, 0x6c, 0x69, 0x6e, 0x6b, 0x73, 0x22, 0x4f, 0x0a, 0x0d, 0x44, 0x69, 0x72, 0x65, 0x63, 0x74, 0x6f, 0x72, 0x79, 0x4e, 0x6f, 0x64, 0x65, 0x12, 0x12, 0x0a, 0x04, 0x6e, 0x61, 0x6d, 0x65, 0x18, 0x01, - 0x20, 0x01, 0x28, 0x09, 0x52, 0x04, 0x6e, 0x61, 0x6d, 0x65, 0x12, 0x16, 0x0a, 0x06, 0x64, 0x69, + 0x20, 0x01, 0x28, 0x0c, 0x52, 0x04, 0x6e, 0x61, 0x6d, 0x65, 0x12, 0x16, 0x0a, 0x06, 0x64, 0x69, 0x67, 0x65, 0x73, 0x74, 0x18, 0x02, 0x20, 0x01, 0x28, 0x0c, 0x52, 0x06, 0x64, 0x69, 0x67, 0x65, 0x73, 0x74, 0x12, 0x12, 0x0a, 0x04, 0x73, 0x69, 0x7a, 0x65, 0x18, 0x03, 0x20, 0x01, 0x28, 0x0d, 0x52, 0x04, 0x73, 0x69, 0x7a, 0x65, 0x22, 0x6a, 0x0a, 0x08, 0x46, 0x69, 0x6c, 0x65, 0x4e, 0x6f, - 0x64, 0x65, 0x12, 0x12, 0x0a, 0x04, 0x6e, 0x61, 0x6d, 0x65, 0x18, 0x01, 0x20, 0x01, 0x28, 0x09, + 0x64, 0x65, 0x12, 0x12, 0x0a, 0x04, 0x6e, 0x61, 0x6d, 0x65, 0x18, 0x01, 0x20, 0x01, 0x28, 0x0c, 0x52, 0x04, 0x6e, 0x61, 0x6d, 0x65, 0x12, 0x16, 0x0a, 0x06, 0x64, 0x69, 0x67, 0x65, 0x73, 0x74, 0x18, 0x02, 0x20, 0x01, 0x28, 0x0c, 0x52, 0x06, 0x64, 0x69, 0x67, 0x65, 0x73, 0x74, 0x12, 0x12, 0x0a, 0x04, 0x73, 0x69, 0x7a, 0x65, 0x18, 0x03, 0x20, 0x01, 0x28, 0x0d, 0x52, 0x04, 0x73, 0x69, 0x7a, 0x65, 0x12, 0x1e, 0x0a, 0x0a, 0x65, 0x78, 0x65, 0x63, 0x75, 0x74, 0x61, 0x62, 0x6c, 0x65, 0x18, 0x04, 0x20, 0x01, 0x28, 0x08, 0x52, 0x0a, 0x65, 0x78, 0x65, 0x63, 0x75, 0x74, 0x61, 0x62, 0x6c, 0x65, 0x22, 0x39, 0x0a, 0x0b, 0x53, 0x79, 0x6d, 0x6c, 0x69, 0x6e, 0x6b, 0x4e, 0x6f, 0x64, - 0x65, 0x12, 0x12, 0x0a, 0x04, 0x6e, 0x61, 0x6d, 0x65, 0x18, 0x01, 0x20, 0x01, 0x28, 0x09, 0x52, + 0x65, 0x12, 0x12, 0x0a, 0x04, 0x6e, 0x61, 0x6d, 0x65, 0x18, 0x01, 0x20, 0x01, 0x28, 0x0c, 0x52, 0x04, 0x6e, 0x61, 0x6d, 0x65, 0x12, 0x16, 0x0a, 0x06, 0x74, 0x61, 0x72, 0x67, 0x65, 0x74, 0x18, - 0x02, 0x20, 0x01, 0x28, 0x09, 0x52, 0x06, 0x74, 0x61, 0x72, 0x67, 0x65, 0x74, 0x42, 0x28, 0x5a, + 0x02, 0x20, 0x01, 0x28, 0x0c, 0x52, 0x06, 0x74, 0x61, 0x72, 0x67, 0x65, 0x74, 0x42, 0x28, 0x5a, 0x26, 0x63, 0x6f, 0x64, 0x65, 0x2e, 0x74, 0x76, 0x6c, 0x2e, 0x66, 0x79, 0x69, 0x2f, 0x74, 0x76, 0x69, 0x78, 0x2f, 0x73, 0x74, 0x6f, 0x72, 0x65, 0x2f, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x73, 0x3b, 0x73, 0x74, 0x6f, 0x72, 0x65, 0x76, 0x31, 0x62, 0x06, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x33, diff --git a/tvix/store/protos/castore.proto b/tvix/store/protos/castore.proto index 747aab08b..347815107 100644 --- a/tvix/store/protos/castore.proto +++ b/tvix/store/protos/castore.proto @@ -25,7 +25,7 @@ message Directory { // A DirectoryNode represents a directory in a Directory. message DirectoryNode { // The (base)name of the directory - string name = 1; + bytes name = 1; // The blake3 hash of a Directory message, serialized in protobuf canonical form. bytes digest = 2; // Number of child elements in the Directory referred to by `digest`. @@ -44,7 +44,7 @@ message DirectoryNode { // A FileNode represents a regular or executable file in a Directory. message FileNode { // The (base)name of the file - string name = 1; + bytes name = 1; // The blake3 digest of the file contents bytes digest = 2; // The file content size @@ -56,7 +56,7 @@ message FileNode { // A SymlinkNode represents a symbolic link in a Directory. message SymlinkNode { // The (base)name of the symlink - string name = 1; + bytes name = 1; // The target of the symlink. - string target = 2; + bytes target = 2; } diff --git a/tvix/store/src/bin/tvix-store.rs b/tvix/store/src/bin/tvix-store.rs index 054b4f957..fa5b9bfca 100644 --- a/tvix/store/src/bin/tvix-store.rs +++ b/tvix/store/src/bin/tvix-store.rs @@ -234,7 +234,7 @@ fn print_node(node: &Node, path: &Path) { Node::Directory(directory_node) => { info!( path = ?path, - name = directory_node.name, + name = ?directory_node.name, digest = BASE64.encode(&directory_node.digest), "import successful", ) @@ -242,7 +242,7 @@ fn print_node(node: &Node, path: &Path) { Node::File(file_node) => { info!( path = ?path, - name = file_node.name, + name = ?file_node.name, digest = BASE64.encode(&file_node.digest), "import successful" ) @@ -250,8 +250,8 @@ fn print_node(node: &Node, path: &Path) { Node::Symlink(symlink_node) => { info!( path = ?path, - name = symlink_node.name, - target = symlink_node.target, + name = ?symlink_node.name, + target = ?symlink_node.target, "import successful" ) } diff --git a/tvix/store/src/directoryservice/traverse.rs b/tvix/store/src/directoryservice/traverse.rs index 8dfccd4ff..17f709f40 100644 --- a/tvix/store/src/directoryservice/traverse.rs +++ b/tvix/store/src/directoryservice/traverse.rs @@ -1,6 +1,6 @@ use super::DirectoryService; use crate::{proto::NamedNode, B3Digest, Error}; -use std::sync::Arc; +use std::{os::unix::ffi::OsStrExt, sync::Arc}; use tracing::{instrument, warn}; /// This traverses from a (root) node to the given (sub)path, returning the Node @@ -58,9 +58,9 @@ pub fn traverse_to( // look for first_component in the [Directory]. // FUTUREWORK: as the nodes() iterator returns in a sorted fashion, we // could stop as soon as e.name is larger than the search string. - let child_node = directory.nodes().find(|n| { - n.get_name() == first_component.as_os_str().to_str().unwrap() - }); + let child_node = directory + .nodes() + .find(|n| n.get_name() == first_component.as_os_str().as_bytes()); match child_node { // child node not found means there's no such element inside the directory. @@ -105,7 +105,7 @@ mod tests { // construct the node for DIRECTORY_COMPLICATED let node_directory_complicated = crate::proto::node::Node::Directory(crate::proto::DirectoryNode { - name: "doesntmatter".to_string(), + name: "doesntmatter".into(), digest: DIRECTORY_COMPLICATED.digest().to_vec(), size: DIRECTORY_COMPLICATED.size(), }); diff --git a/tvix/store/src/fuse/inode_tracker.rs b/tvix/store/src/fuse/inode_tracker.rs index 06434d25b..8d9156471 100644 --- a/tvix/store/src/fuse/inode_tracker.rs +++ b/tvix/store/src/fuse/inode_tracker.rs @@ -13,7 +13,7 @@ pub struct InodeTracker { blob_digest_to_inode: HashMap, // lookup table for symlinks by their target - symlink_target_to_inode: HashMap, + symlink_target_to_inode: HashMap, u64>, // lookup table for directories by their B3Digest. // Note the corresponding directory may not be present in data yet. @@ -171,7 +171,7 @@ impl InodeTracker { self.blob_digest_to_inode.insert(digest.clone(), ino); } InodeData::Symlink(ref target) => { - self.symlink_target_to_inode.insert(target.to_string(), ino); + self.symlink_target_to_inode.insert(target.to_vec(), ino); } InodeData::Directory(DirectoryInodeData::Sparse(ref digest, _size)) => { self.directory_digest_to_inode.insert(digest.clone(), ino); @@ -251,7 +251,7 @@ mod tests { #[test] fn put_symlink() { let mut inode_tracker = InodeTracker::default(); - let f = InodeData::Symlink("target".to_string()); + let f = InodeData::Symlink("target".into()); // put it in let ino = inode_tracker.put(f.clone()); @@ -260,7 +260,7 @@ mod tests { let data = inode_tracker.get(ino).expect("must be some"); match *data { InodeData::Symlink(ref target) => { - assert_eq!("target", target); + assert_eq!(b"target".to_vec(), *target); } InodeData::Regular(..) | InodeData::Directory(..) => panic!("wrong type"), } @@ -269,10 +269,7 @@ mod tests { assert_eq!(ino, inode_tracker.put(f)); // inserting another file should return a different ino - assert_ne!( - ino, - inode_tracker.put(InodeData::Symlink("target2".to_string())) - ); + assert_ne!(ino, inode_tracker.put(InodeData::Symlink("target2".into()))); } // TODO: put sparse directory diff --git a/tvix/store/src/fuse/inodes.rs b/tvix/store/src/fuse/inodes.rs index 78756472b..bf883cb22 100644 --- a/tvix/store/src/fuse/inodes.rs +++ b/tvix/store/src/fuse/inodes.rs @@ -5,7 +5,7 @@ use crate::{proto, B3Digest}; #[derive(Clone, Debug)] pub enum InodeData { Regular(B3Digest, u32, bool), // digest, size, executable - Symlink(String), // target + Symlink(Vec), // target Directory(DirectoryInodeData), // either [DirectoryInodeData:Sparse] or [DirectoryInodeData:Populated] } diff --git a/tvix/store/src/fuse/mod.rs b/tvix/store/src/fuse/mod.rs index dd2c479b3..077c6ce04 100644 --- a/tvix/store/src/fuse/mod.rs +++ b/tvix/store/src/fuse/mod.rs @@ -19,6 +19,7 @@ use crate::{ use fuser::{FileAttr, ReplyAttr, Request}; use nix_compat::store_path::StorePath; use std::io::{self, Read, Seek}; +use std::os::unix::ffi::OsStrExt; use std::str::FromStr; use std::sync::Arc; use std::{collections::HashMap, time::Duration}; @@ -142,7 +143,7 @@ impl FUSE { let root_node = path_info.node.unwrap().node.unwrap(); // The name must match what's passed in the lookup, otherwise we return nothing. - if root_node.get_name() != store_path.to_string() { + if root_node.get_name() != store_path.to_string().as_bytes() { return Ok(None); } @@ -279,7 +280,9 @@ impl fuser::Filesystem for FUSE { let _enter = span.enter(); // in the children, find the one with the desired name. - if let Some((child_ino, _)) = children.iter().find(|e| e.1.get_name() == name) { + if let Some((child_ino, _)) = + children.iter().find(|e| e.1.get_name() == name.as_bytes()) + { // lookup the child [InodeData] in [self.inode_tracker]. // We know the inodes for children have already been allocated. let child_inode_data = self.inode_tracker.get(*child_ino).unwrap(); @@ -353,7 +356,7 @@ impl fuser::Filesystem for FUSE { Node::File(_) => fuser::FileType::RegularFile, Node::Symlink(_) => fuser::FileType::Symlink, }, - child_node.get_name(), + std::ffi::OsStr::from_bytes(child_node.get_name()), ); if full { break; @@ -480,7 +483,7 @@ impl fuser::Filesystem for FUSE { InodeData::Directory(..) | InodeData::Regular(..) => { reply.error(libc::EINVAL); } - InodeData::Symlink(ref target) => reply.data(target.as_bytes()), + InodeData::Symlink(ref target) => reply.data(target), } } } diff --git a/tvix/store/src/fuse/tests.rs b/tvix/store/src/fuse/tests.rs index 1a53b9f5d..8577e062e 100644 --- a/tvix/store/src/fuse/tests.rs +++ b/tvix/store/src/fuse/tests.rs @@ -57,7 +57,7 @@ fn populate_blob_a( let path_info = PathInfo { node: Some(proto::Node { node: Some(proto::node::Node::File(FileNode { - name: BLOB_A_NAME.to_string(), + name: BLOB_A_NAME.into(), digest: fixtures::BLOB_A_DIGEST.to_vec(), size: fixtures::BLOB_A.len() as u32, executable: false, @@ -83,7 +83,7 @@ fn populate_blob_b( let path_info = PathInfo { node: Some(proto::Node { node: Some(proto::node::Node::File(FileNode { - name: BLOB_B_NAME.to_string(), + name: BLOB_B_NAME.into(), digest: fixtures::BLOB_B_DIGEST.to_vec(), size: fixtures::BLOB_B.len() as u32, executable: false, @@ -103,8 +103,8 @@ fn populate_symlink( let path_info = PathInfo { node: Some(proto::Node { node: Some(proto::node::Node::Symlink(proto::SymlinkNode { - name: SYMLINK_NAME.to_string(), - target: BLOB_A_NAME.to_string(), + name: SYMLINK_NAME.into(), + target: BLOB_A_NAME.into(), })), }), ..Default::default() @@ -123,8 +123,8 @@ fn populate_symlink2( let path_info = PathInfo { node: Some(proto::Node { node: Some(proto::node::Node::Symlink(proto::SymlinkNode { - name: SYMLINK_NAME2.to_string(), - target: "/nix/store/somewhereelse".to_string(), + name: SYMLINK_NAME2.into(), + target: "/nix/store/somewhereelse".into(), })), }), ..Default::default() @@ -153,7 +153,7 @@ fn populate_directory_with_keep( let path_info = PathInfo { node: Some(proto::Node { node: Some(proto::node::Node::Directory(DirectoryNode { - name: DIRECTORY_WITH_KEEP_NAME.to_string(), + name: DIRECTORY_WITH_KEEP_NAME.into(), digest: fixtures::DIRECTORY_WITH_KEEP.digest().to_vec(), size: fixtures::DIRECTORY_WITH_KEEP.size(), })), @@ -174,7 +174,7 @@ fn populate_pathinfo_without_directory( let path_info = PathInfo { node: Some(proto::Node { node: Some(proto::node::Node::Directory(DirectoryNode { - name: DIRECTORY_WITH_KEEP_NAME.to_string(), + name: DIRECTORY_WITH_KEEP_NAME.into(), digest: fixtures::DIRECTORY_WITH_KEEP.digest().to_vec(), size: fixtures::DIRECTORY_WITH_KEEP.size(), })), @@ -194,7 +194,7 @@ fn populate_blob_a_without_blob( let path_info = PathInfo { node: Some(proto::Node { node: Some(proto::node::Node::File(FileNode { - name: BLOB_A_NAME.to_string(), + name: BLOB_A_NAME.into(), digest: fixtures::BLOB_A_DIGEST.to_vec(), size: fixtures::BLOB_A.len() as u32, executable: false, @@ -231,7 +231,7 @@ fn populate_directory_complicated( let path_info = PathInfo { node: Some(proto::Node { node: Some(proto::node::Node::Directory(DirectoryNode { - name: DIRECTORY_COMPLICATED_NAME.to_string(), + name: DIRECTORY_COMPLICATED_NAME.into(), digest: fixtures::DIRECTORY_COMPLICATED.digest().to_vec(), size: fixtures::DIRECTORY_COMPLICATED.size(), })), diff --git a/tvix/store/src/import.rs b/tvix/store/src/import.rs index dd366aef9..74c45c7a7 100644 --- a/tvix/store/src/import.rs +++ b/tvix/store/src/import.rs @@ -1,6 +1,7 @@ use crate::blobservice::BlobService; use crate::directoryservice::DirectoryService; use crate::{directoryservice::DirectoryPutter, proto}; +use std::os::unix::ffi::OsStrExt; use std::sync::Arc; use std::{ collections::HashMap, @@ -79,11 +80,7 @@ fn process_entry( .map_err(|e| Error::UploadDirectoryError(entry.path().to_path_buf(), e))?; return Ok(proto::node::Node::Directory(proto::DirectoryNode { - name: entry - .file_name() - .to_str() - .map(|s| Ok(s.to_owned())) - .unwrap_or(Err(Error::InvalidEncoding(entry.path().to_path_buf())))?, + name: entry.file_name().as_bytes().to_vec(), digest: directory_digest.to_vec(), size: directory_size, })); @@ -94,15 +91,8 @@ fn process_entry( .map_err(|e| Error::UnableToStat(entry_path.clone(), e))?; return Ok(proto::node::Node::Symlink(proto::SymlinkNode { - name: entry - .file_name() - .to_str() - .map(|s| Ok(s.to_owned())) - .unwrap_or(Err(Error::InvalidEncoding(entry.path().to_path_buf())))?, - target: target - .to_str() - .map(|s| Ok(s.to_owned())) - .unwrap_or(Err(Error::InvalidEncoding(entry.path().to_path_buf())))?, + name: entry.file_name().as_bytes().to_vec(), + target: target.as_os_str().as_bytes().to_vec(), })); } @@ -123,11 +113,7 @@ fn process_entry( let digest = writer.close()?; return Ok(proto::node::Node::File(proto::FileNode { - name: entry - .file_name() - .to_str() - .map(|s| Ok(s.to_owned())) - .unwrap_or(Err(Error::InvalidEncoding(entry.path().to_path_buf())))?, + name: entry.file_name().as_bytes().to_vec(), digest: digest.to_vec(), size: metadata.len() as u32, // If it's executable by the user, it'll become executable. @@ -163,13 +149,9 @@ pub fn ingest_path + Debug>( .as_ref() .file_name() .unwrap_or_default() - .to_str() - .map(|s| Ok(s.to_owned())) - .unwrap_or(Err(Error::InvalidEncoding(p.as_ref().to_path_buf())))?, - target: target - .to_str() - .map(|s| Ok(s.to_owned())) - .unwrap_or(Err(Error::InvalidEncoding(p.as_ref().to_path_buf())))?, + .as_bytes() + .to_vec(), + target: target.as_os_str().as_bytes().to_vec(), })); } diff --git a/tvix/store/src/nar/mod.rs b/tvix/store/src/nar/mod.rs index 13e4f7bd9..84a48ba5f 100644 --- a/tvix/store/src/nar/mod.rs +++ b/tvix/store/src/nar/mod.rs @@ -12,14 +12,14 @@ pub enum RenderError { #[error("failure talking to a backing store client: {0}")] StoreError(crate::Error), - #[error("unable to find directory {}, referred from {}", .0, .1)] - DirectoryNotFound(B3Digest, String), + #[error("unable to find directory {}, referred from {:?}", .0, .1)] + DirectoryNotFound(B3Digest, Vec), - #[error("unable to find blob {}, referred from {}", BASE64.encode(.0), .1)] - BlobNotFound([u8; 32], String), + #[error("unable to find blob {}, referred from {:?}", BASE64.encode(.0), .1)] + BlobNotFound([u8; 32], Vec), - #[error("unexpected size in metadata for blob {}, referred from {} returned, expected {}, got {}", BASE64.encode(.0), .1, .2, .3)] - UnexpectedBlobMeta([u8; 32], String, u32, u32), + #[error("unexpected size in metadata for blob {}, referred from {:?} returned, expected {}, got {}", BASE64.encode(.0), .1, .2, .3)] + UnexpectedBlobMeta([u8; 32], Vec, u32, u32), #[error("failure using the NAR writer: {0}")] NARWriterError(std::io::Error), diff --git a/tvix/store/src/proto/grpc_directoryservice_wrapper.rs b/tvix/store/src/proto/grpc_directoryservice_wrapper.rs index 434d660f3..ec9e3cb12 100644 --- a/tvix/store/src/proto/grpc_directoryservice_wrapper.rs +++ b/tvix/store/src/proto/grpc_directoryservice_wrapper.rs @@ -116,7 +116,7 @@ impl proto::directory_service_server::DirectoryService for GRPCDirectoryServiceW match seen_directories_sizes.get(&child_directory_digest) { None => { return Err(Status::invalid_argument(format!( - "child directory '{}' ({}) in directory '{}' not seen yet", + "child directory '{:?}' ({}) in directory '{}' not seen yet", child_directory.name, &child_directory_digest, &directory.digest(), @@ -125,7 +125,7 @@ impl proto::directory_service_server::DirectoryService for GRPCDirectoryServiceW Some(seen_child_directory_size) => { if seen_child_directory_size != &child_directory.size { return Err(Status::invalid_argument(format!( - "child directory '{}' ({}) in directory '{}' referred with wrong size, expected {}, actual {}", + "child directory '{:?}' ({}) in directory '{}' referred with wrong size, expected {}, actual {}", child_directory.name, &child_directory_digest, &directory.digest(), diff --git a/tvix/store/src/proto/mod.rs b/tvix/store/src/proto/mod.rs index 7e6972663..7e81efc51 100644 --- a/tvix/store/src/proto/mod.rs +++ b/tvix/store/src/proto/mod.rs @@ -1,6 +1,5 @@ #![allow(clippy::derive_partial_eq_without_eq)] // https://github.com/hyperium/tonic/issues/1056 -use std::str::FromStr; use std::{collections::HashSet, iter::Peekable}; use thiserror::Error; @@ -35,14 +34,14 @@ mod tests; #[derive(Debug, PartialEq, Eq, Error)] pub enum ValidateDirectoryError { /// Elements are not in sorted order - #[error("{0} is not sorted")] - WrongSorting(String), + #[error("{0:?} is not sorted")] + WrongSorting(Vec), /// Multiple elements with the same name encountered - #[error("{0} is a duplicate name")] - DuplicateName(String), + #[error("{0:?} is a duplicate name")] + DuplicateName(Vec), /// Invalid name encountered - #[error("Invalid name in {0}")] - InvalidName(String), + #[error("Invalid name in {0:?}")] + InvalidName(Vec), /// Invalid digest length encountered #[error("Invalid Digest length: {0}")] InvalidDigestLen(usize), @@ -56,8 +55,8 @@ pub enum ValidatePathInfoError { NoNodePresent(), /// Invalid node name encountered. - #[error("Failed to parse {0} as StorePath: {1}")] - InvalidNodeName(String, store_path::Error), + #[error("Failed to parse {0:?} as StorePath: {1}")] + InvalidNodeName(Vec, store_path::Error), /// The digest the (root) node refers to has invalid length. #[error("Invalid Digest length: {0}")] @@ -73,10 +72,14 @@ pub enum ValidatePathInfoError { /// error that's generated from the supplied constructor. /// /// We disallow slashes, null bytes, '.', '..' and the empty string. -fn validate_node_name(name: &str, err: fn(String) -> E) -> Result<(), E> { - if name.is_empty() || name == ".." || name == "." || name.contains('\x00') || name.contains('/') +fn validate_node_name(name: &[u8], err: fn(Vec) -> E) -> Result<(), E> { + if name.is_empty() + || name == b".." + || name == b"." + || name.contains(&0x00) + || name.contains(&b'/') { - return Err(err(name.to_string())); + return Err(err(name.to_vec())); } Ok(()) } @@ -95,12 +98,12 @@ fn validate_digest(digest: &Vec, err: fn(usize) -> E) -> Result<(), E> { /// On success, this returns the parsed [StorePath]. /// On error, it returns an error generated from the supplied constructor. fn parse_node_name_root( - name: &str, - err: fn(String, store_path::Error) -> E, + name: &[u8], + err: fn(Vec, store_path::Error) -> E, ) -> Result { - match StorePath::from_str(name) { + match StorePath::from_bytes(name) { Ok(np) => Ok(np), - Err(e) => Err(err(name.to_string(), e)), + Err(e) => Err(err(name.to_vec(), e)), } } @@ -169,29 +172,29 @@ impl PathInfo { /// NamedNode is implemented for [FileNode], [DirectoryNode] and [SymlinkNode] /// and [node::Node], so we can ask all of them for the name easily. pub trait NamedNode { - fn get_name(&self) -> &str; + fn get_name(&self) -> &[u8]; } impl NamedNode for &FileNode { - fn get_name(&self) -> &str { - self.name.as_str() + fn get_name(&self) -> &[u8] { + &self.name } } impl NamedNode for &DirectoryNode { - fn get_name(&self) -> &str { - self.name.as_str() + fn get_name(&self) -> &[u8] { + &self.name } } impl NamedNode for &SymlinkNode { - fn get_name(&self) -> &str { - self.name.as_str() + fn get_name(&self) -> &[u8] { + &self.name } } impl NamedNode for node::Node { - fn get_name(&self) -> &str { + fn get_name(&self) -> &[u8] { match self { node::Node::File(node_file) => &node_file.name, node::Node::Directory(node_directory) => &node_directory.name, @@ -204,11 +207,11 @@ impl NamedNode for node::Node { /// If the passed name is larger than the previous one, the reference is updated. /// If it's not, an error is returned. fn update_if_lt_prev<'n>( - prev_name: &mut &'n str, - name: &'n str, + prev_name: &mut &'n [u8], + name: &'n [u8], ) -> Result<(), ValidateDirectoryError> { if *name < **prev_name { - return Err(ValidateDirectoryError::WrongSorting(name.to_string())); + return Err(ValidateDirectoryError::WrongSorting(name.to_vec())); } *prev_name = name; Ok(()) @@ -217,11 +220,11 @@ fn update_if_lt_prev<'n>( /// Inserts the given name into a HashSet if it's not already in there. /// If it is, an error is returned. fn insert_once<'n>( - seen_names: &mut HashSet<&'n str>, - name: &'n str, + seen_names: &mut HashSet<&'n [u8]>, + name: &'n [u8], ) -> Result<(), ValidateDirectoryError> { if seen_names.get(name).is_some() { - return Err(ValidateDirectoryError::DuplicateName(name.to_string())); + return Err(ValidateDirectoryError::DuplicateName(name.to_vec())); } seen_names.insert(name); Ok(()) @@ -258,11 +261,11 @@ impl Directory { /// - not properly sorted lists /// - duplicate names in the three lists pub fn validate(&self) -> Result<(), ValidateDirectoryError> { - let mut seen_names: HashSet<&str> = HashSet::new(); + let mut seen_names: HashSet<&[u8]> = HashSet::new(); - let mut last_directory_name: &str = ""; - let mut last_file_name: &str = ""; - let mut last_symlink_name: &str = ""; + let mut last_directory_name: &[u8] = b""; + let mut last_file_name: &[u8] = b""; + let mut last_symlink_name: &[u8] = b""; // check directories for directory_node in &self.directories { @@ -272,8 +275,8 @@ impl Directory { ValidateDirectoryError::InvalidDigestLen, )?; - update_if_lt_prev(&mut last_directory_name, directory_node.name.as_str())?; - insert_once(&mut seen_names, directory_node.name.as_str())?; + update_if_lt_prev(&mut last_directory_name, &directory_node.name)?; + insert_once(&mut seen_names, &directory_node.name)?; } // check files @@ -281,16 +284,16 @@ impl Directory { validate_node_name(&file_node.name, ValidateDirectoryError::InvalidName)?; validate_digest(&file_node.digest, ValidateDirectoryError::InvalidDigestLen)?; - update_if_lt_prev(&mut last_file_name, file_node.name.as_str())?; - insert_once(&mut seen_names, file_node.name.as_str())?; + update_if_lt_prev(&mut last_file_name, &file_node.name)?; + insert_once(&mut seen_names, &file_node.name)?; } // check symlinks for symlink_node in &self.symlinks { validate_node_name(&symlink_node.name, ValidateDirectoryError::InvalidName)?; - update_if_lt_prev(&mut last_symlink_name, symlink_node.name.as_str())?; - insert_once(&mut seen_names, symlink_node.name.as_str())?; + update_if_lt_prev(&mut last_symlink_name, &symlink_node.name)?; + insert_once(&mut seen_names, &symlink_node.name)?; } Ok(()) diff --git a/tvix/store/src/proto/tests/directory.rs b/tvix/store/src/proto/tests/directory.rs index 8d6ca7241..48eeaa7b5 100644 --- a/tvix/store/src/proto/tests/directory.rs +++ b/tvix/store/src/proto/tests/directory.rs @@ -20,7 +20,7 @@ fn size() { { let d = Directory { directories: vec![DirectoryNode { - name: String::from("foo"), + name: "foo".into(), digest: DUMMY_DIGEST.to_vec(), size: 0, }], @@ -31,7 +31,7 @@ fn size() { { let d = Directory { directories: vec![DirectoryNode { - name: String::from("foo"), + name: "foo".into(), digest: DUMMY_DIGEST.to_vec(), size: 4, }], @@ -42,7 +42,7 @@ fn size() { { let d = Directory { files: vec![FileNode { - name: String::from("foo"), + name: "foo".into(), digest: DUMMY_DIGEST.to_vec(), size: 42, executable: false, @@ -54,8 +54,8 @@ fn size() { { let d = Directory { symlinks: vec![SymlinkNode { - name: String::from("foo"), - target: String::from("bar"), + name: "foo".into(), + target: "bar".into(), }], ..Default::default() }; @@ -89,7 +89,7 @@ fn validate_invalid_names() { { let d = Directory { directories: vec![DirectoryNode { - name: "".to_string(), + name: "".into(), digest: DUMMY_DIGEST.to_vec(), size: 42, }], @@ -97,7 +97,7 @@ fn validate_invalid_names() { }; match d.validate().expect_err("must fail") { ValidateDirectoryError::InvalidName(n) => { - assert_eq!(n, "") + assert_eq!(n, b"") } _ => panic!("unexpected error"), }; @@ -106,7 +106,7 @@ fn validate_invalid_names() { { let d = Directory { directories: vec![DirectoryNode { - name: ".".to_string(), + name: ".".into(), digest: DUMMY_DIGEST.to_vec(), size: 42, }], @@ -114,7 +114,7 @@ fn validate_invalid_names() { }; match d.validate().expect_err("must fail") { ValidateDirectoryError::InvalidName(n) => { - assert_eq!(n, ".") + assert_eq!(n, b".") } _ => panic!("unexpected error"), }; @@ -123,7 +123,7 @@ fn validate_invalid_names() { { let d = Directory { files: vec![FileNode { - name: "..".to_string(), + name: "..".into(), digest: DUMMY_DIGEST.to_vec(), size: 42, executable: false, @@ -132,7 +132,7 @@ fn validate_invalid_names() { }; match d.validate().expect_err("must fail") { ValidateDirectoryError::InvalidName(n) => { - assert_eq!(n, "..") + assert_eq!(n, b"..") } _ => panic!("unexpected error"), }; @@ -141,14 +141,14 @@ fn validate_invalid_names() { { let d = Directory { symlinks: vec![SymlinkNode { - name: "\x00".to_string(), - target: "foo".to_string(), + name: "\x00".into(), + target: "foo".into(), }], ..Default::default() }; match d.validate().expect_err("must fail") { ValidateDirectoryError::InvalidName(n) => { - assert_eq!(n, "\x00") + assert_eq!(n, b"\x00") } _ => panic!("unexpected error"), }; @@ -157,14 +157,14 @@ fn validate_invalid_names() { { let d = Directory { symlinks: vec![SymlinkNode { - name: "foo/bar".to_string(), - target: "foo".to_string(), + name: "foo/bar".into(), + target: "foo".into(), }], ..Default::default() }; match d.validate().expect_err("must fail") { ValidateDirectoryError::InvalidName(n) => { - assert_eq!(n, "foo/bar") + assert_eq!(n, b"foo/bar") } _ => panic!("unexpected error"), }; @@ -175,7 +175,7 @@ fn validate_invalid_names() { fn validate_invalid_digest() { let d = Directory { directories: vec![DirectoryNode { - name: "foo".to_string(), + name: "foo".into(), digest: vec![0x00, 0x42], // invalid length size: 42, }], @@ -196,12 +196,12 @@ fn validate_sorting() { let d = Directory { directories: vec![ DirectoryNode { - name: "b".to_string(), + name: "b".into(), digest: DUMMY_DIGEST.to_vec(), size: 42, }, DirectoryNode { - name: "a".to_string(), + name: "a".into(), digest: DUMMY_DIGEST.to_vec(), size: 42, }, @@ -210,7 +210,7 @@ fn validate_sorting() { }; match d.validate().expect_err("must fail") { ValidateDirectoryError::WrongSorting(s) => { - assert_eq!(s, "a".to_string()); + assert_eq!(s, b"a"); } _ => panic!("unexpected error"), } @@ -221,12 +221,12 @@ fn validate_sorting() { let d = Directory { directories: vec![ DirectoryNode { - name: "a".to_string(), + name: "a".into(), digest: DUMMY_DIGEST.to_vec(), size: 42, }, DirectoryNode { - name: "a".to_string(), + name: "a".into(), digest: DUMMY_DIGEST.to_vec(), size: 42, }, @@ -235,7 +235,7 @@ fn validate_sorting() { }; match d.validate().expect_err("must fail") { ValidateDirectoryError::DuplicateName(s) => { - assert_eq!(s, "a".to_string()); + assert_eq!(s, b"a"); } _ => panic!("unexpected error"), } @@ -246,12 +246,12 @@ fn validate_sorting() { let d = Directory { directories: vec![ DirectoryNode { - name: "a".to_string(), + name: "a".into(), digest: DUMMY_DIGEST.to_vec(), size: 42, }, DirectoryNode { - name: "b".to_string(), + name: "b".into(), digest: DUMMY_DIGEST.to_vec(), size: 42, }, @@ -267,19 +267,19 @@ fn validate_sorting() { let d = Directory { directories: vec![ DirectoryNode { - name: "b".to_string(), + name: "b".into(), digest: DUMMY_DIGEST.to_vec(), size: 42, }, DirectoryNode { - name: "c".to_string(), + name: "c".into(), digest: DUMMY_DIGEST.to_vec(), size: 42, }, ], symlinks: vec![SymlinkNode { - name: "a".to_string(), - target: "foo".to_string(), + name: "a".into(), + target: "foo".into(), }], ..Default::default() }; diff --git a/tvix/store/src/proto/tests/directory_nodes_iterator.rs b/tvix/store/src/proto/tests/directory_nodes_iterator.rs index 9a283f72b..68f147a33 100644 --- a/tvix/store/src/proto/tests/directory_nodes_iterator.rs +++ b/tvix/store/src/proto/tests/directory_nodes_iterator.rs @@ -1,7 +1,7 @@ -use crate::proto::node::Node; use crate::proto::Directory; use crate::proto::DirectoryNode; use crate::proto::FileNode; +use crate::proto::NamedNode; use crate::proto::SymlinkNode; #[test] @@ -9,68 +9,66 @@ fn iterator() { let d = Directory { directories: vec![ DirectoryNode { - name: "c".to_string(), + name: "c".into(), ..DirectoryNode::default() }, DirectoryNode { - name: "d".to_string(), + name: "d".into(), ..DirectoryNode::default() }, DirectoryNode { - name: "h".to_string(), + name: "h".into(), ..DirectoryNode::default() }, DirectoryNode { - name: "l".to_string(), + name: "l".into(), ..DirectoryNode::default() }, ], files: vec![ FileNode { - name: "b".to_string(), + name: "b".into(), ..FileNode::default() }, FileNode { - name: "e".to_string(), + name: "e".into(), ..FileNode::default() }, FileNode { - name: "g".to_string(), + name: "g".into(), ..FileNode::default() }, FileNode { - name: "j".to_string(), + name: "j".into(), ..FileNode::default() }, ], symlinks: vec![ SymlinkNode { - name: "a".to_string(), + name: "a".into(), ..SymlinkNode::default() }, SymlinkNode { - name: "f".to_string(), + name: "f".into(), ..SymlinkNode::default() }, SymlinkNode { - name: "i".to_string(), + name: "i".into(), ..SymlinkNode::default() }, SymlinkNode { - name: "k".to_string(), + name: "k".into(), ..SymlinkNode::default() }, ], }; + // We keep this strings here and convert to string to make the comparison + // less messy. let mut node_names: Vec = vec![]; for node in d.nodes() { - match node { - Node::Directory(n) => node_names.push(n.name), - Node::File(n) => node_names.push(n.name), - Node::Symlink(n) => node_names.push(n.name), - }; + node_names.push(String::from_utf8(node.get_name().to_vec()).unwrap()); } assert_eq!( diff --git a/tvix/store/src/proto/tests/grpc_directoryservice.rs b/tvix/store/src/proto/tests/grpc_directoryservice.rs index a1058706d..73ce0082d 100644 --- a/tvix/store/src/proto/tests/grpc_directoryservice.rs +++ b/tvix/store/src/proto/tests/grpc_directoryservice.rs @@ -190,8 +190,8 @@ async fn put_reject_failed_validation() { // construct a broken Directory message that fails validation let broken_directory = Directory { symlinks: vec![SymlinkNode { - name: "".to_string(), - target: "doesntmatter".to_string(), + name: "".into(), + target: "doesntmatter".into(), }], ..Default::default() }; @@ -214,7 +214,7 @@ async fn put_reject_wrong_size() { // Construct a directory referring to DIRECTORY_A, but with wrong size. let broken_parent_directory = Directory { directories: vec![DirectoryNode { - name: "foo".to_string(), + name: "foo".into(), digest: DIRECTORY_A.digest().to_vec(), size: 42, }], diff --git a/tvix/store/src/proto/tests/grpc_pathinfoservice.rs b/tvix/store/src/proto/tests/grpc_pathinfoservice.rs index 186461d16..57c88c286 100644 --- a/tvix/store/src/proto/tests/grpc_pathinfoservice.rs +++ b/tvix/store/src/proto/tests/grpc_pathinfoservice.rs @@ -48,8 +48,8 @@ async fn put_get() { let path_info = PathInfo { node: Some(Node { node: Some(Symlink(SymlinkNode { - name: "00000000000000000000000000000000-foo".to_string(), - target: "doesntmatter".to_string(), + name: "00000000000000000000000000000000-foo".into(), + target: "doesntmatter".into(), })), }), ..Default::default() diff --git a/tvix/store/src/proto/tests/pathinfo.rs b/tvix/store/src/proto/tests/pathinfo.rs index bccec539f..a14554ee4 100644 --- a/tvix/store/src/proto/tests/pathinfo.rs +++ b/tvix/store/src/proto/tests/pathinfo.rs @@ -43,7 +43,7 @@ fn validate_no_node( #[test_case( proto::DirectoryNode { - name: DUMMY_NAME.to_string(), + name: DUMMY_NAME.into(), digest: DUMMY_DIGEST.to_vec(), size: 0, }, @@ -52,7 +52,7 @@ fn validate_no_node( )] #[test_case( proto::DirectoryNode { - name: DUMMY_NAME.to_string(), + name: DUMMY_NAME.into(), digest: vec![], size: 0, }, @@ -61,12 +61,12 @@ fn validate_no_node( )] #[test_case( proto::DirectoryNode { - name: "invalid".to_string(), + name: "invalid".into(), digest: DUMMY_DIGEST.to_vec(), size: 0, }, Err(ValidatePathInfoError::InvalidNodeName( - "invalid".to_string(), + "invalid".into(), store_path::Error::InvalidLength() )); "invalid node name" @@ -87,7 +87,7 @@ fn validate_directory( #[test_case( proto::FileNode { - name: DUMMY_NAME.to_string(), + name: DUMMY_NAME.into(), digest: DUMMY_DIGEST.to_vec(), size: 0, executable: false, @@ -97,7 +97,7 @@ fn validate_directory( )] #[test_case( proto::FileNode { - name: DUMMY_NAME.to_string(), + name: DUMMY_NAME.into(), digest: vec![], ..Default::default() }, @@ -106,12 +106,12 @@ fn validate_directory( )] #[test_case( proto::FileNode { - name: "invalid".to_string(), + name: "invalid".into(), digest: DUMMY_DIGEST.to_vec(), ..Default::default() }, Err(ValidatePathInfoError::InvalidNodeName( - "invalid".to_string(), + "invalid".into(), store_path::Error::InvalidLength() )); "invalid node name" @@ -129,7 +129,7 @@ fn validate_file(t_file_node: proto::FileNode, t_result: Result { @@ -265,7 +264,7 @@ impl EvalIO for TvixStoreIO { } #[instrument(skip(self), ret, err)] - fn read_dir(&self, path: &Path) -> Result, io::Error> { + fn read_dir(&self, path: &Path) -> Result, FileType)>, io::Error> { if let Ok((store_path, sub_path)) = StorePath::from_absolute_path_full(&path.to_string_lossy()) { @@ -285,17 +284,17 @@ impl EvalIO for TvixStoreIO { })?; if let Some(directory) = self.directory_service.get(&digest)? { - let mut children: Vec<(SmolStr, FileType)> = Vec::new(); + let mut children: Vec<(Vec, FileType)> = Vec::new(); for node in directory.nodes() { children.push(match node { crate::proto::node::Node::Directory(e) => { - (e.name.into(), FileType::Directory) + (e.name, FileType::Directory) } crate::proto::node::Node::File(e) => { - (e.name.into(), FileType::Regular) + (e.name, FileType::Regular) } crate::proto::node::Node::Symlink(e) => { - (e.name.into(), FileType::Symlink) + (e.name, FileType::Symlink) } }) } @@ -338,11 +337,20 @@ impl EvalIO for TvixStoreIO { let path_info = self.import_path_with_pathinfo(path)?; // from the [PathInfo], extract the store path (as string). - let mut path = PathBuf::from(nix_compat::store_path::STORE_DIR_WITH_SLASH); - path.push(path_info.node.unwrap().node.unwrap().get_name()); + Ok({ + let mut path = PathBuf::from(nix_compat::store_path::STORE_DIR_WITH_SLASH); - // and return it - Ok(path) + let root_node_name = path_info.node.unwrap().node.unwrap().get_name().to_vec(); + + // This must be a string, otherwise it would have failed validation. + let root_node_name = String::from_utf8(root_node_name).unwrap(); + + // append to the PathBuf + path.push(root_node_name); + + // and return it + path + }) } #[instrument(skip(self), ret)] diff --git a/tvix/store/src/tests/fixtures.rs b/tvix/store/src/tests/fixtures.rs index 934d9e4c5..a1df729f1 100644 --- a/tvix/store/src/tests/fixtures.rs +++ b/tvix/store/src/tests/fixtures.rs @@ -33,7 +33,7 @@ lazy_static! { pub static ref DIRECTORY_WITH_KEEP: proto::Directory = proto::Directory { directories: vec![], files: vec![FileNode { - name: ".keep".to_string(), + name: b".keep".to_vec(), digest: EMPTY_BLOB_DIGEST.to_vec(), size: 0, executable: false, @@ -42,25 +42,25 @@ lazy_static! { }; pub static ref DIRECTORY_COMPLICATED: proto::Directory = proto::Directory { directories: vec![DirectoryNode { - name: "keep".to_string(), + name: b"keep".to_vec(), digest: DIRECTORY_WITH_KEEP.digest().to_vec(), size: DIRECTORY_WITH_KEEP.size(), }], files: vec![FileNode { - name: ".keep".to_string(), + name: b".keep".to_vec(), digest: EMPTY_BLOB_DIGEST.to_vec(), size: 0, executable: false, }], symlinks: vec![SymlinkNode { - name: "aa".to_string(), - target: "/nix/store/somewhereelse".to_string(), + name: b"aa".to_vec(), + target: b"/nix/store/somewhereelse".to_vec(), }], }; pub static ref DIRECTORY_A: Directory = Directory::default(); pub static ref DIRECTORY_B: Directory = Directory { directories: vec![DirectoryNode { - name: "a".to_string(), + name: b"a".to_vec(), digest: DIRECTORY_A.digest().to_vec(), size: DIRECTORY_A.size(), }], @@ -69,12 +69,12 @@ lazy_static! { pub static ref DIRECTORY_C: Directory = Directory { directories: vec![ DirectoryNode { - name: "a".to_string(), + name: b"a".to_vec(), digest: DIRECTORY_A.digest().to_vec(), size: DIRECTORY_A.size(), }, DirectoryNode { - name: "a'".to_string(), + name: b"a'".to_vec(), digest: DIRECTORY_A.digest().to_vec(), size: DIRECTORY_A.size(), } diff --git a/tvix/store/src/tests/import.rs b/tvix/store/src/tests/import.rs index ab6557421..291501f72 100644 --- a/tvix/store/src/tests/import.rs +++ b/tvix/store/src/tests/import.rs @@ -5,6 +5,9 @@ use crate::tests::fixtures::DIRECTORY_COMPLICATED; use crate::tests::fixtures::*; use tempfile::TempDir; +#[cfg(target_family = "unix")] +use std::os::unix::ffi::OsStrExt; + #[cfg(target_family = "unix")] #[test] fn symlink() { @@ -26,8 +29,8 @@ fn symlink() { assert_eq!( crate::proto::node::Node::Symlink(proto::SymlinkNode { - name: "doesntmatter".to_string(), - target: "/nix/store/somewhereelse".to_string(), + name: "doesntmatter".into(), + target: "/nix/store/somewhereelse".into(), }), root_node, ) @@ -50,7 +53,7 @@ fn single_file() { assert_eq!( crate::proto::node::Node::File(proto::FileNode { - name: "root".to_string(), + name: "root".into(), digest: HELLOWORLD_BLOB_DIGEST.to_vec(), size: HELLOWORLD_BLOB_CONTENTS.len() as u32, executable: false, @@ -62,6 +65,7 @@ fn single_file() { assert!(blob_service.has(&HELLOWORLD_BLOB_DIGEST).unwrap()); } +#[cfg(target_family = "unix")] #[test] fn complicated() { let tmpdir = TempDir::new().unwrap(); @@ -88,12 +92,7 @@ fn complicated() { // ensure root_node matched expectations assert_eq!( crate::proto::node::Node::Directory(proto::DirectoryNode { - name: tmpdir - .path() - .file_name() - .unwrap() - .to_string_lossy() - .to_string(), + name: tmpdir.path().file_name().unwrap().as_bytes().to_vec(), digest: DIRECTORY_COMPLICATED.digest().to_vec(), size: DIRECTORY_COMPLICATED.size(), }), diff --git a/tvix/store/src/tests/nar_renderer.rs b/tvix/store/src/tests/nar_renderer.rs index b92fdc087..055538376 100644 --- a/tvix/store/src/tests/nar_renderer.rs +++ b/tvix/store/src/tests/nar_renderer.rs @@ -15,8 +15,8 @@ fn single_symlink() { write_nar( &mut buf, &crate::proto::node::Node::Symlink(SymlinkNode { - name: "doesntmatter".to_string(), - target: "/nix/store/somewhereelse".to_string(), + name: "doesntmatter".into(), + target: "/nix/store/somewhereelse".into(), }), // don't put anything in the stores, as we don't actually do any requests. gen_blob_service(), @@ -35,7 +35,7 @@ fn single_file_missing_blob() { let e = write_nar( &mut buf, &crate::proto::node::Node::File(FileNode { - name: "doesntmatter".to_string(), + name: "doesntmatter".into(), digest: HELLOWORLD_BLOB_DIGEST.to_vec(), size: HELLOWORLD_BLOB_CONTENTS.len() as u32, executable: false, @@ -76,7 +76,7 @@ fn single_file_wrong_blob_size() { let e = write_nar( &mut buf, &crate::proto::node::Node::File(FileNode { - name: "doesntmatter".to_string(), + name: "doesntmatter".into(), digest: HELLOWORLD_BLOB_DIGEST.to_vec(), size: 42, // <- note the wrong size here! executable: false, @@ -101,7 +101,7 @@ fn single_file_wrong_blob_size() { let e = write_nar( &mut buf, &crate::proto::node::Node::File(FileNode { - name: "doesntmatter".to_string(), + name: "doesntmatter".into(), digest: HELLOWORLD_BLOB_DIGEST.to_vec(), size: 2, // <- note the wrong size here! executable: false, @@ -138,7 +138,7 @@ fn single_file() { write_nar( &mut buf, &crate::proto::node::Node::File(FileNode { - name: "doesntmatter".to_string(), + name: "doesntmatter".into(), digest: HELLOWORLD_BLOB_DIGEST.to_vec(), size: HELLOWORLD_BLOB_CONTENTS.len() as u32, executable: false, @@ -176,7 +176,7 @@ fn test_complicated() { write_nar( &mut buf, &crate::proto::node::Node::Directory(DirectoryNode { - name: "doesntmatter".to_string(), + name: "doesntmatter".into(), digest: DIRECTORY_COMPLICATED.digest().to_vec(), size: DIRECTORY_COMPLICATED.size(), }), @@ -190,7 +190,7 @@ fn test_complicated() { // ensure calculate_nar does return the correct sha256 digest and sum. let (nar_size, nar_digest) = calculate_size_and_sha256( &crate::proto::node::Node::Directory(DirectoryNode { - name: "doesntmatter".to_string(), + name: "doesntmatter".into(), digest: DIRECTORY_COMPLICATED.digest().to_vec(), size: DIRECTORY_COMPLICATED.size(), }),