Refactor attributes API

This patch refactory the API to read and write attributes so that the
buffer is always provided by the caller.  This means that the caller is
not required to always use a 1 kB buffer even if the attribute length is
much smaller.

Fixes: #75

CHANGELOG.md

@@ -34,6 +34,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
   - Remove `Error::Success` and enforce negative values for `Error`.
 - Replace `Path::exists` with `Filesystem::exists`
 - Replace `DynFilesystem::open_file_with_options_and_then{,unit}` with `DynFilesystem::open_file_with_flags_and_then{,unit}` using `FileOpenFlags` instead of `OpenOptionsCallback`
+- Refactor attributes API:
+  - Change the `set_attribute` function in `DynFilesystem` and `Filesystem` to accept an ID and a slice instead of an `Attribute`.
+  - Add a buffer argument to the `attribute` function in `DynFilesystem` and `Filesystem` and return a slice of that buffer containing the read data.
+  - Change the `Attribute` struct to store a slice with the read data and the total size of the attribute on the filesystem.
 
 ### Removed
 

core/src/fs.rs

@@ -82,61 +82,31 @@ impl Metadata {
 #[derive(Clone, Debug, Eq, PartialEq)]
 /// Custom user attribute that can be set on files and directories.
 ///
-/// Consists of an numerical identifier between 0 and 255, and arbitrary
-/// binary data up to size `ATTRBYTES_MAX`.
+/// This struct stores the data that has been read from the filesystem and
+/// the total size of the attribute on the filesystem.  The maximum size of an
+/// attribute is [`Attribute::MAX_SIZE`][].
 ///
-/// Use [`Filesystem::attribute`](struct.Filesystem.html#method.attribute),
-/// [`Filesystem::set_attribute`](struct.Filesystem.html#method.set_attribute), and
-/// [`Filesystem::clear_attribute`](struct.Filesystem.html#method.clear_attribute).
-pub struct Attribute {
-    id: u8,
-    data: [u8; Attribute::MAX_SIZE as _],
-    // invariant: size <= data.len()
-    size: usize,
+/// See [`DynFilesystem::attribute`](`crate::object_safe::DynFilesystem::attribute`).
+pub struct Attribute<'a> {
+    data: &'a [u8],
+    total_size: usize,
 }
 
-impl Attribute {
+impl<'a> Attribute<'a> {
     pub const MAX_SIZE: u32 = 1_022;
 
-    pub fn new(id: u8) -> Self {
-        Attribute {
-            id,
-            data: [0; Self::MAX_SIZE as _],
-            size: 0,
-        }
-    }
-
-    pub fn id(&self) -> u8 {
-        self.id
+    pub fn new(data: &'a [u8], total_size: usize) -> Self {
+        let n = cmp::min(data.len(), total_size);
+        let data = &data[..n];
+        Attribute { data, total_size }
     }
 
     pub fn data(&self) -> &[u8] {
-        let attr_max = Self::MAX_SIZE as _;
-        let len = cmp::min(attr_max, self.size);
-        &self.data[..len]
-    }
-
-    pub fn size(&self) -> usize {
-        self.size
-    }
-
-    pub fn set_data(&mut self, data: &[u8]) -> &mut Self {
-        let attr_max = Self::MAX_SIZE as _;
-        let len = cmp::min(attr_max, data.len());
-        self.data[..len].copy_from_slice(&data[..len]);
-        self.size = len;
-        for entry in self.data[len..].iter_mut() {
-            *entry = 0;
-        }
-        self
-    }
-
-    pub fn buffer_mut(&mut self) -> &mut [u8] {
-        &mut self.data
+        self.data
     }
 
-    pub fn set_size(&mut self, size: usize) {
-        self.size = cmp::min(size, self.data.len());
+    pub fn total_size(&self) -> usize {
+        self.total_size
     }
 }
 

core/src/object_safe.rs

@@ -73,9 +73,14 @@ pub trait DynFilesystem {
         path: &Path,
         f: FileCallback<'_>,
     ) -> Result<()>;
-    fn attribute(&self, path: &Path, id: u8) -> Result<Option<Attribute>>;
+    fn attribute<'a>(
+        &self,
+        path: &Path,
+        id: u8,
+        buffer: &'a mut [u8],
+    ) -> Result<Option<Attribute<'a>>>;
     fn remove_attribute(&self, path: &Path, id: u8) -> Result<()>;
-    fn set_attribute(&self, path: &Path, attribute: &Attribute) -> Result<()>;
+    fn set_attribute(&self, path: &Path, id: u8, data: &[u8]) -> Result<()>;
     fn read_dir_and_then_unit(&self, path: &Path, f: DirEntriesCallback<'_>) -> Result<()>;
     fn create_dir(&self, path: &Path) -> Result<()>;
     fn create_dir_all(&self, path: &Path) -> Result<()>;

src/fs.rs

@@ -416,23 +416,27 @@ impl<Storage: driver::Storage> Filesystem<'_, Storage> {
     }
 
     /// Read attribute.
-    pub fn attribute(&self, path: &Path, id: u8) -> Result<Option<Attribute>> {
-        let mut attribute = Attribute::new(id);
-        let attr_max = crate::consts::ATTRBYTES_MAX;
+    pub fn attribute<'a>(
+        &self,
+        path: &Path,
+        id: u8,
+        buffer: &'a mut [u8],
+    ) -> Result<Option<Attribute<'a>>> {
+        let n = u32::try_from(buffer.len()).unwrap_or(u32::MAX);
 
         let return_code = unsafe {
             ll::lfs_getattr(
                 &mut self.alloc.borrow_mut().state,
                 path.as_ptr(),
                 id,
-                attribute.buffer_mut() as *mut _ as *mut c_void,
-                attr_max,
+                buffer as *mut _ as *mut c_void,
+                n,
             )
         };
 
         if return_code >= 0 {
-            attribute.set_size(return_code as usize);
-            return Ok(Some(attribute));
+            let total_size = usize::try_from(return_code).unwrap_or(usize::MAX);
+            return Ok(Some(Attribute::new(buffer, total_size)));
         }
         if return_code == ll::lfs_error_LFS_ERR_NOATTR {
             return Ok(None);
@@ -451,14 +455,14 @@ impl<Storage: driver::Storage> Filesystem<'_, Storage> {
     }
 
     /// Set attribute.
-    pub fn set_attribute(&self, path: &Path, attribute: &Attribute) -> Result<()> {
+    pub fn set_attribute(&self, path: &Path, id: u8, data: &[u8]) -> Result<()> {
         let return_code = unsafe {
             ll::lfs_setattr(
                 &mut self.alloc.borrow_mut().state,
                 path.as_ptr(),
-                attribute.id(),
-                attribute.data() as *const _ as *const c_void,
-                attribute.size() as u32,
+                id,
+                data as *const _ as *const c_void,
+                u32::try_from(data.len()).unwrap_or(u32::MAX),
             )
         };
 
@@ -1274,15 +1278,14 @@ mod tests {
                     {
                         println!("path: {:?}", entry.path());
 
-                        let mut attribute = Attribute::new(37);
-                        if entry.file_type().is_dir() {
-                            attribute.set_data(b"directory alarm");
+                        let attribute: &[u8] = if entry.file_type().is_dir() {
+                            b"directory alarm"
                         } else {
-                            attribute.set_data(b"ceci n'est pas une pipe");
                             // not 100% sure this is allowed, but if seems to work :)
                             fs.write(entry.path(), b"Alles neu macht n\xc3\xa4chstens der Mai")?;
-                        }
-                        fs.set_attribute(entry.path(), &attribute)?;
+                            b"ceci n'est pas une pipe"
+                        };
+                        fs.set_attribute(entry.path(), 37, attribute)?;
                     }
                 }
                 Ok(())
@@ -1301,7 +1304,8 @@ mod tests {
                         // fs.remove(entry.path())?;
                     }
 
-                    if let Some(attribute) = fs.attribute(entry.path(), 37)? {
+                    let mut buffer = [0; Attribute::MAX_SIZE as _];
+                    if let Some(attribute) = fs.attribute(entry.path(), 37, &mut buffer)? {
                         println!(
                             "attribute 37: {:?}",
                             core::str::from_utf8(attribute.data()).unwrap()

src/object_safe.rs

@@ -106,16 +106,21 @@ impl<S: Storage> DynFilesystem for Filesystem<'_, S> {
         )
     }
 
-    fn attribute(&self, path: &Path, id: u8) -> Result<Option<Attribute>> {
-        Filesystem::attribute(self, path, id)
+    fn attribute<'a>(
+        &self,
+        path: &Path,
+        id: u8,
+        buffer: &'a mut [u8],
+    ) -> Result<Option<Attribute<'a>>> {
+        Filesystem::attribute(self, path, id, buffer)
     }
 
     fn remove_attribute(&self, path: &Path, id: u8) -> Result<()> {
         Filesystem::remove_attribute(self, path, id)
     }
 
-    fn set_attribute(&self, path: &Path, attribute: &Attribute) -> Result<()> {
-        Filesystem::set_attribute(self, path, attribute)
+    fn set_attribute(&self, path: &Path, id: u8, data: &[u8]) -> Result<()> {
+        Filesystem::set_attribute(self, path, id, data)
     }
 
     fn read_dir_and_then_unit(&self, path: &Path, f: DirEntriesCallback<'_>) -> Result<()> {

src/tests.rs

@@ -431,38 +431,39 @@ fn attributes() {
     let mut storage = RamStorage::new(&mut backend);
     Filesystem::format(&mut storage).unwrap();
     Filesystem::mount_and_then(&mut storage, |fs| {
+        let mut buffer = [0; Attribute::MAX_SIZE as _];
+
         let filename = b"some.file\0".try_into().unwrap();
         fs.write(filename, &[])?;
-        assert!(fs.attribute(filename, 37)?.is_none());
-
-        let mut attribute = Attribute::new(37);
-        attribute.set_data(b"top secret");
+        assert!(fs.attribute(filename, 37, &mut buffer)?.is_none());
 
-        fs.set_attribute(filename, &attribute).unwrap();
-        assert_eq!(b"top secret", fs.attribute(filename, 37)?.unwrap().data());
+        let data = b"top secret";
+        fs.set_attribute(filename, 37, data).unwrap();
+        let attribute = fs.attribute(filename, 37, &mut buffer)?.unwrap();
+        assert_eq!(data, attribute.data());
+        assert_eq!(data.len(), attribute.total_size());
 
         // // not sure if we should have this method (may be quite expensive)
         // let attributes = unsafe { fs.attributes("some.file", &mut storage).unwrap() };
         // assert!(attributes[37]);
         // assert_eq!(attributes.iter().fold(0, |sum, i| sum + (*i as u8)), 1);
 
         fs.remove_attribute(filename, 37)?;
-        assert!(fs.attribute(filename, 37)?.is_none());
+        assert!(fs.attribute(filename, 37, &mut buffer)?.is_none());
 
         // // Directories can have attributes too
         let tmp_dir = b"/tmp\0".try_into().unwrap();
         fs.create_dir(tmp_dir)?;
 
-        attribute.set_data(b"temporary directory");
-        fs.set_attribute(tmp_dir, &attribute)?;
+        let data = b"temporary directory";
+        fs.set_attribute(tmp_dir, 37, data)?;
 
-        assert_eq!(
-            b"temporary directory",
-            fs.attribute(tmp_dir, 37)?.unwrap().data()
-        );
+        let attribute = fs.attribute(tmp_dir, 37, &mut buffer)?.unwrap();
+        assert_eq!(data, attribute.data());
+        assert_eq!(data.len(), attribute.total_size());
 
         fs.remove_attribute(tmp_dir, 37)?;
-        assert!(fs.attribute(tmp_dir, 37)?.is_none());
+        assert!(fs.attribute(tmp_dir, 37, &mut buffer)?.is_none());
 
         Ok(())
     })