Skip to content

chdir_syscall() and fchdir_syscall() updates#294

Merged
rennergade merged 12 commits intodevelopfrom
vlad_chdir
Jul 13, 2024
Merged

chdir_syscall() and fchdir_syscall() updates#294
rennergade merged 12 commits intodevelopfrom
vlad_chdir

Conversation

@ve1nard
Copy link
Copy Markdown
Contributor

@ve1nard ve1nard commented Jul 8, 2024

Description

Fixes # (issue)

The following changes more elaborate comments and new unit tests for chdir_syscall() and fchdir_syscall().

Type of change

  • More detailed comments for chdir_syscall() and fchdir_syscall().
  • New unit tests for chdir_syscall() and fchdir_syscall().
  • Removal of incrementing/decrementing refcount for cwd to allow the complete removal of cwd from the filesystem

How Has This Been Tested?

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_chdir_valid_args()
  • Test B - ut_lind_fs_chdir_removeddir()
  • Test C - ut_lind_fs_chdir_invalid_args()
  • Test D - ut_lind_fs_fchdir_valid_args()
  • Test E - ut_lind_fs_fchdir_invalid_args()

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
Copy Markdown
Contributor

@davidge20 davidge20 left a comment

Choose a reason for hiding this comment

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

Great job overall! Great tests as well! Minor comments from my end

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
Comment thread src/safeposix/syscalls/fs_calls.rs
@ve1nard ve1nard requested a review from davidge20 July 11, 2024 19:51
Copy link
Copy Markdown
Contributor

@davidge20 davidge20 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks

@ve1nard ve1nard requested review from rennergade and yashaswi2000 and removed request for yashaswi2000 July 11, 2024 21:07
Comment thread src/safeposix/filesystem.rs
Copy link
Copy Markdown
Contributor

@rennergade rennergade left a comment

Choose a reason for hiding this comment

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

This looks really good just want a few clarifications.

@ve1nard ve1nard requested a review from rennergade July 12, 2024 23:25
Copy link
Copy Markdown
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.

looks good. almost done.

Comment thread src/safeposix/syscalls/fs_calls.rs Outdated
Comment thread src/safeposix/syscalls/fs_calls.rs Outdated
@ve1nard ve1nard requested a review from yashaswi2000 July 13, 2024 16:16
Copy link
Copy Markdown
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

Copy link
Copy Markdown
Contributor

@rennergade rennergade left a comment

Choose a reason for hiding this comment

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

Awesome. Approved!

@rennergade rennergade merged commit c830d62 into develop Jul 13, 2024
Anway-Agte pushed a commit that referenced this pull request Jul 15, 2024
* updated syscall description

* removed refcount and finished the description

* testing chdir

* new tests for chdir and fchdir

* formatted

* removed decrementing refcount for cwd

* fixed according to the review

* merged develop and fixed typos

* fixed according to the review

---------

Co-authored-by: lind <lind@nyu.edu>
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