Improve error logs for unsupported operations: File Overwrite, Random…

… Write, Directory Shadowing, Unlink (without mount option) (#699) * Improved error logs for unsupported operations Signed-off-by: Ankit Saurabh <[email protected]> * Improved Invalid Inode Status error message Signed-off-by: Ankit Saurabh <[email protected]> * Reformatted the entry if match Signed-off-by: Ankit Saurabh <[email protected]> * Combined the match for next and last entry Signed-off-by: Ankit Saurabh <[email protected]> * Removed extra line from warn message Signed-off-by: Ankit Saurabh <[email protected]> --------- Signed-off-by: Ankit Saurabh <[email protected]>
awslabs · Jan 17, 2024 · 45cad69 · 45cad69
1 parent 804b8d0
commit 45cad69
Show file tree

Hide file tree

Showing 6 changed files with 24 additions and 13 deletions.
diff --git a/mountpoint-s3-client/src/object_client.rs b/mountpoint-s3-client/src/object_client.rs
@@ -365,7 +365,7 @@ pub enum RestoreStatus {
 ///
 /// See [Object](https://docs.aws.amazon.com/AmazonS3/latest/API/API_Object.html) in the *Amazon S3
 /// API Reference* for more details.
-#[derive(Debug)]
+#[derive(Debug, Clone)]
 pub struct ObjectInfo {
     /// Key for this object.
     pub key: String,

diff --git a/mountpoint-s3/src/fs.rs b/mountpoint-s3/src/fs.rs
@@ -1198,7 +1198,10 @@ where
 
     pub async fn unlink(&self, parent_ino: InodeNo, name: &OsStr) -> Result<(), Error> {
         if !self.config.allow_delete {
-            return Err(err!(libc::EPERM, "deletes are disabled"));
+            return Err(err!(
+                libc::EPERM,
+                "Deletes are disabled. Use '--allow-delete' mount option to enable it."
+            ));
         }
         Ok(self.superblock.unlink(&self.client, parent_ino, name).await?)
     }

diff --git a/mountpoint-s3/src/fs/error.rs b/mountpoint-s3/src/fs/error.rs
@@ -120,6 +120,7 @@ impl ToErrno for InodeError {
             // Not obvious what InodeNotWritable, InodeAlreadyWriting, InodeNotReadableWhileWriting should be.
             // EINVAL or EROFS would also be reasonable -- but we'll treat them like sealed files.
             InodeError::InodeNotWritable(_) => libc::EPERM,
+            InodeError::InodeInvalidWriteStatus(_) => libc::EPERM,
             InodeError::InodeAlreadyWriting(_) => libc::EPERM,
             InodeError::InodeNotReadableWhileWriting(_) => libc::EPERM,
             InodeError::InodeNotWritableWhileReading(_) => libc::EPERM,

diff --git a/mountpoint-s3/src/inode.rs b/mountpoint-s3/src/inode.rs
@@ -1174,7 +1174,7 @@ impl WriteHandle {
 
                 Ok(())
             }
-            _ => Err(InodeError::InodeNotWritable(inode.err())),
+            _ => Err(InodeError::InodeInvalidWriteStatus(inode.err())),
         }
     }
 }
@@ -1555,6 +1555,8 @@ pub enum InodeError {
     FileAlreadyExists(InodeErrorInfo),
     #[error("inode {0} is not writable")]
     InodeNotWritable(InodeErrorInfo),
+    #[error("Invalid state of inode {0} to be written. Aborting the write.")]
+    InodeInvalidWriteStatus(InodeErrorInfo),
     #[error("inode {0} is already being written")]
     InodeAlreadyWriting(InodeErrorInfo),
     #[error("inode {0} is not readable while being written")]

diff --git a/mountpoint-s3/src/inode/readdir.rs b/mountpoint-s3/src/inode/readdir.rs
@@ -198,7 +198,7 @@ impl ReaddirHandle {
 
 /// A single entry in a readdir stream. Remote entries have not yet been converted to inodes -- that
 /// should be done lazily by the consumer of the entry.
-#[derive(Debug)]
+#[derive(Debug, Clone)]
 enum ReaddirEntry {
     RemotePrefix { name: String },
     RemoteObject { name: String, object_info: ObjectInfo },
@@ -406,7 +406,7 @@ mod ordered {
         local: LocalIter,
         next_remote: Option<ReaddirEntry>,
         next_local: Option<ReaddirEntry>,
-        last_name: Option<String>,
+        last_entry: Option<ReaddirEntry>,
     }
 
     impl ReaddirIter {
@@ -421,7 +421,7 @@ mod ordered {
                 local: LocalIter::new(local_entries),
                 next_remote: None,
                 next_local: None,
-                last_name: None,
+                last_entry: None,
             }
         }
 
@@ -454,19 +454,24 @@ mod ordered {
                 };
 
                 // Deduplicate the entry we want to return
-                match next {
-                    Some(entry) => {
-                        if self.last_name.as_deref() == Some(entry.name()) {
+                match (next, &self.last_entry) {
+                    (Some(entry), Some(last_entry)) => {
+                        if last_entry.name() == entry.name() {
                             warn!(
-                                "{} is shadowed by another entry with the same name and will be unavailable",
+                                "{} is omitted because another {} exist with the same name",
                                 entry.description(),
+                                last_entry.description(),
                             );
                         } else {
-                            self.last_name = Some(entry.name().to_owned());
+                            self.last_entry = Some(entry.clone());
                             return Ok(Some(entry));
                         }
                     }
-                    None => return Ok(None),
+                    (Some(entry), None) => {
+                        self.last_entry = Some(entry.clone());
+                        return Ok(Some(entry));
+                    }
+                    _ => return Ok(None),
                 }
             }
         }

diff --git a/mountpoint-s3/src/upload.rs b/mountpoint-s3/src/upload.rs
@@ -49,7 +49,7 @@ pub enum UploadWriteError<E: std::error::Error> {
     #[error("put request failed")]
     PutRequestFailed(#[from] E),
 
-    #[error("out of order write; expected offset {expected_offset:?} but got {write_offset:?}")]
+    #[error("out of order write is NOT supported by Mountpoint, aborting the upload; expected offset {expected_offset:?} but got {write_offset:?}")]
     OutOfOrderWrite { write_offset: u64, expected_offset: u64 },
 
     #[error("object exceeded maximum upload size of {maximum_size} bytes")]