Open_syscall comments and tests by namanlalitnyu · Pull Request #264 · Lind-Project/safeposix-rust

namanlalitnyu · 2024-06-14T18:55:46Z

Description

Fixes # (issue)
The following changes include the tests and comments in the code for the "open_syscall" file system call under RustPosix.
The tests were added to cover all the possible scenarios that might happen when calling the file system_call open_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_open_empty_directory()
Test B - ut_lind_fs_open_nonexisting_parentdirectory_and_file()
Test C - ut_lind_fs_open_existing_parentdirectory_and_nonexisting_file()
Test D - ut_lind_fs_open_existing_file_without_flags()
Test E - ut_lind_fs_open_existing_file_with_flags()
Test F - ut_lind_fs_open_create_new_file_and_check_link_count()
Test G - ut_lind_fs_open_existing_file_with_o_trunc_flag()
Test H - ut_lind_fs_open_new_file_with_s_ifchar_flag()

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)

namanlalitnyu · 2024-06-14T18:57:52Z

Completed with the comments in the sys_call and added a few tests for it. But, still working on the remaining ones and will update the PR within some time once the other tests are completed.

JustinCappos

Good first cut. A few minor changes requested.

JustinCappos · 2024-06-14T19:34:49Z

+    // Function Arguments
+    // The open_syscall() receives three arguments:
+    // 1. Path - This argument points to a pathname naming the file.
+    //           For example: "/parentdir/file1" represents a file which will be either opened if existed or will be created at the given path.


Suggested change

// For example: "/parentdir/file1" represents a file which will be either opened if existed or will be created at the given path.

// For example: "/parentdir/file1" represents a file which will be either opened if it exists or will be created at the given path.

JustinCappos · 2024-06-14T19:39:01Z

+        // Walk through the absolute path which returns a tuple consisting of inode number of file (if it exists), and inode number of parent (if it exists)
        match metawalkandparent(truepath.as_path()) {
-            //If neither the file nor parent exists
+            // Case 1: When neither the file directory nor the parent directory exists


It's sort of weird to structure it like this because if the parent director doesn't exist, then the file clearly won't. Can you just check to make sure the parent dir exists and return an error otherwise?

If you just want clarity the tuple check in match statement would just be (.., None) since the second field is the parent and the first field doesn't matter. Thought metawalkparent will always return (None, None) if the parent doesn't exist since like you said if we don't find a parent we obviously don't have the file.

JustinCappos · 2024-06-18T22:33:00Z

@namanlalitnyu A few minor github things...

feel free to resolve conversations after you think you've addressed something.
it's better to address a question with a comment in the code (when this makes sense) instead of only on the issue. When we read the code later, we've lost the q/a.

This isn't a huge problem here (you've done a nice job!), but I wanted to bring them up since they are top-of-mind)

namanlalitnyu · 2024-06-19T02:31:46Z

@namanlalitnyu A few minor github things...

feel free to resolve conversations after you think you've addressed something.

it's better to address a question with a comment in the code (when this makes sense) instead of only on the issue. When we read the code later, we've lost the q/a.

This isn't a huge problem here (you've done a nice job!), but I wanted to bring them up since they are top-of-mind)

Sure professor, thanks for the suggestions. Will do that!

rennergade · 2024-06-20T14:23:52Z

+                                    if let interface::RustHashEntry::Occupied(occ) = entry {
+                                        occ.remove_entry();
+                                    }



remove this extra whitespace

I checked, and there is not any extra whitespace here; it is just because of the indentation as the code section has moved inside the matching block.

rennergade · 2024-06-20T14:26:07Z

-                            let entry = FILEOBJECTTABLE.entry(inodenum);
-                            if let interface::RustHashEntry::Occupied(occ) = &entry {
-                                occ.get().close().unwrap();
+                                // Doubt: Why are we removing the previous entry from the FileObjectTable and then inserting a new file with size 0. 


This is hacky way to truncate since a new file will be size 0 instead of actually adjusting the filesize and pointer. I would comment that thats what were doing but also label it as a possible bug.

Updated the comment.

rennergade

Two minor changes, should be able to get this merged today.

JustinCappos

Looks fine after the return values / panics in the comment block is updated.

JustinCappos · 2024-06-20T15:22:10Z

+    /// Upon successful completion of this call, a file descriptor is returned which points the file which is opened.
+    /// Otherwise, an error with a proper errorNumber and errorMessage is returned based on the different scenarios.
+    ///
+    /// for more detailed description of all the commands and return values, see 


We don't cover all the cases of open though, do we? What errors are returned and which are not?

No, we don't cover all the errors. Manpage has a lot of them mentioned which are remaining.

namanlalitnyu · 2024-06-20T17:37:50Z

Looks fine after the return values / panics in the comment block is updated.

Updated the comments with error values and panics in the comment block.

yashaswi2000

LGTM!

rennergade · 2024-06-20T18:17:08Z

I think this looks ready. Good job!

Added comments for open_syscall and wrote few tests

e8ecd2a

namanlalitnyu requested review from JustinCappos, Yaxuan-w, rennergade and yashaswi2000 June 14, 2024 18:56

JustinCappos requested changes Jun 14, 2024

View reviewed changes

namanlalitnyu requested review from davidge20, qianxichen233, rupeshkoushik07 and ve1nard June 15, 2024 17:54

ve1nard reviewed Jun 15, 2024

View reviewed changes

Comment thread src/safeposix/syscalls/fs_calls.rs

Comment thread src/safeposix/syscalls/fs_calls.rs Outdated

Comment thread src/safeposix/syscalls/fs_calls.rs Outdated

ve1nard reviewed Jun 15, 2024

View reviewed changes

Comment thread src/tests/fs_tests.rs

Added more tests for open_syscall

1e929bc

namanlalitnyu requested a review from JustinCappos June 17, 2024 17:51

Updated comments with RustDoc format

f58a2f6

namanlalitnyu closed this Jun 19, 2024

namanlalitnyu reopened this Jun 19, 2024

Added checks for fd

963ef40

yashaswi2000 reviewed Jun 20, 2024

View reviewed changes

Comment thread src/safeposix/syscalls/fs_calls.rs Outdated

Comment thread src/safeposix/syscalls/fs_calls.rs

rennergade reviewed Jun 20, 2024

View reviewed changes

rennergade requested changes Jun 20, 2024

View reviewed changes

namanlalitnyu added 2 commits June 20, 2024 14:52

Reordering of the fd errors and added panic

a25cc72

Updated comments for potential file truncate bug

b7aa6fa

JustinCappos approved these changes Jun 20, 2024

View reviewed changes

namanlalitnyu requested a review from rennergade June 20, 2024 15:29

Updated Comments with error values

164ce39

yashaswi2000 approved these changes Jun 20, 2024

View reviewed changes

rennergade merged commit 546acc5 into develop Jun 20, 2024

namanlalitnyu mentioned this pull request Jun 26, 2024

Documentation and testing enhancement for RustPosix #260

Open

	// For example: "/parentdir/file1" represents a file which will be either opened if existed or will be created at the given path.
	// For example: "/parentdir/file1" represents a file which will be either opened if it exists or will be created at the given path.

Conversation

namanlalitnyu commented Jun 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

How Has This Been Tested?

Checklist:

Uh oh!

namanlalitnyu commented Jun 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JustinCappos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JustinCappos commented Jun 18, 2024

Uh oh!

namanlalitnyu commented Jun 19, 2024

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rennergade left a comment

Choose a reason for hiding this comment

Uh oh!

JustinCappos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

namanlalitnyu commented Jun 20, 2024

Uh oh!

yashaswi2000 left a comment

Choose a reason for hiding this comment

Uh oh!

rennergade commented Jun 20, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

namanlalitnyu commented Jun 14, 2024 •

edited

Loading

namanlalitnyu commented Jun 14, 2024 •

edited

Loading