Description, Comments and tests for access_syscall & `rename_syscal…

…l` (#319)

* added descriptions and tests for access and rename syscalls

* tweaked description

* addressed comments

* added BUG comments

* fixes for failing tests

---------

Co-authored-by: Nicholas Renner <nicholassrenner@gmail.com>
Co-authored-by: Yashaswi Makula <Yashaswi.makula@gmail.com>

src/safeposix/filesystem.rs

@@ -395,7 +395,7 @@ pub fn create_log() {
     logobj.replace(log_mapobj);
 }
 
-// Serialize New Metadata to CBOR, write to logfile
+/// Serialize New Metadata to CBOR, write to logfile
 pub fn log_metadata(metadata: &FilesystemMetadata, inodenum: usize) {
     let serialpair: (usize, Option<&Inode>);
     let entrybytes;

src/safeposix/syscalls/fs_calls.rs

@@ -2815,12 +2815,53 @@ impl Cage {
     }
 
     //------------------------------------ACCESS SYSCALL------------------------------------
+    /// ### Description
+    ///
+    /// `access_syscall` checks the accessibility of the file specified by
+    /// `path` according to the given `amode`. The assumption while running this
+    /// command is that the current user owns the file.
+    ///
+    /// ### Arguments
+    ///
+    /// It accepts two parameters:
+    /// * `path` - A string slice that specifies the file path for which
+    ///   accessibility is to be checked.
+    /// * `amode` - A bitmask value specifying the accessibility check to be
+    ///   performed. It can be a combination of the following constants:
+    ///     * `R_OK` - Check for read permission.
+    ///     * `W_OK` - Check for write permission.
+    ///     * `X_OK` - Check for execute (search) permission.
+    ///
+    /// ### Returns
+    ///
+    /// For a successful call, the return value will be 0, indicating that the
+    /// requested access is allowed. On error, a negative errno is returned to
+    /// indicate the error.
+    ///
+    /// ### Errors
+    ///
+    /// * `ENOENT` - The file specified by `path` does not exist or the path is
+    ///   invalid.
+    /// * `EACCES` - The requested access would be denied to the file due to
+    ///   insufficient permissions.
+    ///
+    /// ### Panics
+    ///
+    /// * There are no known panics in this system call.
+    ///
+    /// For more detailed description of all the commands and return values,
+    /// refer to the access syscall man page [here](https://man7.org/linux/man-pages/man2/access.2.html).
 
     pub fn access_syscall(&self, path: &str, amode: u32) -> i32 {
         let truepath = normpath(convpath(path), self);
 
         //Walk the file tree to get inode from path
         if let Some(inodenum) = metawalk(truepath.as_path()) {
+            // BUG: We don't support F_OK as a valid amode flag, which when passed we need
+            // to check only whether the file exists or not
+
+            // will not panic since check for existence in table already happened in
+            // metawalk
             let inodeobj = FS_METADATA.inodetable.get(&inodenum).unwrap();
 
             //Get the mode bits if the type of the inode is sane
@@ -2835,13 +2876,19 @@ impl Cage {
 
             //Construct desired access bits (i.e. 0777) based on the amode parameter
             let mut newmode: u32 = 0;
+            // if we are checking for execute rights
             if amode & X_OK == X_OK {
+                // update newmode with S_IXUSR flags
                 newmode |= S_IXUSR;
             }
+            // if we are checking for write rights
             if amode & W_OK == W_OK {
+                // update newmode with S_IWUSR flags
                 newmode |= S_IWUSR;
             }
+            // if we are checking for read rights
             if amode & R_OK == R_OK {
+                // update newmode with S_IRUSR flags
                 newmode |= S_IRUSR;
             }
 
@@ -2857,6 +2904,7 @@ impl Cage {
                 )
             }
         } else {
+            // if incase the file isn't found
             syscall_error(
                 Errno::ENOENT,
                 "access",
@@ -4654,6 +4702,38 @@ impl Cage {
     }
 
     //------------------RENAME SYSCALL------------------
+    /// ### Description
+    ///
+    /// `rename_syscall` renames a file or directory specified by `oldpath` to
+    /// `newpath`.
+    ///
+    /// ### Arguments
+    ///
+    /// It accepts two parameters:
+    /// * `oldpath` - A string slice that specifies the current name and
+    ///   location of the file or directory.
+    /// * `newpath` - A string slice that specifies the new name and location
+    ///   for the file or directory.
+    ///
+    /// ### Returns
+    ///
+    /// For a successful call, the return value will be 0, indicating that the
+    /// rename operation was successful. On error, a negative errno is returned
+    /// to indicate the error.
+    ///
+    /// ### Errors
+    ///
+    /// * `ENOENT` - The `oldpath` or `newpath` is null, i.e. not provided.
+    /// * `EEXIST` - The `oldpath` does not exist.
+    /// * `EBUSY` - Cannot rename the root directory.
+    /// * `EOPNOTSUPP` - Cannot move the file or directory to another directory.
+    ///
+    /// ### Panics
+    ///
+    /// * There are no known panics in this function.
+    ///
+    /// For more detailed description of all the commands and return values,
+    /// refer to the rename syscall man page [here](https://man7.org/linux/man-pages/man2/rename.2.html).
 
     pub fn rename_syscall(&self, oldpath: &str, newpath: &str) -> i32 {
         if oldpath.len() == 0 {
@@ -4676,7 +4756,12 @@ impl Cage {
                 // make sure file is not moved to another dir
                 // get inodenum for parent of new path
                 let (_, new_par_inodenum) = metawalkandparent(true_newpath.as_path());
+
+                // BUG: the rename according to the spec supports moving of files from one
+                // parent to another, so the below behavior is not as per spec
+
                 // check if old and new paths share parent
+                // if they don't, then that is not allowed
                 if new_par_inodenum != Some(parent_inodenum) {
                     return syscall_error(
                         Errno::EOPNOTSUPP,
@@ -4685,7 +4770,9 @@ impl Cage {
                     );
                 }
 
+                // get parent directory inode object
                 let pardir_inodeobj = FS_METADATA.inodetable.get_mut(&parent_inodenum).unwrap();
+
                 if let Inode::Dir(parent_dir) = &*pardir_inodeobj {
                     // add pair of new path and its inodenum to filename-inode dict
                     parent_dir.filename_to_inode_dict.insert(
@@ -4707,7 +4794,11 @@ impl Cage {
                             .unwrap()
                             .to_string(),
                     );
+
+                    // drop the ref to the parent dir inode object
                     drop(pardir_inodeobj);
+
+                    // log and update metadata
                     log_metadata(&FS_METADATA, parent_inodenum);
                 }
                 NET_METADATA.domsock_paths.insert(true_newpath);

src/tests/fs_tests.rs

@@ -4480,9 +4480,12 @@ pub mod fs_tests {
         let mut statdata = StatData::default();
 
         // test out whether an error is output for a non existent fd (1000)
-        // (ENOENT[-2])
+        // (EBADF[-9])
         let non_existent_fd = 1000;
-        assert_eq!(cage.fstat_syscall(non_existent_fd, &mut statdata), -9);
+        assert_eq!(
+            cage.fstat_syscall(non_existent_fd, &mut statdata),
+            syscall_error(Errno::EBADF, "fstat", "test_failure")
+        );
 
         // setting up directory inode object '/tmp' for testing fstat_syscall with a
         // directory
@@ -4524,7 +4527,10 @@ pub mod fs_tests {
         assert_eq!(cage.bind_syscall(socketfd, &socket), 0);
 
         // Errno::EOPNOTSUPP : -95
-        assert_eq!(cage.fstat_syscall(socketfd, &mut statdata), -95);
+        assert_eq!(
+            cage.fstat_syscall(socketfd, &mut statdata),
+            syscall_error(Errno::EOPNOTSUPP, "fstat", "test_failure")
+        );
 
         // Clean up
         assert_eq!(cage.close_syscall(socketfd), 0);
@@ -4548,7 +4554,7 @@ pub mod fs_tests {
         // (ENOENT[-2])
         assert_eq!(
             cage.statfs_syscall("non_existent_file_path", &mut fsdata),
-            syscall_error(Errno::ENOENT, "stat", "test_failure")
+            syscall_error(Errno::ENOENT, "statfs", "test_failure")
         );
 
         // setting up inode object "/tmp/generic" for testing statfs_syscall
@@ -4576,7 +4582,7 @@ pub mod fs_tests {
         let non_existent_fd = 1000;
         assert_eq!(
             cage.fstatfs_syscall(non_existent_fd, &mut fsdata),
-            syscall_error(Errno::EBADF, "stat", "test_failure")
+            syscall_error(Errno::EBADF, "fstatfs", "test_failure")
         );
 
         // setting up generic inode object "/tmp/generic" for testing fstat_syscall with
@@ -4600,7 +4606,7 @@ pub mod fs_tests {
         // Errno::EBADF : -9
         assert_eq!(
             cage.fstatfs_syscall(socketfd, &mut fsdata),
-            syscall_error(Errno::EBADF, "stat", "test_failure")
+            syscall_error(Errno::EBADF, "fstatfs", "test_failure")
         );
 
         // Clean up
@@ -4611,4 +4617,85 @@ pub mod fs_tests {
         lindrustfinalize();
         return;
     }
+
+    #[test]
+    pub fn ut_lind_fs_access_syscall_tests() {
+        // acquiring a lock on TESTMUTEX prevents other tests from running concurrently,
+        // and also performs clean env setup
+        let _thelock = setup::lock_and_init();
+
+        let cage = interface::cagetable_getref(1);
+
+        // test out whether an error is output for a non existent file path
+        // (ENOENT[-2])
+        assert_eq!(
+            cage.access_syscall("non_existent_file_path", S_IRWXA),
+            syscall_error(Errno::ENOENT, "access", "test_failure")
+        );
+
+        // setting up inode object "/tmp/generic" for testing statfs_syscall
+        let generic_path = "/tmp/generic";
+        let creat_fd = cage.creat_syscall(generic_path, S_IRWXA);
+        assert!(creat_fd > 0);
+        // set mode to some value
+        assert_eq!(cage.access_syscall(generic_path, S_IRWXA), 0);
+
+        // change mode
+        assert_eq!(cage.chmod_syscall(generic_path, S_IRGRP), 0);
+        assert_eq!(
+            cage.access_syscall(generic_path, S_IRWXA),
+            syscall_error(Errno::EACCES, "access", "incorrect access")
+        );
+
+        lindrustfinalize();
+        return;
+    }
+
+    #[test]
+    pub fn ut_lind_fs_rename_syscall_tests() {
+        // acquiring a lock on TESTMUTEX prevents other tests from running concurrently,
+        // and also performs clean env setup
+        let _thelock = setup::lock_and_init();
+
+        let cage = interface::cagetable_getref(1);
+
+        // test out whether an error is output for a non existent file path
+        // (EEXIST[-17])
+        assert_eq!(
+            cage.rename_syscall("non_existent_file_path", "non-existent-target"),
+            syscall_error(Errno::EEXIST, "rename", "test_failure")
+        );
+
+        // empty inputs for rename
+        assert_eq!(
+            cage.rename_syscall("", ""),
+            syscall_error(Errno::ENOENT, "rename", "empty inputs")
+        );
+
+        // root path provided
+        assert_eq!(
+            cage.rename_syscall("/", "rename-to-this"),
+            syscall_error(Errno::EBUSY, "rename", "cant deal with root")
+        );
+
+        // setting up inode object "/tmp/generic" for testing statfs_syscall
+        let generic_path = "/tmp/generic";
+        let creat_fd = cage.creat_syscall(generic_path, S_IRWXA);
+        assert!(creat_fd > 0);
+
+        // try to rename to different parent
+        assert_eq!(
+            cage.rename_syscall("/tmp", generic_path),
+            syscall_error(Errno::EOPNOTSUPP, "rename", "cant move directories")
+        );
+
+        // normal rename
+        assert_eq!(cage.rename_syscall("/tmp/generic", "/tmp/generic1"), 0);
+
+        // clean up
+        cage.close_syscall(creat_fd);
+
+        lindrustfinalize();
+        return;
+    }
 }