Skip to content

Added tests for mkdir_syscall #255

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jun 10, 2024
Merged

Added tests for mkdir_syscall #255

merged 5 commits into from
Jun 10, 2024

Conversation

namanlalitnyu
Copy link
Contributor

@namanlalitnyu namanlalitnyu commented May 28, 2024

Description

Fixes # (issue)
The following changes include the tests for the "mkdir" file system call under RustPosix.
The tests were added to cover all the possible scenarios that might happen when calling the file system_call mkdir_syscall.

Type of change

  • This change just contains the tests for an existing file system call.

How Has This Been Tested?

Inorder to run the tests, we need to run cargo test --lib command inside the safeposix-rust directory.

All the tests are present under this directory: lind_project/src/safeposix-rust/src/tests/fs_tests.rs

  • Test A - ut_lind_fs_mkdir_empty_directory()
  • Test B - ut_lind_fs_mkdir_nonexisting_directory()
  • Test C - ut_lind_fs_mkdir_existing_directory()
  • Test D - ut_lind_fs_mkdir_invalid_modebits()
  • Test E - ut_lind_fs_mkdir_success()
  • Test F - ut_lind_fs_mkdir_using_symlink()

Checklist:

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • Any dependent changes have been added to a pull request and/or merged in other modules (native-client, lind-glibc, lind-project)

Copy link
Member

@Yaxuan-w Yaxuan-w left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall logic makes sense to me, lgtm so approved

Copy link
Member

@JustinCappos JustinCappos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Can you add a test for doing mkdir through a symlink?

@yashaswi2000
Copy link
Contributor

Great starter! i think we can bundle the comments update for the syscall in this PR itself.

@namanlalitnyu
Copy link
Contributor Author

Looks great! Can you add a test for doing mkdir through a symlink?

Added the test for the same.

Comment on lines 1312 to 1314
// Create a directory which will be referred to as originaldir
let fd = cage.open_syscall("/originaldir", O_CREAT | O_EXCL | O_WRONLY, S_IRWXA);
assert_eq!(cage.write_syscall(fd, str2cbuf("hi"), 2), 2);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This creates a file, not a directory, right?

Copy link
Contributor Author

@namanlalitnyu namanlalitnyu May 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This creates a file, not a directory, right?
Sorry, my bad...I just rechecked the other references for this function call and it creates a file, not a directory. Updated the changes.

{
parentdir
.filename_to_inode_dict
.insert(filename, newinodenum);
parentdir.linkcount += 1;
parentdir.ctime = time;
parentdir.mtime = time;
// Here, update the ctime and mtime for the parent directory as well
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Man pages for this call contain the update to the timestamps as well - https://docs.oracle.com/cd/E86824_01/html/E54765/mkdir-2.html

assert_eq!(cage.mkdir_syscall("/parentdir/dir", S_IRWXA), 0);

// Get the stat data for the child directory and check for inode link count to be 3 initially
let mut statdata2 = StatData::default();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated this test to verify the "link count" when a new directory is created.

@@ -187,6 +187,8 @@ impl Cage {
if path.len() == 0 {
return syscall_error(Errno::ENOENT, "mkdir", "given path was null");
}

// Get the absolute path
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is done to get the absolute path from the root directory of Lind so that we can trace the entire path and check for errors.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, please explain why we need to do this to check for errors in the code comment.

I can tell you're getting the absolute path from the code, but don't know why you are getting it until you tell me...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, will update the comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the comments.

Copy link
Member

@JustinCappos JustinCappos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good. I like the tests particularly. A few comments could be improved, but it mostly looks ready.

@yashaswi2000
Copy link
Contributor

A brief intro and info about the arguments, Flags being used, purpose and return types above the function signature will make the documentation whole. you can refer to the man pages to add this, potentially also include any limitations and unsupported features we have today as a Note.

other than that, everything looks good!

@namanlalitnyu
Copy link
Contributor Author

A brief intro and info about the arguments, Flags being used, purpose and return types above the function signature will make the documentation whole. you can refer to the man pages to add this, potentially also include any limitations and unsupported features we have today as a Note.

other than that, everything looks good!

Done, added relevant information before the system call and added more context inside the function call for better understanding. Couldn't find any limitations / unsupported features for now, but will update it once I find in future.

Copy link
Contributor

@yashaswi2000 yashaswi2000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@yashaswi2000 yashaswi2000 merged commit 69a31d5 into develop Jun 10, 2024
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants